Skip to content

Comments

Diff only after checking repeatability#1694

Closed
rokatyy wants to merge 1 commit intostretchr:masterfrom
rokatyy:find-expected-call
Closed

Diff only after checking repeatability#1694
rokatyy wants to merge 1 commit intostretchr:masterfrom
rokatyy:find-expected-call

Conversation

@rokatyy
Copy link

@rokatyy rokatyy commented Jan 7, 2025

Summary

Optimisation of findExpectedCall

Motivation

That's quite a long story about how I got here. We are migrating Nuclio from Go version 1.21 to 1.23, and apparently, something has changed there in between. This test stopped working, failing on a mock call. So, I decided to dig into how testify works and noticed that if you have more than one call of the same function in ExpectedCalls, the Diff function will still run against it, even if the function has already been called once and its Repeatability is -1. And it would be fine if only we didn't have suite.Require().Equal as part of mock.MatchedBy function, which failed on the 1st iteration here without even reaching the required iteration in ExpectedCalls.

UPD:I noticed that the tests fail in cases where this function returns a call without considering Repeatability. While this behavior is tied to the implementation details, it can take quite a bit of time to figure out the root cause of these failures, which isn't ideal from a UX perspective. This PR is more of a suggestion rather than a change request, so feel free to close it if you believe it’s unnecessary.

@brackendawson
Copy link
Collaborator

It's not working because we've lost the case where there are matching calls but all of them have run out of times they should be called, findExpectedCall returns -1 and the last matcing one.

That's not really the job of "findExpectedCall", this part of the Mock class needs careful re-factoring and could include the cleanup intended by this PR. findClosestCall and they way Mock.Diff is called could also use work.

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

Successfully merging this pull request may close these issues.

2 participants