Skip to content
This repository has been archived by the owner on Jun 27, 2023. It is now read-only.

gomock/controller: use skip additional frame #443

Merged
merged 2 commits into from
Jun 12, 2020
Merged

Conversation

stephen
Copy link
Contributor

@stephen stephen commented Jun 5, 2020

Description
In 8321731, the callerInfo call
gets nested within an additional function call, but the frame skip
isn't updated. When used with mockgen, this causes the location frame
to unhelpfully point to the generated mock instead of the callsite in
the user's test.

Before:

... at .../mocks/mock_ecriface/mock_ECRAPI.go:612 because: there are no expected calls of...

After:

... at .../example_test.go:29 because: there are no expected calls of...

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes tests

Reviewer Notes

  • The code flow looks good.
  • Tests added.

Release Notes

Describe any changes here so maintainer can include it in the release notes, or delete this block.

- Fixes blame location for unexpected calls to mock

In 8321731, the callerInfo call
gets nested within an additional function call, but the frame skip
isn't updated. When used with mockgen, this causes the location frame
to unhelpfully point to the generated mock instead of the callsite in
the user's test.

Before:
... at .../mocks/mock_ecriface/mock_ECRAPI.go:612 because: there are no expected calls of...

After:
... at .../example_test.go:29 because: there are no expected calls of...
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Copy link
Member

@codyoss codyoss left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. I think we should just document the magic number a little.

gomock/controller.go Show resolved Hide resolved
Add an explanation for future readers, explaining callerInfo and when it
should be updated.
@stephen
Copy link
Contributor Author

stephen commented Jun 12, 2020

@codyoss thanks for looking. i added a commit documenting both usages of callerInfo and explaining the skip number.

Copy link
Member

@codyoss codyoss left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

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

Successfully merging this pull request may close these issues.

3 participants