Skip to content

Nil actual type made AnythingOfType matcher panic #1219

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Nil actual type made AnythingOfType matcher panic #1219

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4893b39
Nil actual type made AnythingOfType matcher panic
brackendawson Jun 29, 2022
fee3c2d
Test names should not contain double underscore
brackendawson Jun 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions mock/mock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -909,10 +909,15 @@ func (args Arguments) Diff(objects []interface{}) (string, int) {
}
} else if reflect.TypeOf(expected) == reflect.TypeOf((*AnythingOfTypeArgument)(nil)).Elem() {
// type checking
if reflect.TypeOf(actual).Name() != string(expected.(AnythingOfTypeArgument)) && reflect.TypeOf(actual).String() != string(expected.(AnythingOfTypeArgument)) {
if reflect.TypeOf(actual) == nil {
if expected != AnythingOfTypeArgument("<nil>") {
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)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

@brackendawson brackendawson Jun 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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:

  1. Dereference pointer types and prepend a * ourselves.
  2. Join the value returned by PkgPath() and Name() on the dereferenced value with a /, (this might also be considered a breaking change as types would become fully qualified).
  3. 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) AnythingOfTypeArgument

to:

func AnythingOfType(t any) argumentMatcher

and do away with AnythingOfTypeArgument altogether.

// not match
differences++
output = fmt.Sprintf("%s\t%d: FAIL: type %s != type %s - %s\n", output, i, expected, reflect.TypeOf(actual).Name(), actualFmt)
output = fmt.Sprintf("%s\t%d: FAIL: type %s != type %s - %s\n", output, i, expected, reflect.TypeOf(actual).String(), actualFmt)
}
} else if reflect.TypeOf(expected) == reflect.TypeOf((*IsTypeArgument)(nil)) {
t := expected.(*IsTypeArgument).t
Expand Down
44 changes: 44 additions & 0 deletions mock/mock_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1613,6 +1613,50 @@ func Test_Arguments_Diff_WithAnythingOfTypeArgument_Failing(t *testing.T) {

}

func Test_Arguments_Diff_WithAnythingOfTypeArgument_NilType(t *testing.T) {

var args = Arguments([]interface{}{AnythingOfType("<nil>")})
var count int
_, count = args.Diff([]interface{}{nil})

assert.Zero(t, count)

}

func Test_Arguments_Diff_WithAnythingOfTypeArgument_NilType_Failing(t *testing.T) {

var args = Arguments([]interface{}{AnythingOfType("string")})
var count int
var diff string
diff, count = args.Diff([]interface{}{nil})

assert.Equal(t, 1, count)
assert.Contains(t, diff, "type string != type <nil> - (<nil>=<nil>)")

}

func Test_Arguments_Diff_WithAnythingOfTypeArgument_NilValue(t *testing.T) {

var args = Arguments([]interface{}{AnythingOfType("*string")})
var count int
_, count = args.Diff([]interface{}{(*string)(nil)})

assert.Zero(t, count)

}

func Test_Arguments_Diff_WithAnythingOfTypeArgument_NilValue_Failing(t *testing.T) {

var args = Arguments([]interface{}{AnythingOfType("*int")})
var count int
var diff string
diff, count = args.Diff([]interface{}{(*string)(nil)})

assert.Equal(t, 1, count)
assert.Contains(t, diff, "type *int != type *string - (*string=<nil>)")

}

func Test_Arguments_Diff_WithIsTypeArgument(t *testing.T) {
var args = Arguments([]interface{}{"string", IsType(0), true})
var count int
Expand Down