deprecation of `Controller.Finish` breaks certain test frameworks
marten-seemann opened this issue · 1 comments
Actual behavior
When using the Ginkgo test framework, there's only a single TestXXX function. This function call all other test functions written in the Ginkgo framework. When using gomock, users will need to manually call the gomock.Controller.Finish method after each test.
Expected behavior
gomock.Controller.Finish needs to be available, and needs to remain so in the future. #50 deprecated this function, which makes staticcheck emit errors on my test code. That's ok (I can add nolint directives), but I'm worried that the deprecation is a first step to an eventual remove of Finish, which would break my entire test setup in quic-go. (For the record, I'm not happy with Ginkgo, but that's what we're stuck with for the moment, see quic-go/quic-go#3652).
Given that there are valid use cases of Finish, the function should probably not deprecated at all. Instead, documentation could be added explaining that usually (i.e. when not using any test framework) it's not necessary to call that function.
Additional Information
- gomock mode (reflect or source): both
- gomock version or git ref: 837f20a
- golang version: Go 1.21.1
Triage Notes for the Maintainers
Hey @marten-seemann , thanks for raising this issue.
I believe we have a disagreement on what marking an API as Deprecated entails:
From https://github.com/golang/go/wiki/Deprecated:
In contrast to some other systems, an API feature being deprecated does not mean it is going to be removed in the future.
We merged #50 because we believe that calling ctrl.Finish being no-op means we should discourage users from calling that all the time. We get that it creates inconvenience with linters, etc., but it is a step towards the right direction.
I want to reiterate that marking this as deprecated does not mean that we will be eventually removing it. That is a breaking change, and we don't want to make any breaking changes to any of the public APIs of gomock.