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

fix: compare functional option names for indirect calls #1626

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

Conversation

arjun-1
Copy link

@arjun-1 arjun-1 commented Jul 23, 2024

Summary

Closes Functional Options testing broken for indirect calls #1380. The fix is consists of not comparing the the function names, as it is already asserted when comparing expectedValues and actualValues

Changes

  • Adds test Test_Mock_AssertExpectationsFunctionalOptionsTypeIndirectly which would fail without the fix
  • Adds test Test_Mock_AssertExpectationsFunctionalOptionsType_Diff to examine behavior of slightly different functional options in expected vs actual.
  • Changes the funcName implementation to only compare the relevant last part, if applicable. This aids in reading the diff that is produced when the function names don't. match (for go1.20 or higher)

Motivation

The choice was made to omit the comparison of function names completely, instead of comparing only the relevant last part. The reason is that go1.19 and lower don't give enough any meaningful comparable part in the function name. Nevertheless the most relevant part of the function name is included in the diff result. Without it, the diff yielded in Test_Mock_AssertExpectationsFunctionalOptionsType_Diff would have resulted in (go1.20):

mock.Test_Mock_AssertExpectationsFunctionalOptionsType_Diff.OpNum &{num:0 str:1} != mock.Test_Mock_AssertExpectationsFunctionalOptionsType_Diff.OpNum &{num:1 str:}

The current implementation instead yields a diff:

OpStr &{num:0 str:1} != OpNum &{num:1 str:}

Though there could be added benefit in displaying the full location of the function, I can revert that part if desired.

Related issues

Closes #1380

Closes Functional Options testing broken for indirect calls stretchr#1380
@arjun-1
Copy link
Author

arjun-1 commented Jul 23, 2024

Looks like the approach to compare a relevant part only fails for older Go versions, I'll rethink that
Done

@arjun-1 arjun-1 marked this pull request as draft July 23, 2024 14:36
…mes (but do include them in the diff output)
@arjun-1 arjun-1 marked this pull request as ready for review July 23, 2024 15:01
@arjun-1
Copy link
Author

arjun-1 commented Aug 5, 2024

Hey @dolmen I believe you were involved in reviewing related change #1381. Anything specific required for this review?

@pmokeev
Copy link

pmokeev commented Aug 30, 2024

@dolmen can you have a look at this PR? I've also encountered the same problem

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.

Functional Options testing broken for indirect calls
2 participants