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

mock: make (mock.Call).Times(int) add expectations for each expected call #1468

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ullaakut
Copy link

Summary

See this playground example that highlights the issue.

The current behavior of Times(int) is that if the expectation of X calls being made is not met, the error message that gets written is The code you are testing needs to make 1 more call(s). which is very confusing, because there might be more than 1 call missing.

For example if you expect 50 calls and that only 5 are made, one would expect the message to be The code you are testing needs to make 45 more call(s)..

This pull request addresses that, by making the Times(int) method append one expectation per call, on the mock.

Changes

  • Makes the (mock.Call).Times() method append expectations for each expected call, on the call's parent.
  • Updates unit tests accordingly.

Motivation

The current mock error output is misleading.

Related issues

Closes #1383.

* Makes the (mock.Call).Times() method append expectations for each expected call, on the call's parent.
* Updates unit tests accordingly.
@dolmen dolmen added bug pkg-mock Any issues related to Mock hacktoberfest-accepted Hacktoberfest labels Oct 10, 2023
Copy link
Collaborator

@dolmen dolmen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have the knowledge of that code. I would be sure that there is no conflict with the Repeatability property.

I would like the reviews of other people before accepting this change.

@dolmen dolmen changed the title Make (mock.Call).Times(int) add expectations for each expected call mock: make (mock.Call).Times(int) add expectations for each expected call Oct 10, 2023
Copy link
Collaborator

@brackendawson brackendawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do like the idea of adding multiple expectations rather than having to count with mock.Call.Repeatability. Though it would make it even harder to fix .Times(0), which is currently broken too. And it would also break mock.Call.NotBefore.

So we must go with the alternateive and smaller approach, which is to make the error message aware of the Repeatability instance variable.

The approach in this PR is ineffective, it just changes the message to a different fixed number.


// Add expected calls to the parent mock.
for j := 1; j < i; j++ {
c.Parent.ExpectedCalls = append(c.Parent.ExpectedCalls, c)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

c is a *Call (pointer to Call), so you actually casue the same call to be added to the Mock's expectations i times.

This is working the same way it used to because we are decrementing c.Repeatabilty on all the expectations at once, since they are the same expectation. The only difference is that the error now says you need to make i more calls, rather than "1" more calls, regarless of how many more you actually need to make.

@githoober
Copy link

@Ullaakut thanks for the PR. Are you planning on fixing it?

@Ullaakut
Copy link
Author

@Ullaakut thanks for the PR. Are you planning on fixing it?

Oh sorry, forgot about this PR 😁 I would love to but I don't have time at the moment. If anyone else can pick it up, please do, otherwise feel free to close it and I'll reopen with an alternate fix if I find the time for it 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug hacktoberfest-accepted Hacktoberfest pkg-mock Any issues related to Mock
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mock.AssertExpectations error message is not ideal
4 participants