Skip to content

Exclude ConvertibleTo from equality test#128

Merged
tylerstillwater merged 1 commit intostretchr:masterfrom
icecrime:equality_without_conversions
Feb 10, 2015
Merged

Exclude ConvertibleTo from equality test#128
tylerstillwater merged 1 commit intostretchr:masterfrom
icecrime:equality_without_conversions

Conversation

@icecrime
Copy link
Copy Markdown
Contributor

@icecrime icecrime commented Feb 7, 2015

ObjectsAreEqual using ConvertibleTo causes the ObjectsAreEqual function to be asymmetrical and producing incorrect assertions.

To give a few invalid examples that this patch fixes:

  • ObjectsAreEqual(2.2, 2) == true but ObjectsAreEqual(2, 2.2) == false
  • ObjectsAreEqual('x', "x") == true but ObjectsAreEqual("x", 'x') == false

Exclude conversions from equality tests
9bab92e 
`ObjectsAreEqual` using `ConvertibleTo` causes the `ObjectsAreEqual`
function to be asymmetrical and producing incorrect assertions.

Signed-off-by: Arnaud Porterie <[email protected]>
@icecrime icecrime mentioned this pull request Feb 7, 2015
Closed
@vieux
Copy link
Copy Markdown

vieux commented Feb 7, 2015

I don't know about ObjectsAreEqual but the Equal and NotEqual functions (the one you should use) are perfectly fine to me:

        assert.Equal(t, 2, 2)
        assert.Equal(t, 2.2, 2.2)
        assert.NotEqual(t, 2.2, 2)
        assert.NotEqual(t, 2, 2.2)

        assert.Equal(t, 'x', 'x')
        assert.Equal(t, "x", "x")
        assert.NotEqual(t, 'x', "x")
        assert.NotEqual(t, "x", 'x')

This is completely broken
This works as expected

@icecrime
Copy link
Copy Markdown
Contributor Author

icecrime commented Feb 7, 2015

This was introduced in #54 as far as I can tell.

@vieux
Copy link
Copy Markdown

vieux commented Feb 7, 2015

@icecrime my bad, I wasn't using master, but a vendored version.

Master it completely broken. Our vendored version is d6577e0 and it works as expected

@vieux
Copy link
Copy Markdown

vieux commented Feb 7, 2015

@tylerb can you take a look, this is pretty bad

@vieux
Copy link
Copy Markdown

vieux commented Feb 7, 2015

@icecrime even better: 49c5c6c

@tiborvass
Copy link
Copy Markdown

tiborvass commented Feb 10, 2015

Ping @matryer @tylerb

tylerstillwater pushed a commit that referenced this pull request Feb 10, 2015
@tylerb
Merge pull request #128 from icecrime/equality_without_conversions
43d0eed 
Exclude `ConvertibleTo` from equality test
@tylerstillwater tylerstillwater merged commit 43d0eed into stretchr:master Feb 10, 2015
This was referenced Feb 10, 2015
Closed
Closed
@mynameiswhm mynameiswhm mentioned this pull request May 19, 2015
Merged
@unclejack unclejack mentioned this pull request Apr 14, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@icecrime @vieux @tiborvass @tylerstillwater