nunit: CollectionAssert.AllItemsAreUnique() very slow
Consider following simple test case:
// using NUnit.Framework;
[TestCase(10000)]
public void TestMethod1(int range)
{
CollectionAssert.AllItemsAreUnique(Enumerable.Range(0, range).ToArray());
}
it takes about 14 seconds to complete.
However, applying MS Test Framework to same test case, it takes about 12 ms only to finish.
// using Microsoft.VisualStudio.TestTools.UnitTesting;
[TestMethod]
public void TestMethod1()
{
CollectionAssert.AllItemsAreUnique(Enumerable.Range(0, 10000).ToArray());
}
The gap between two should not be that huge.
About this issue
- Original URL
- State: closed
- Created 7 years ago
- Comments: 20 (18 by maintainers)
Commits related to this issue
- Initial try at a fix for #2494 — committed to nunit/nunit by CharliePoole 7 years ago
- Initial fix for #2494 — committed to nunit/nunit by CharliePoole 7 years ago
- Initial fix of #2494 — committed to nunit/nunit by CharliePoole 7 years ago
- Merge pull request #2501 from nunit/issue-2494 Initial fix for #2494 — committed to nunit/nunit by oznetmaster 7 years ago
@ChrisMaddock Thanks for looking into the issue. We have a test case which requires to check uniqueness of 10k strings and it cannot finish within one minute. The performance improvement shall help us a lot.
The top-down fix is ready to merge in PR #2501. I’m taking my name off this issue and leaving @ChrisMaddock to decide if further work at the back end is necessary.
@CharliePoole - Looks that way to me: https://github.com/Microsoft/testfx/blob/e3d6e822df12fed112a9c68311a11fac8a4cf914/src/TestFramework/MSTest.Core/Assertions/CollectionAssert.cs#L338
I’d personally considered changing functionality here too much of a breaking change. I agree we should work to improve this, however, if nothing can be done, I would consider leaving it unchanged preferable to changing our measure of ‘unique’. Of course, there’s more voices than just mine in this decision. 😄
Option 4 is a good shout. Option 5 is something else I considered, but sounds pretty messy!
Another option, which I have no idea to the feasibility of, is introduce a typed NUnitEqualityComparer. The current version checks all tests of equality for all types - meaning asserting that something is not equal as we are doing here is pretty expensive. The method current takes two objects - I wonder if we could introduce specific overloads to handle different types. This is similar to option (4) - but tackling the issue at a lower level.
@ChrisMaddock I added my name as a co-assignee. I’m working at the top end and I’ll leavE
NUnitEqualityComparerfor you. PR shortly.A simple thing to try would be (in
Matches)That way, if an IList were passed in, we would not create a new one.UPDATE: We would then make the comparisons directly in the existing list.
Following that (if tests showed it necessary) I’d use a generic helper to eliminate boxing. I’m busy on parallelization stuff or I’d give it a try myself.
NUnit uses a different way to test for uniqueness, than MSTest.
MSTest uses simple object equality. NUnit equality tests go through NUnitEqualityComparer - which has extra complexity.
~Looking at that class however, there may be low hanging fruit. Before comparing for object equivalence, we create an empty List of FailurePoint objects, which doesn’t seem to be used. We then do two null checks prior to object.ReferenceEquals(), which I think are unnecessary.~ This is incorrect - Resharper’s ‘find usages’ failed me!
I wonder if initialising that list lazily is possible. I’ll have a dig at some point.