Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Once and None for calls. #236

Open
nstandif opened this issue Feb 14, 2025 · 1 comment
Open

Add Once and None for calls. #236

nstandif opened this issue Feb 14, 2025 · 1 comment

Comments

@nstandif
Copy link

Summary

Provide convenience methods for calls happening only once or none. This is frequent pattern in assertions and would make testing more readable.

Details

Presently, gomock includes func (*Call) Times(int) to handle exactly once or none. There are several reasons why this is an antipattern.

First, it enforces author intention. Changing Times(0) to Times(1) requires only changing the parameter. This a very similar argument to == vs =, which causes issues to this day. Looking at something like Once or None is very helpful for reviewers of code, since they can see clearly the test author intended it to do.

Second, it is easy to get lost in the context when scanning over a complicated test.

For example:

// initialization logic
// ...
foo.EXPECT().Run(gomock.Any()).Return(context.Canceled).Times(1)
baz.EXPECT().Run(gomock.Any()).Return(context.Canceled).Times(2)
fizzBuzz.EXPECT().Run(gomock.Any()).Return(context.Canceled).Times(0)

Here it is important to know that baz is called exactly twice, but the repetitive calls on foo and fizzBuzz make this less emphasized. When using Once() or None(), the intention of the author is very clear.

Third, it makes the line long:
Times(n) is eight characters. Having fewer characters is helpful when working with inlining parameters. Once() And None() are six characters.

Proposed solution

foo.EXPECT().Run(gomock.Any()).Return(context.Canceled).Times(1)
baz.EXPECT().Run(gomock.Any()).Return(context.Canceled).Times(2)
fizzBuzz.EXPECT().Run(gomock.Any()).Return(context.Canceled).Times(0)

becomes

foo.EXPECT().Run(gomock.Any()).Return(context.Canceled).Once()
baz.EXPECT().Run(gomock.Any()).Return(context.Canceled).Times(2)
fizzBuzz.EXPECT().Run(gomock.Any()).Return(context.Canceled).None()
@rabbbit
Copy link

rabbbit commented Feb 16, 2025

Isn't this already in place?

  • None - Calling a mock without a pre-existing expectation results in an error?
  • Once - That's the default option if you don't pass in Time(1).

If anything, I would argue that Times(0) seems to be an anti-pattern here (?).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants