Nil actual type made AnythingOfType matcher panic #1219
Conversation
Handle nil types up front before calling any methods on the reflect.Type
mock/mock_test.go
Outdated
|
|
||
| } | ||
|
|
||
| func Test_Arguments_Diff__WithAnythingOfTypeArgument_NilType(t *testing.T) { |
There was a problem hiding this comment.
You have an extra underscore here, please look at test names above to maintain consistency :)
mock/mock_test.go
Outdated
|
|
||
| } | ||
|
|
||
| func Test_Arguments_Diff__WithAnythingOfTypeArgument_NilType_Failing(t *testing.T) { |
There was a problem hiding this comment.
See comment on line 1616
| differences++ | ||
| output = fmt.Sprintf("%s\t%d: FAIL: type %s != type <nil> - %s\n", output, i, expected, actualFmt) | ||
| } | ||
| } else if reflect.TypeOf(actual).Name() != string(expected.(AnythingOfTypeArgument)) && reflect.TypeOf(actual).String() != string(expected.(AnythingOfTypeArgument)) { |
There was a problem hiding this comment.
According to the String() function documentation:
// String returns a string representation of the type.
// The string representation may use shortened package names
// (e.g., base64 instead of "encoding/base64") and is not
// guaranteed to be unique among types. To test for type identity,
// compare the Types directly.
we should not be using it for comparison.
Do you mind updating and fixing the logic to reflect this?
There was a problem hiding this comment.
Definitely agree we should not be using String() for that comparison. The original author seems to have used both Name() and String() to work around the non-defined types thing on Name().
I'm struggling to find an alternative though, the ideal would be to follow the advice in the docs and compare the reflect.Type values directly, but this would be a breaking change because right now people write expectations like mock.AnythingOfType("*string") and we'd need them to pass in at least a literal instead, which might not even be possible for foreign unexported types.
reflect doesn't offer an easy way of doing it (some experimentation here). I think it could maybe be done but we'd need to:
- Dereference pointer types and prepend a
*ourselves. - Join the value returned by
PkgPath()andName()on the dereferenced value with a/, (this might also be considered a breaking change as types would become fully qualified). - Figure out if there are any other "non-defined" types and work out how to get their name as a string.
Do you think it's worth trying in this PR? I'm leaning towards open an issue to fix it in testify v2 by changing:
func AnythingOfType(t string) AnythingOfTypeArgumentto:
func AnythingOfType(t any) argumentMatcherand do away with AnythingOfTypeArgument altogether.
|
I ran into this issue as well. Is there any activity on this PR fixing this issue? |
I believe the state is that fixing this well would be a breaking change, so not until Testify 2.0, and I think we should do that. But the comments above object to something which is not a regression, so I think this change can be merged into 1.x. |
dolmen
added
pkg-mock
|
#1799 is a better approach. |
4 participants
Summary
Passing an actual value of type nil via a Mock into the argument matcher for AnythingOfType in a Mock causes a panic.
Changes
Name()toString()because Name is emptystring for pointer and some other types.Motivation
Someone may expect their mock to be called with a nil interface, we should not panic in this case.
Related issues
Closes #1209