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: print caller info on unexpected method call #1448

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Lochlanna
Copy link

@Lochlanna Lochlanna commented Aug 2, 2023

Summary

Print caller info when an unexpected method call is made on a mock and a "closest call" can be found

Motivation

Both other call failures print this info so this change makes it consistent across the three failures.
The caller info is useful information when using the mock to develop tests.

Related issues

None that I could find

Example test that benefits from this change

https://go.dev/play/p/hZulK4Fc_1_I

@Lochlanna Lochlanna marked this pull request as ready for review August 2, 2023 04:33
@Lochlanna Lochlanna force-pushed the caller-info-on-unexpected-method-call branch from 212be46 to d291745 Compare August 2, 2023 04:35
@Lochlanna Lochlanna force-pushed the caller-info-on-unexpected-method-call branch from d291745 to c3e1218 Compare August 2, 2023 04:35
@Lochlanna
Copy link
Author

@CodiumAI-Agent /review

@CodiumAI-Agent
Copy link

PR Analysis

  • 🎯 Main theme: Adding caller info on unexpected method call
  • 📌 Type of PR: Enhancement
  • 🧪 Relevant tests added: No
  • Focused PR: Yes, the PR has a clear and coherent title and description, and all PR code diff changes are properly derived from the title and description.
  • 🔒 Security concerns: No, the changes made in this PR do not introduce any apparent security concerns.

PR Feedback

  • 💡 General PR suggestions: The PR is well-structured and the changes are clear. However, it would be beneficial to add tests to verify the new functionality. This would ensure that the new feature works as expected and does not introduce any regressions.

How to use

Tag me in a comment '@CodiumAI-Agent' and add one of the following commands:
/review [-i]: Request a review of your Pull Request. For an incremental review, which only considers changes since the last review, include the '-i' option.
/describe: Modify the PR title and description based on the contents of the PR.
/improve: Suggest improvements to the code in the PR.
/ask <QUESTION>: Pose a question about the PR.

To edit any configuration parameter from 'configuration.toml', add --config_path=new_value
For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."

Copy link

@reza-ryte-club reza-ryte-club left a comment

Choose a reason for hiding this comment

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

I found that printing callerInfo would be a neat thing to do. LGTM.

@dolmen dolmen added pkg-mock Any issues related to Mock enhancement labels Aug 8, 2023
@dolmen
Copy link
Collaborator

dolmen commented Aug 8, 2023

Please provide a test running on go.dev/play for which the output would be improved by the proposed change.

Here is a template: https://go.dev/play/p/yobgjBYsyWz

@dolmen dolmen changed the title Print caller info on unexpected method call mock: print caller info on unexpected method call Aug 8, 2023
@Lochlanna
Copy link
Author

Lochlanna commented Aug 9, 2023

Here is an example which shows a test that would benefit from the extra information. In this test it doesn't tell you which of the calls to DoSomething failed. With the fix in this MR the location of the call that wasn't matched is printed out so the developer immediately knows which call is not being called with the correct input/what is missing in the mocks.

https://go.dev/play/p/hZulK4Fc_1_I

@dolmen
Copy link
Collaborator

dolmen commented Oct 16, 2023

Reproducer, simplified: https://go.dev/play/p/LqwLNJpzo9e

The information is in the stacktrace, but having it directly in the message would be helpful.

mock/mock.go Outdated
@@ -491,11 +491,12 @@ func (m *Mock) MethodCalled(methodName string, arguments ...interface{}) Argumen
m.mutex.Unlock()

if closestCall != nil {
m.fail("\n\nmock: Unexpected Method Call\n-----------------------------\n\n%s\n\nThe closest call I have is: \n\n%s\n\n%s\nDiff: %s",
m.fail("\n\nmock: Unexpected Method Call\n-----------------------------\n\n%s\n\nThe closest call I have is: \n\n%s\n\n%s\nDiff: %s\n\nat: %s",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fix formatting to match the style of the other message in the other branch of the if.

Suggested change
m.fail("\n\nmock: Unexpected Method Call\n-----------------------------\n\n%s\n\nThe closest call I have is: \n\n%s\n\n%s\nDiff: %s\n\nat: %s",
m.fail("\n\nmock: Unexpected Method Call\n-----------------------------\n\n%s\n\nThe closest call I have is: \n\n%s\n\n%s\n\tat: %s\nDiff: %s",

Copy link
Author

Choose a reason for hiding this comment

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

I've fixed it up so that it uses \n\t but I feel like the caller info should go last as it does for both of the other scenarios. I'm not too fussed either way though if you're keen for diff to be last

callString(methodName, arguments, true),
callString(methodName, closestCall.Arguments, true),
diffArguments(closestCall.Arguments, arguments),
strings.TrimSpace(mismatch),
assert.CallerInfo(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move the caller info nearer to the function call, above the diff.

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

Successfully merging this pull request may close these issues.

4 participants