deprecation of Controller.Finish breaks certain test frameworks

[marten-seemann]

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

[sywhang]

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.

[marten-seemann]

Hi @sywhang! Thank you. It's reassuring to know that there's no plan to remove ctrl.Finish. I can work around the linter warning, as long as I can rely on this function continuing to exist in the future.

I disagree on the use of deprecation here though. Even with no intent to remove a feature, the feature should only be deprecated if there's no use case for it at all. This is clearly not the case here: As I've described above, being able to call ctrl.Finish is absolutely critical when using 3rd-party testing frameworks. It wouldn't be possible to use gomock without ctrl.Finish otherwise.

It's common that APIs provide functions that are only needed for a small minority of use cases. That's ok, and a sign of a versatile API that can be used in many different ways. The fact that some functions are only useful for a few but not other uses doesn't mean that these functions should be deprecated. Instead, they should just come with a comment that explains why they're not needed in most cases and what the more common usage of that API is.

[kannapoix]

It's very helpful if the document suggests that Finish() is no longer needed, depending on the Go version.
The golang/mock's document does this, but not in uber's

[zchenyu]

+1 Can we get some confirmation that calling this function is indeed a no-op? Happy to prepare a PR updating the comment if this is the case.