Skip to content

deprecation of Controller.Finish breaks certain test frameworks #81

Closed

Description

@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

Activity

  1. sywhang commented on Sep 12, 2023

    @sywhang
    Contributor

    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.

  2. locked and limited conversation to collaborators on Sep 12, 2023
  3. converted this issue into a discussion #84 on Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions