codyoss
commented
2 years ago Hey @snargleplax thank you for providing a detailed report. This was really helpful in understanding your use-case. I am sorry this change seemed to break some of your existing workflows.
I think this is kind of a case of Hyrum's Law. The intended behavior of using this library has always been that the user should be calling ctrl.Finish(), as without it nothing is verified. For this reason I think this is a case of unspecified behavior and does not constitute this being a breaking change.
That said I would like to work with you to see if there are any changes to the library we could make to directly support use-cases like yours. I don't think that something like a stub controller is out of the question. There has also been suggestions in that past that maybe we should support a version of NewController that takes functional options to configure behavior (see #238). I think something like something like one of these solutions sounds doable. What do you think? If we wanted to move forward with something like that I suggest a new issue be created for discussion or we start talking more seriously on the linked issue above.
snargleplax
Actual behavior
Test that passed on previous minor version 1.5.0 fails with errors about missing expected calls.
Expected behavior
Tests that passed on previous minor version pass on 1.6.0 without code changes.
To Reproduce
Controller.FinishAdditional Information
Discussion
The change was introduced here in response to this issue.
On that issue, there was some brief discussion of whether omitting a call to
Controller.Finishwas a valid use case. @prashantv, who opened the issue, described the use case as "arcane." @cvgw responded with the assertion that if this happens, it is probably always a mistake.There doesn't seem to have been anyone at the time giving any serious consideration to the possibility that someone might be legitimately using it this way, or that changing the behavior in this way would violate semantic versioning by breaking large numbers of existing tests. My team, though, makes extensive and intentional use of a testing pattern that relies on exactly this. I'll talk a bit about how we got there, because I think it speaks effectively to the legitimatacy of this use case.
From an earlier version of our team, we had inherited a decent-sized body of tests that made heavy use of mocks, and those all called
Controller.Finishin the expected fashion. We found, partly due to unrelated design issues that caused high dependency fan-out, that these tests were brittle and laborious to maintain. We'd make a change, and spend a bunch of time fixing up expectations we didn't actually care about, just to provide "life support" for the actual test scenario.I got sick of this and wondered whether we really wanted mocks at all, or just stubs -- the crucial difference being that stubs provide only behavior, not verification. In probably over 90% of cases, we have no interest in verifying that calls have been made. That verification stands outside the intentionality we desire, and interferes with the test design ideal of one logical assertion (one reason to fail) per test. Verifying mocks also meant adding extra control flow to avoid setting expectations in error cases, etc. So we started experimenting with omitting the call to
Controller.Finish(), and right away found that our tests started to become simpler and more focused on our actual intent. We've been very happy with the reduction in boilerplate, irrelevant breakage, etc.So, I think it's an extremely reasonable use case. The question is, did this change break semantic versioning as defined for Go modules? And whether or not it did, should something be done to accommodate cases like mine? We got this version bump as a side-effect of upgrading another package, and did not expect to suddenly have to go update dozens and dozens of tests and abandon a test design approach we've been migrating in the direction of for multiple years.
The Go modules documentation states:
Is the change backwards-incompatible? Judging by all my broken tests, I'd say so. Is it a change to the module's public interface or documented functionality? That's a little bit more of a question.
As of at least 1.3.1 (the version we had been on), the package GoDoc does state that
Finishshould be called:The GoDoc for
Finishitself says:More recently, this was added:
Frankly, I think there's an issue here on the face of it, even without reference to my use case. If
Finishis not idempotent and can only be invoked once, then according to this documentation it would be incorrect to invoke it anywhere in the presence of other code that implicitly does so. The currentREADME.mdstates:So if one goes by that, it would seem to follow that any previously existing code became "incorrect" (for Go 1.14+) on mockgen 1.5.0 (shouldn't that be gomock 1.6.0+? Not sure why it refers to "mockgen" there instead -- seems like a matter for the runtime library, not the code generator). I'm guessing that that's not the case, and that these docs just aren't quite self-consistent, but I think it underscores the fact that the documentation does not provide adequate clarity to justify going by a strict interpretation that any omission of call to
Controller.Finish()is incorrect and not intended to be supported, even though it has worked fine that way for quite some time.I should acknowledge that the
README.mddoes include an example (with no particular discussion) of how to do stubs. What I take from that example is that the intended approach is to append.AnyTimes()to every single expectation. For usage like ours, that's an awful lot of boilerplate compared to simply not callingFinish. I won't dispute that it can be used to get that behavior, but it seems manifestly undesirable and we'd have to edit hundreds of lines of code to fix that up everywhere now, not to mention committing to do so in perpetuity and abandon our longstanding lighter-weight approach.So, what to do? Unfortunately, since this version has been released, the bell has to some extent been rung. Even if one accepts my point of view that this change violated semantic versioning for a reasonable use case based on a reasonable read of the docs, it's now out in the wild and there are presumably many teams already relying on the new behavior, who would be similarly disrupted by reversing it.
One option would be to retroactively deem this change to be backwards-incompatible, and handle this strictly through the Go module mechanisms. I guess in those terms the least disruptive thing would actually involve two major version bumps -- you'd introduce v2, which reinstates the old behavior so that someone in my position can use that without requiring changes to v1. And then you'd have v3, which operates the same as v1 now does, as a basis for the future (so that anyone on the latest version is getting the latest behavior). I can readily imagine that introducing two new major versions for this one issue is not an attractive option. Or perhaps the more appropriate thing would be to use a
retractdirective; but I still anticipate reluctance.Another option I could see, and perhaps the most viable one to propose here, is the addition of some kind of stronger first-class support for stub behavior within gomock. I could see this being an alternative to
NewController(NewStubController?), an adapter to wrap around a*testing.T, or some other kind of thing less boilerplate-heavy than sprinkling.AnyTimes()all over the place. There are probably a number of workable approaches; I'd be glad if we could discuss concrete options here in this thread.Failing either of those, we'll have to default to sorting it out ourselves -- either by accepting the push to add all that boilerplate and abandon our current heading, or by coming up with some kind of adapter pattern ourselves. Or forking, I suppose. I really think that our approach to testing is reasonable enough to merit a form of support unburdened by pervasive boilerplate, though.