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

Panic: Equal on type aliases causes panic #644

Closed
bfosberry opened this issue Aug 3, 2018 · 6 comments
Closed

Panic: Equal on type aliases causes panic #644

bfosberry opened this issue Aug 3, 2018 · 6 comments

Comments

@bfosberry
Copy link

bfosberry commented Aug 3, 2018

Current behaviour:

With go 1.9.3 and testify 1.2.2 or latest master (f35b8ab) if I require/assert.Equal two aliased types, it causes testify to panic:

goroutine 5 [running]:
testing.tRunner.func1(0xc42010a0f0)
	/Users/brendan.fosberry/.gvm/gos/go1.9.3/src/testing/testing.go:711 +0x2d2
panic(0x1289000, 0xc420066780)
	/Users/brendan.fosberry/.gvm/gos/go1.9.3/src/runtime/panic.go:491 +0x283
github.com/stretchr/testify/assert.diff(0x126fae0, 0xc420010d10, 0x126fae0, 0x130a0b0, 0x0, 0x0)
	/Users/brendan.fosberry/go/src/github.com/stretchr/testify/assert/assertions.go:1352 +0x494
github.com/stretchr/testify/assert.Equal(0x141c480, 0xc42010a0f0, 0x126fae0, 0xc420010d10, 0x126fae0, 0x130a0b0, 0x0, 0x0, 0x0, 0xc420039f01)
	/Users/brendan.fosberry/go/src/github.com/stretchr/testify/assert/assertions.go:338 +0x271
_/Users/brendan.fosberry/workspace/derp.TestMyTypeRequire(0xc42010a0f0)
	/Users/brendan.fosberry/workspace/derp/derp_test.go:19 +0x11f
testing.tRunner(0xc42010a0f0, 0x12e9c88)
	/Users/brendan.fosberry/.gvm/gos/go1.9.3/src/testing/testing.go:746 +0xd0
created by testing.(*T).Run
	/Users/brendan.fosberry/.gvm/gos/go1.9.3/src/testing/testing.go:789 +0x2de
FAIL	_/Users/brendan.fosberry/workspace/derp	0.038s

Expected behaviour: testify should determine if the two types are equal, or fail gracefully.

Here is a test case to replicate the issue

package main

import (
        "testing"

        "github.com/stretchr/testify/assert"
)

type MyType string

const (
        MyTypeA MyType = "a"
        MyTypeB MyType = "b"
)

func TestMyTypeRequire(t *testing.T) {
        mt := MyTypeA
        assert.Equal(t, mt, MyTypeA)
        assert.Equal(t, mt, MyTypeB)
}

I don't know much about reflect, but I think it comes down to this:

	var e, a string
	if ek != reflect.String {
		e = spewConfig.Sdump(expected)
		a = spewConfig.Sdump(actual)
	} else {
		e = expected.(string)
		a = actual.(string)
	}

ek == String comes back as true for string type aliases since they are of kind string, however a type assertion causes a panic since they are not of type string.

My workaround for now is not to use Equal with type aliases, however this is something that should be handled internally.

@ryanmcnamara
Copy link

👍 just hit this took a while to debug

@HaraldNordgren
Copy link
Contributor

HaraldNordgren commented Sep 11, 2018

Created a PR here which should solve the problem 😊

#661

@bfosberry
Copy link
Author

Nice, thanks Harald!

@georgelesica-wf
Copy link

@HaraldNordgren could you comment on the difference between this solution and the one referenced above (#651)?

@HaraldNordgren
Copy link
Contributor

HaraldNordgren commented Sep 18, 2018

@georgelesica-wf I'm not sure, both solutions might be equivalent. But the other solution is currently untested which is a problem for a testing library.

@georgelesica-wf
Copy link

@HaraldNordgren agreed, thanks for looking at it. I just want to make sure that we resolve all the issues people have seen with this bit of code.

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

No branches or pull requests

4 participants