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 call stack to unexpected mock call #145

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

feldentm-SAP
Copy link

Make identification of unexpected calls easier.

Note, this is a copy of #110 which is miraculously permanently closed.

Make identification of unexpected calls easier
@JacobOaks
Copy link
Contributor

Hey @feldentm-SAP, sorry for the confusion on the other PR. Not sure why github insists on it remaining closed.

I think this could be a helpful addition. I'm not too worried about performance implications of collecting stack traces since this is for testing code. One thing we may want to do is use https://pkg.go.dev/runtime#Callers instead and skip the first couple of frames since these are irrelevant and point to internal gomock code.

@tchung1118 what do you think?

@tchung1118
Copy link
Contributor

I agree with @JacobOaks that skipping internal callers could be better.

@feldentm-SAP
Copy link
Author

@JacobOaks My understanding is that the code is executed for failed tests, only. Hence, performance considerations are almost irrelevant. I also agree that skipping callers would improve the output, but the benefit over the current proposal does not justify spending the required time as I no longer understand the change. Hence, I'd propose that this is either merged or one of you takes an educated guess on how many callers need to be skipped and if this can be determined statically. Getting this wrong by one reduces the benefit of the change to almost zero.

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.

3 participants