Add interface to allow mocking

[jclebreton]

• add mock file to simplify testing for lib users
• for backward compatibility, add a method 'AsInterface' to cast SessionManager to interface
• add some tests

And also remove useless variable inside legacy constructor.

[titouanfreville]

related to #97

[verygoodsoftwarenotvirus]

It's convention in Go code to name interfaces with an -er suffix, and not an I prefix. For example, nearly every standard library interface. Changing the name of this structure to, well, SessionManager is the most idiomatic, imho.

[jclebreton]

Hello @verygoodsoftwarenotvirus. Yes I know this rule very well but SessionManager and Session are already taken...and I don't want break backward compatibility. Do you have a better idea to rename it ? SessionMocker ? ;-)

[alexedwards]

Hi,

Thanks for your work on this and sending the PR.

I'm not sure that this approach is the right way to solve issue #97. The three things that I'm worried about are:

1. The number of dependencies this introduces. Currently the core scs package has no dependencies, this PR adds 6. More dependencies mean that there is more code that can go wrong, more things to maintain, and more potential attack vectors. I'm not against adding dependencies when it's necessary, but I feel like there should be a way to allow mocking without needing all of these:

• github.com/davecgh/go-spew
• github.com/pmezard/go-difflib
• github.com/stretchr/objx
• github.com/stretchr/testify
• gopkg.in/check.v1
• gopkg.in/yaml.v2

2. If someone wants to mock the SessionManager, they would need to create their own mock which implements that very long list of methods (GetString(), PopString() etc). That doesn't seem very clean or easy, especially if you are only using a couple of them.

3. If we add another method to the SessionManager in the future (like GetInt64() for example), then we will also need to update ISession to include it. In turn, that would break people's existing mock implementations. Basically, if we go down this route then it would also mean accepting that any further additions would be backwards incompatible changes. I'm not sure that's a corner we should back ourselves into.

If you're testing your handlers by using end-to-end tests (which encompass the LoadAndSave() middleware, then I can't really see the need to mock the session manager. You could always set the session store to memstore or your own mock store to avoid hitting the DB.

If you want to test the handler completely standalone, then I agree that things are awkward because the http.Request that you are using for the test won't include the context object required for the session manager to work and it will result in a panic. To fix that, we could make SCS provide a MockRequest() function which gives you a http.Request with the appropriate context. It would be a smaller change, avoid the problems outlined above, but still make it possible to conduct standalone unit tests.

[alexedwards]

N.b. The -er convention only applies to interfaces with one method: https://golang.org/doc/effective_go.html#interface-names

[titouanfreville]

@alexedwards That's why we provide the mock interface. As we don't require the user to create its own mock, it will not need to implement the heavy struct mock nor risk breaking without introducing a breaking change on the methods already existing. Though it's also why there are more dependencies so it can be a choice.

I think we can also choose to document the process to create the interface as long as it doesn't need to return interfaces defined in SCS and its dependencies. If so, we could define the interface in our own projects for the part we need to test and SCS will be able to comply to it.

For methods returning SCS's defined interfaces, we will require them to implement their own interfaces and returned them into the methods to be able to mock them.

[titouanfreville]

Also, it doesn't seems that dependencies required for test such as mock will have an impact on the global system: https://github.com/golang/go/issues/26913

Perhaps it will require a build tag though for this case as the mock implementation should still be present in the build otherwise (I think ?)

[jclebreton]

Thanks @alexedwards and @titouanfreville for your comments.

I have done a new commit to:

• reduce number of dependencies (if the mock is inside a dedicated package and not called by scs package, go module will not include automatically)
• Rename interface to MockableSession

[jclebreton]

Hi @alexedwards. Is this PR still alive or not? What is missing to be merged?

[alexedwards]

I'm going to close this PR. This isn't something that I'll ever use myself, I'm afraid that I don't understand the code (and therefore am not comfortable maintaining it), and I don't want to add more to the core API footprint. Sorry.