Cleanup ImmutableArray tests #14798

jamesqo · 2017-01-02T19:19:28Z

This is still WIP, but since it's a rather large PR and almost finished I thought I'd submit it. There are still a few TODOs floating around, but feel free to review.

Changes:

Theorize most of the tests to make it easier to add new test cases
Segregate tests that assert equality from tests that check exceptions
Standardize method/variable naming to make tests easier to read
Fill in some gaps in the tests
- Test more explicit interface implementations
- Run methods that accept comparers over a variety of comparers
- Run methods that accept collections over a variety of collections (arrays, lists, IList<T>, lazy enumerables, etc.)
General cleanup
- Replace Assert.Equal(immutableArray1, immutableArray2) with Assert.True(immutableArray1 == immutableArray2)
  - This has a poorer stack trace, but it makes it more explicit that we're testing reference equality

Note: I tried to keep many of the existing test cases we have, so that's why you'll see a lot of (sometimes redundant) cases in the MemberData.

/cc @AArnott @ianhays @stephentoub

jamesqo · 2017-01-02T19:23:50Z

src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray_1.cs

+            // If the index is less than 0, the && below will short-circuit, and self.Length will
+            // not be evaluated. Then we will get an ArgumentOutOfRangeException.
+            // Otherwise, we will get a NullReferenceException when trying to evaluate self.Length.
+
            var self = this;
            Requires.Range(index >= 0 && index <= self.Length, nameof(index));


There is a problem here where if self is default and index is negative, an ArgumentOutOfRangeException will be thrown. But if index is positive or length is negative, a NullReferenceException will be thrown. Would it be acceptable to add a self.ThrowNullRefIfNotInitialized() before this line to make the behavior consistent?

Either exception should never be caught and is a sign of an error in the caller. So consistently prioritizing one over the other sounds like (mild) goodness to me.
But unless it's blocking your test development, I suggest you fix this in a separate PR as well.

jamesqo · 2017-01-02T19:27:32Z

src/System.Collections.Immutable/tests/ImmutableArrayTest.cs

        {
-            Assert.Throws<ArgumentNullException>("items", () => ImmutableArray.CreateRange((IEnumerable<int>)null));
+            // Once https://github.com/xunit/assert.xunit/pull/5 comes into corefx, all the StrongBox stuff can be removed.


xUnit tries to format the collection for each test case by calling GetEnumerator and stringifying each of the values. For default immutable arrays, this is problematic because that methods throws an InvalidOperationException. This was fixed in xunit/assert.xunit#5, but it looks like corefx hasn't picked up that change yet. I've worked around this issue temporarily by wrapping each enumerable in a StrongBox<T> so it won't get formatted.

jamesqo · 2017-01-02T19:28:27Z

src/System.Collections.Immutable/tests/ImmutableArrayTest.cs

        }

        [Fact]
-        public void CreateRangeFromImmutableArrayWithSelector()
+        public void CreateEnumerableElementType()


Added extra tests here to ensure Create creates an immutable array of collections for non-arrays.

jamesqo · 2017-01-02T19:31:18Z

src/System.Collections.Immutable/tests/ImmutableArrayTest.cs

-            Assert.Equal(new double[] { }, copy1);
+        [Theory]
+        [MemberData(nameof(CreateRangeWithSelectorData))]
+        public void CreateRangeWithSelector<TResult>(IEnumerable<int> source, Func<int, TResult> selector, TResult dummy)


Another xUnit issue that has been fixed, but not made it into corefx yet: the type of TResult cannot be inferred unless there is a TResult parameter that isn't wrapped. xunit/xunit#965

jamesqo · 2017-01-02T19:34:46Z

src/System.Collections.Immutable/tests/ImmutableArrayTest.cs

+
+        [Theory]
+        [MemberData(nameof(ContainsEqualityComparerData))]
+        public void ContainsEqualityComparer<T>(IEnumerable<T> source, T value, IEqualityComparer<T> comparer)


After cleaning up this test I found out this was actually calling a method in LINQ, so it isn't really relevant to ImmutableArray (aside from testing GetEnumerator, which is covered below). Should I remove this?

jamesqo · 2017-01-02T19:39:39Z

src/System.Collections.Immutable/tests/ImmutableArrayTest.cs

+
+            // TODO: The parameter names in CoreCLR need to be better.
+
+            Assert.Throws<ArgumentNullException>("dest", () => array.CopyTo(null));


The parameter names in Array.Copy actually match that of ImmutableArray.CopyTo (sourceIndex, destinationIndex, destination, etc.) but the names in the exceptions do not match up. I will look into changing the names in CoreCLR in a later PR.

jamesqo · 2017-01-05T02:15:19Z

OK, I have just finished my first pass, so un-marking this as WIP. I still have some comments/skipped tests labeled with TODO on issues I am looking for guidance on.

jamesqo · 2017-01-05T02:22:00Z

src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray_1.cs

@@ -728,6 +728,9 @@ public ImmutableArray<T> Replace(T oldValue, T newValue)
        public ImmutableArray<T> Replace(T oldValue, T newValue, IEqualityComparer<T> equalityComparer)
        {
            var self = this;
+            // TODO: This is calling the IImmutableList<T> overload which is silently boxing the ImmutableArray<T>.
+            // It's also causing inconsistencies where calling Replace on an IsDefault instance will throw an
+            // InvalidOperationException rather than a NullReferenceException.


I added some extra test coverage that called this method on a default ImmutableArray. That caused problems because the IndexOf call here is actually boxing this to an IImmutableList<T> and calling the extension method in ImmutableList.

The fix would be to simply add self.ThrowNullRefIfNotInitialized() before this line, or modify the IndexOf call so it calls the ImmutableArray overload and avoids boxing. This would be a behavioral change from InvalidOperationException -> NullReferenceException for default arrays, but I am assuming throwing IOE in the first place because normally everything throws NRE for default arrays. Is this OK to change?

Avoiding boxing sounds reasonable. But let's make that change in another PR and keep this one about testing.
I would suggest you revert the TODO comment and file an issue to track.

@AArnott OK, I have reverted the comment and opened https://github.com/dotnet/corefx/issues/14961 to be addresed after the test changes go in.

jamesqo · 2017-01-05T02:23:49Z

src/System.Collections.Immutable/tests/ImmutableArrayTest.cs

        {
            Assert.Throws<NullReferenceException>(() => s_emptyDefault.LastIndexOf(5));
            Assert.Throws<NullReferenceException>(() => s_emptyDefault.LastIndexOf(5, 0));
            Assert.Throws<NullReferenceException>(() => s_emptyDefault.LastIndexOf(5, 0, 0));
        }

+        // TODO: Cleanup IndexOf/LastIndexOf tests.


Self-note: Remove. This PR is big enough already and I can do this separately.

jamesqo · 2017-01-05T02:27:29Z

src/System.Collections.Immutable/tests/ImmutableArrayTest.cs


-            Assert.Equal(new[] { 12345 }, s_oneElemen


Same here as with the Contains tests above. These are calling the Linq methods, Concat doesn't exist on ImmutableArray. Can I delete these?

jamesqo · 2017-01-05T02:28:59Z

src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray_1.cs

@@ -764,6 +767,7 @@ public ImmutableArray<T> Remove(T item, IEqualityComparer<T> equalityComparer)
        {
            var self = this;
            self.ThrowNullRefIfNotInitialized();
+            // TODO: This is silently boxing.


I discovered that there was silent boxing here too.

jamesqo · 2017-01-05T02:34:13Z

src/System.Collections.Immutable/tests/ImmutableArrayTest.cs

-            Assert.Equal(1, removed13.Length);
-            Assert.Equal(new[] { 2 }, removed13);
+        [Theory]
+        [InlineData(-1, Skip = "TODO. See comment in ImmutableArray<T>.RemoveRange(int, int).")]


tl;dr RemoveRange on a default array is throwing AOORE when the index is negative, but when other parameters are invalid it's throwing NRE because it evaluates self.Length. Can I fix this?

jamesqo · 2017-01-05T02:36:39Z

src/System.Collections.Immutable/tests/ImmutableArrayTest.cs

+            var array = source.ToImmutableArray();
+            IEnumerable<int> expected = items.Aggregate(
+                seed: source.ToImmutableArray(),
+                func: (a, i) => a.Remove(i, comparer));


Self-note: This is the cleanest way to calculate expected but it seems uncomfortably close to the code I'm actually testing.

jamesqo · 2017-01-05T02:37:55Z

src/System.Collections.Immutable/tests/ImmutableArrayTest.cs


-            Assert.Equal(new[] { 12345 }, s_oneElemen


This was the one where the IImmutableList box was causing an IOE to be thrown for default immutable arrays.

jamesqo · 2017-01-05T02:39:14Z

src/System.Collections.Immutable/tests/ImmutableArrayTest.cs


-            Assert.Equal(new[] { 12345 }, s_oneElemen


I made a review note in SetItem. Requires.Range(index >= 0 && index < self.Length)-- if index is negative the condition short-circuits.

jamesqo · 2017-01-05T02:41:48Z

src/System.Collections.Immutable/tests/ImmutableArrayTest.cs

-            Assert.Equal(array.GetHashCode(EverythingEqual<int>.Default), immArray.GetHashCode(EverythingEqual<int>.Default));
+            return SharedComparers<int>()
+                .OfType<IEqualityComparer>()
+                .Except(new IEqualityComparer[] { null })


Self-nit: I think all of the Except(new[] { null })s may not be necessary. I found out (while writing this PR) what OfType does not include nulls.

jamesqo · 2017-01-05T02:43:10Z

src/System.Collections.Immutable/tests/ImmutableArrayTest.cs

-            IStructuralComparable equalArray = new int[3] { 1, 2, 3 };
-            IStructuralComparable immArray = ImmutableArray.Create((int[])array);
-            IStructuralComparable equalImmArray = ImmutableArray.Create((int[])equalArray);
+            var array = source?.ToImmutableArray() ?? s_emptyDefault;


I got a bit lazy here and used null for source/other to represent a default immutable array, instead of a StrongBox.

jamesqo · 2017-01-05T02:45:32Z

/cc @hughbe This affects your IStructuralEquatable / IStructuralComparable changes from #13436

ianhays · 2017-01-10T23:24:51Z

src/System.Collections.Immutable/tests/ImmutableArrayTest.cs

            Assert.Equal((object)derivedImmutable, baseImmutable, EqualityComparer<object>.Default);

            // Make sure we can reverse that, as a means to verify the underlying array is the same instance.
            ImmutableArray<string> derivedImmutable2 = baseImmutable.As<string>();
            Assert.False(derivedImmutable2.IsDefault);
-            Assert.Equal(derivedImmutable, derivedImmutable2);
+            Assert.True(derivedImmutable == derivedImmutable2);


Why the change from Assert.Equal?

@ianhays When I first saw Assert.Equal I was confused because I thought it was calling the Equal(IEnumerable<T>, IEnumerable<T>) overload which compares the contents of the arrays. Hovering over the method in VS showed that this was not the case, but I still think this is better because the tests will fail loudly if you accidentally write Assert.True(expected == source) (where source is an enumerable and expected is an ImmutableArray) whereas they will silently pass if you write Assert.Equal(expected, source).

ianhays · 2017-01-10T23:32:17Z

src/System.Collections.Immutable/tests/ImmutableArrayTest.cs

-            Assert.Throws<NullReferenceException>(() => s_emptyDefault.Insert(1, 10));
-            Assert.Throws<NullReferenceException>(() => s_emptyDefault.Insert(0, 10));
-        }
+            // Once https://github.com/xunit/assert.xunit/pull/5 comes into corefx, all the StrongBox stuff can be removed.


Instead of doing this workaround, why don't we bring those necessary changes into buildtools?

Nice idea. I will look into making a PR to buildtools that updates xunit.

ianhays · 2017-01-10T23:38:43Z

src/System.Collections.Immutable/tests/ImmutableArrayTest.cs

+        {
+            var array = source.ToImmutableArray();
+
+            Assert.Throws<ArgumentNullException>("match", () => array.RemoveAll(match: null));
        }




Same here, lets look into bringing this into buildtools so we don't have to work around its absence.

ianhays · 2017-01-10T23:39:51Z

Thanks @jamesqo, these look good. Sorry for the delay in the review, there are quite a few changes here :) Did you do a coverage comparison to make sure no coverage of value was lost?

jamesqo · 2017-01-11T01:51:18Z

Did you do a coverage comparison to make sure no coverage of value was lost?

I looked into the code coverage reports and all lines in the type are still covered.

AArnott

There's no way I'll be confident from comparing the before and after review. But as you say code coverage is no worse, and as I like the theories you've built, it looks like a good change. Thank you.

Cleanup ImmutableArray tests Commit migrated from dotnet/corefx@bf0b6e6

dnfclas added the cla-already-signed label Jan 2, 2017

jamesqo commented Jan 2, 2017

View reviewed changes

danmoseley added the area-System.Collections label Jan 3, 2017

karelz assigned jamesqo and ianhays Jan 3, 2017

jamesqo force-pushed the ia.tests branch from 124df5a to 38b28ed Compare January 5, 2017 02:13

jamesqo changed the title ~~[WIP] Cleanup ImmutableArray tests~~ Cleanup ImmutableArray tests Jan 5, 2017

jamesqo commented Jan 5, 2017

View reviewed changes

jamesqo force-pushed the ia.tests branch from 38b28ed to 295ce63 Compare January 7, 2017 07:39

Cleanup ImmutableArray tests

295ce63

ianhays reviewed Jan 10, 2017

View reviewed changes

jamesqo mentioned this pull request Jan 12, 2017

Update xunit refs to beta4 dotnet/buildtools#1300

Closed

AArnott approved these changes Jan 17, 2017

View reviewed changes

ianhays approved these changes Jan 17, 2017

View reviewed changes

ianhays merged commit bf0b6e6 into dotnet:master Jan 17, 2017

jamesqo deleted the ia.tests branch January 17, 2017 20:33

jamesqo mentioned this pull request Jan 18, 2017

Fix #14961 and tweak perf in ImmutableArray #15296

Merged

karelz modified the milestone: 2.0.0 Jan 21, 2017

This was referenced Jan 31, 2020

Improve parameter names of Array.Copy dotnet/runtime#7185

Closed

Remove silent boxing/make exceptions consistent in ImmutableArray dotnet/runtime#19840

Closed

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#14798 from jamesqo/ia.tests

b2026a3

Cleanup ImmutableArray tests Commit migrated from dotnet/corefx@bf0b6e6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup ImmutableArray tests #14798

Cleanup ImmutableArray tests #14798

jamesqo commented Jan 2, 2017

jamesqo Jan 2, 2017 •

edited

Loading

AArnott Jan 7, 2017

jamesqo Jan 2, 2017

jamesqo Jan 2, 2017

jamesqo Jan 2, 2017

jamesqo Jan 2, 2017

jamesqo Jan 2, 2017

jamesqo commented Jan 5, 2017

jamesqo Jan 5, 2017

AArnott Jan 7, 2017

jamesqo Jan 7, 2017

jamesqo Jan 5, 2017

jamesqo Jan 5, 2017

jamesqo Jan 5, 2017

jamesqo Jan 5, 2017

jamesqo Jan 5, 2017

jamesqo Jan 5, 2017

jamesqo Jan 5, 2017

jamesqo Jan 5, 2017

jamesqo Jan 5, 2017

jamesqo commented Jan 5, 2017

ianhays Jan 10, 2017

jamesqo Jan 11, 2017

ianhays Jan 10, 2017

jamesqo Jan 11, 2017

ianhays Jan 10, 2017

ianhays commented Jan 10, 2017

jamesqo commented Jan 11, 2017

AArnott left a comment


		// TODO: The parameter names in CoreCLR need to be better.

		Assert.Throws<ArgumentNullException>("dest", () => array.CopyTo(null));

Cleanup ImmutableArray tests #14798

Cleanup ImmutableArray tests #14798

Conversation

jamesqo commented Jan 2, 2017

jamesqo Jan 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesqo commented Jan 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesqo commented Jan 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianhays commented Jan 10, 2017

jamesqo commented Jan 11, 2017

AArnott left a comment

Choose a reason for hiding this comment

jamesqo Jan 2, 2017 •

edited

Loading