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

Bug in CollectionAssert.AreEqual for ValueTuples. #2518

Closed
jam40jeff opened this issue Oct 30, 2017 · 14 comments
Closed

Bug in CollectionAssert.AreEqual for ValueTuples. #2518

jam40jeff opened this issue Oct 30, 2017 · 14 comments

Comments

@jam40jeff
Copy link
Contributor

Within a test, the following code works:

(int, int) t1 = (1, 2);
(int, int) t2 = (1, 2);
Assert.AreEqual(t1, t2);

However, the following code crashes when running the test:

(int, int)[] l1 = { (1, 2), (3, 4) };
(int, int)[] l2 = { (1, 2), (3, 4) };
CollectionAssert.AreEqual(l1, l2);

Here is the exception message and stack trace:

System.NullReferenceException : Object reference not set to an instance of an object.
   at NUnit.Framework.Constraints.Comparers.ValueTupleComparer.Equal(Object x, Object y, Tolerance& tolerance)
   at NUnit.Framework.Constraints.NUnitEqualityComparer.AreEqual(Object x, Object y, Tolerance& tolerance)
   at NUnit.Framework.Constraints.EqualConstraint.ApplyTo[TActual](TActual actual)
   at NUnit.Framework.Assert.That[TActual](TActual actual, IResolveConstraint expression, String message, Object[] args)
   at NUnit.Framework.CollectionAssert.AreEqual(IEnumerable expected, IEnumerable actual)
@ChrisMaddock
Copy link
Member

Thanks for the simple repro @jam40jeff. A PR would be welcome, if you'd be willing.

@jam40jeff
Copy link
Contributor Author

jam40jeff commented Oct 30, 2017 via email

@ChrisMaddock
Copy link
Member

Great! No problem. 😄

@CharliePoole
Copy link
Member

I think this may be related to #2271. @jam40jeff Try doing it using Assert rather than CollectionAssert. If either of the two following work, my guess is that #2271 will also fix this.

Assert.AreEqual(l1, l2);
Assert.That(l2, Is.EqualTo(l1));

A side problem, which this issue should deal with, is that CollectionAssert should really not accept non-collections without giving an error.

@jam40jeff
Copy link
Contributor Author

jam40jeff commented Oct 30, 2017

@CharliePoole What do you mean about CollectionAssert.Assert not accepting "non-collections"? The input parameters are already typed as IEnumerable.

I tried using lists (List<(int, int)>) instead of arrays and it did work. I also tried custom enumerables and they worked as well. So the problem seems to only affects arrays.

Assert.AreEqual does in fact work in all 3 cases (list, custom enumerable, and array). If that should be used instead, what is the purpose of CollectionAssert.AreEqual?

@mikkelbu
Copy link
Member

A side problem, which this issue should deal with, is that CollectionAssert should really not accept non-collections without giving an error.

But (int, int)[] is a collection. I've looked a bit into the code, and I think that the problem is that the logic in TupleComparerBase for detecting whether the current comparator should handle the given type is wrong.

In GetTypeNameWithoutGenerics we extract the type name until the first '`' to determine the underlying type. This works well for System.ValueTuple`2[System.Int32,System.Int32] and System.ValueTuple`3[System.Int32,System.Int32,System.Int32], but unfortunately the type for the array is System.ValueTuple`2[System.Int32,System.Int32][] (notice the square brackets in the end) which mean that we think that the array is actually a tuple consisting of two element, and hence the code fails, when we try to access the first element via Item1.

So we need to make the logic for determining whether the type is a ValueTuple more strict.

@CharliePoole
Copy link
Member

@jam40jeff AH! Never mind, I misread your example. Of course it's an array of ValueTuples, not just a ValueTuple.

@mikkelbu Looks like you're on the right track. I haven't looked at the code, but I would expect that ValueTupleComparer should be called for each array element, not for the entire array. How come it's dealing with the array type rather than the member type?

@jam40jeff
Copy link
Contributor Author

jam40jeff commented Oct 31, 2017

@mikkelbu It looks like the list of comparers (_comparers) in NUnitEqualityComparer should fall through to the _enumerablesComparer, but is being short circuited by the ValueTupleComparer due to the issue you mentioned with extracting the type as a string and ignoring the fact that it's an array.

Interestingly, the problem also happens with Tuples in the latest code, but not in the latest release.

@mikkelbu
Copy link
Member

@jam40jeff Prior there was no special handling of tuples. This was introduced (by me :( ) in #893. I'll take a look at the PR.

@jam40jeff
Copy link
Contributor Author

jam40jeff commented Oct 31, 2017 via email

@timotei
Copy link

timotei commented Feb 25, 2018

@rprouse Is this issue slated to be released in 3.10? (https://github.com/nunit/nunit/milestone/34)

@mikkelbu mikkelbu added this to the 3.10 milestone Feb 25, 2018
@rprouse
Copy link
Member

rprouse commented Feb 25, 2018

@timotei yes, this will be released with 3.10. I was planning on doing the release this week, but I need to check with the @nunit/framework-team to ensure that there are no high priority issues blocking the release and I need to review the backlog.

@mikkelbu
Copy link
Member

@timotei it is. I've just added the milestone.
(@nunit/framework-and-engine-team actually I'm a bit unsure about the process for this. Usually I add the labels when merging, but it looks as though we often don't do this and then just add the labels later, perhaps closer to the milestone).

@rprouse
Copy link
Member

rprouse commented Feb 25, 2018

@mikkelbu feel free to add the milestone when merging pull requests. It makes it easier for me come release. I go through all closed issues before release and make sure that everything has the correct milestone assigned, so it is okay if you forget to add the milestone on merge. When a release goes out, all closed issues must have a milestone assigned. That is why I have the Closed without Action milestone. It allows me to only look at recently closed issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants