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

C# testing improvements #5876

Merged
merged 6 commits into from
Mar 13, 2019

Conversation

jtattermusch
Copy link
Contributor

@jtattermusch jtattermusch commented Mar 12, 2019

@jtattermusch
Copy link
Contributor Author

CC @jskeet

@jtattermusch
Copy link
Contributor Author

I am getting a failure on netcoreapp2.1, which we didn't catch in #5838. I will try to fix in this PR.

Failed   NaNValuesComparedBitwise
Error Message:
   Expected: not equal to -33554432
  But was:  -33554432

Stack Trace:
   at Google.Protobuf.EqualityTester.AssertInequality[T](T first, T second) in /usr/local/google/home/jtattermusch/github/protobuf/csharp/src/Google.Protobuf.Test/EqualityTester.cs:line 60
   at Google.Protobuf.Collections.RepeatedFieldTest.NaNValuesComparedBitwise() in /usr/local/google/home/jtattermusch/github/protobuf/csharp/src/Google.Protobuf.Test/Collections/RepeatedFieldTest.cs:line 753

@jtattermusch
Copy link
Contributor Author

Turns out that SampleNaNs.SignallingFlipped has the same hashcode as SampleNaNs.PayloadFlipped under netcore2.1, which is why the test is failing. :-)

@jskeet
Copy link
Contributor

jskeet commented Mar 12, 2019

Ah, that'll be a fix to the issue I raised a long time ago :) Previously a.Equals(b) could return true, without a.GetHashCode() being equal to b.GetHashCode().

What's relying on the hash codes being the same to determine equality? Is that a production code issue, or our test infrastructure?

@jtattermusch
Copy link
Contributor Author

It's just our testing code

Assert.AreNotEqual(first.GetHashCode(), second.GetHashCode());
(it assumes that normally hash codes would be very likely to be different for non-equal objects). It looks like all the NaNs have the same hashcode though.

@jskeet
Copy link
Contributor

jskeet commented Mar 12, 2019

Right. So we could just remove that part of the code. It's annoying, as in most cases that's a useful sanity check. Perhaps we should have an optional parameter for "check hash codes as well" that defaults to true, and we could explicitly pass in false for these tests, with a comment?

@jtattermusch
Copy link
Contributor Author

Right. So we could just remove that part of the code. It's annoying, as in most cases that's a useful sanity check. Perhaps we should have an optional parameter for "check hash codes as well" that defaults to true, and we could explicitly pass in false for these tests, with a comment?

Sg, I made the check for hashcodes being not equal optional.

@jtattermusch
Copy link
Contributor Author

This is now ready to review.

@jtattermusch jtattermusch merged commit db1d4dc into protocolbuffers:master Mar 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants