fix: compare functional option names for indirect calls

arjun-1 commented 4 months ago

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

arjun-1 commented 4 months ago

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

arjun-1 commented 3 months ago

Hey @dolmen I believe you were involved in reviewing related change https://github.com/stretchr/testify/pull/1381. Anything specific required for this review?

pmokeev commented 2 months ago

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

brackendawson commented 1 month ago

Considering the diff some more, the function name could make a mistake more obvious to the user. But I'd want some clearer separation between the function name and the bahaviour because it's quite hard to grok, how about:

Diff: 0: FAIL:  WithB(*MyMock) -> &MyMock{A:0 B:1} != WithA(*MyMock) -> &MyMock{A:1 B:0}

brackendawson commented 1 month ago

The docstring for FunctionalOptions should also be changed to state that only the behaviour of the functional option argument is compared, and that functional options with identical outcomes compare as equal. Currently it says:

FunctionalOptions returns an [FunctionalOptionsArgument] object containing
the expected functional-options to check for.

For example:

  Assert(t, FunctionalOptions(foo.Opt1("strValue"), foo.Opt2(613)))

arjun-1 commented 3 weeks ago

@brackendawson I agree with you that the regression you mention is not ideal. Originally I removed the function name comparison (only focussing on behavior) to make the tests (fixing the bug of this issue) pass for older Go versions older than 1.20. The function name retrieved via FuncForPC yields false unexpected calls for these versions. For example

github.com/stretchr/testify/mock.Test_Mock_AssertExpectationsFunctionalOptionsType_Indirectly.func1 github.com/stretchr/testify/mock.TheExampleMethodFunctionalOptionsIndirect.func1

I investigated the solution of using Pointer() directly on the reflected value. The pointer value is again falsely different, as I believe it points to the inner closed anonymous function (func1).

I explored a different comparison using FileLine. It's imperfect as two different function values on the same line would not be detected as equal. But it is the only solution that prevents the regression you mention (see Test_Mock_AssertExpectationsFunctionalOptionsType_Diff_Func) and fixes the original issue (Test_Mock_AssertExpectationsFunctionalOptionsType_Indirectly) for all Go versions.

The result is now a comparison based on function file line, and if that is the same a comparison based on function behavior. Let me know if the docstring for FunctionalOptions should still be updated.

stretchr / testify