System.Lazy regression tests #15577

manofstick · 2017-01-28T09:18:41Z

This is a prelude to an alternative Lazy implementation in a PR at
dotnet/coreclr#8963

This is a prelude to an alternative Lazy implementation in a PR at dotnet/coreclr#8963

stephentoub · 2017-01-28T14:03:01Z

src/System.Runtime/tests/System/LazyTests.regression.cs

+    /// the original suite as well as this one. These tests were written with the express purpose of testing
+    /// that the implementation conforms to the existing implementation, rather than to specification.
+    /// The two reasons obviously have major overlap (obviously), but leads to the flavour of this suite 
+    /// being rather dense and repeditive, which for regression I feel is suitable.


While I appreciate the sentiment here, in a month or a year or whatever, it's not really going to matter that the implementation changed: then we really just want a suite that validates Lazy's behavior. We should avoid duplication and have a single suite that validates the behavior. Let's just add tests for things that aren't currently being exercised.

stephentoub · 2017-01-28T15:59:42Z

@dotnet-bot test this please

AlexGhiondea

Thank you!

manofstick · 2017-02-02T05:21:28Z

@AlexGhiondea

I will probably bash this code a little bit; so don't pull out in yet. I have just been a bit busy, sorry.

Just looking at consolidating some of the tests as per @stephentoub comments.

Hopefully sometime over this coming weekend.

AlexGhiondea · 2017-02-02T05:48:53Z

@manofstick sounds good! Thanks for working on this!

manofstick · 2017-02-05T08:28:53Z

@AlexGhiondea

OK; I think I'm good now.

stephentoub · 2017-02-05T17:30:42Z

src/System.Runtime/tests/System/LazyTests.cs

-            Lazy<string> lazy = null;
-            lazy = new Lazy<string>(() =>  x++ < 5 ? lazy.Value : "Test", true);
+        [Fact]
+        public static void Ctor_ExceptionRecovery()


In general we try to use [Theory] with [MemberData] for things like this. Your previous case made sense not to use that, as it would have been difficult/cumbersome to handle needing to construct the lazy instances with a delegate that referred back to the same instance. In this case, though, it'd be pretty straightforward to do it that way, e.g.

public static IEnumerable<object[]> Ctor_ExceptionRecovery_MemberData() { yield return new object[] { new Lazy<InitiallyExceptionThrowingCtor>(), 5 }; yield return new object[] { new Lazy<InitiallyExceptionThrowingCtor>(true), 5 }; ... // etc. } [Theory] [MemberData(nameof(Ctor_ExceptionRecovery_MemberData))] public static void Ctor_ExceptionRecovery(Lazy<InitiallyExceptionThrowingCtor> lazy, int? expected) { ... // same body of Ctor_ExceptionRecovery_Impl test you currently have }

That helps in a variety of ways, e.g. having xunit run each test case as its own test such that one failing doesn't cause the rest to fail, a description from xunit about exactly which case failed, etc., plus consistency with how we're trying to do these across corefx (there are a bunch of tests across corefx we haven't yet converted to that style, that we just ported as is from the internal tests used for desktop, but for all new ones we write we're trying to follow that style).

stephentoub · 2017-02-05T17:32:46Z

src/System.Runtime/tests/System/LazyTests.cs

+        [Fact]
+        public static void CheckExceptions()
+        {
+            CheckException(typeof(MyException), new Lazy<int>(() => { throw new MyException(99); }));


Why can't Assert.Throws<MyException> be used instead of this custom CheckException method?

stephentoub · 2017-02-05T17:35:48Z

src/System.Runtime/tests/System/LazyTests.cs

+            {
+                Assert.Same(first, thrown2);
+            }
+        }


I think this would be better as something like:

private static void AssertIdempotentException<T>(Lazy<T> x) { MyException e = Assert.Throws<MyException>(() => x.Value); Assert.Same(e, Assert.Throws<MyException>(() => x.Value)); ]

In addition to being more concise, it also ensure that the second call to Value throws (the existing version will still succeed if the second call doesn't throw).

stephentoub · 2017-02-05T17:36:26Z

src/System.Runtime/tests/System/LazyTests.cs

+            Assert.NotNull(second);
+
+            Assert.NotEqual(first, second);
+        }


This can similarly be condensed, and just use Assert.NotSame instead of Assert.Same.

stephentoub · 2017-02-05T17:36:48Z

src/System.Runtime/tests/System/LazyTests.cs

+            SameException(new Lazy<ExceptionInCtor>(() => new ExceptionInCtor(99), LazyThreadSafetyMode.None));
+
+            DifferentException(new Lazy<ExceptionInCtor>(() => new ExceptionInCtor(99), LazyThreadSafetyMode.PublicationOnly));
+        }


Another case where a [Theory] would be good (or, actually, two, one for the same case and one for the different case... effectively the assert methods I previously described become the [Theory], with a separate MemberData supplying the values).

stephentoub · 2017-02-05T17:38:29Z

src/System.Runtime/tests/System/LazyTests.cs

+        }
+
+        [Fact]
+        static void Serialization_ValueType()


Does this run if it's not public?

stephentoub · 2017-02-05T17:38:36Z

src/System.Runtime/tests/System/LazyTests.cs

+        }
+
+        [Fact]
+        static void Serialization_RefType()


Does this run if it's not public?

stephentoub · 2017-02-05T17:40:10Z

src/System.Runtime/tests/System/LazyTests.cs

+            var fortytwo = (Lazy<int>)formatter.Deserialize(stream);
+            Assert.True(fortytwo.IsValueCreated);
+            Assert.Equal(fortytwo.Value, 42);
+        }


We have a small helper for this in:
https://github.com/dotnet/corefx/blob/master/src/Common/tests/System/Runtime/Serialization/Formatters/BinaryFormatterHelpers.cs
With that this would become something like:

[Fact] public static void Serialization_Int32_Roundtrips() { Lazy<int> clone = BinaryFormatterHelpers.Clone(new Lazy<int>(() => 42)); Assert.True(clone.IsValueCreated); Assert.Equal(42, clone.Value); }

Argh, running out of time and for some reason this just isn't working... I can see it used in ModuleTests.cs and PointerTests.cs, and I can see in the project file that there is a link to BinaryFormatterHelper.cs, but for some reason it just isn't being picked up; I'm just not seeing the System.Runtime.Serialization.Formatters.Tests namespace at all, and my time is running out, so I've left this as it originally was (baring the public change)

stephentoub · 2017-02-05T17:40:18Z

src/System.Runtime/tests/System/LazyTests.cs

+
+            var fortytwo = (Lazy<string>)formatter.Deserialize(stream);
+            Assert.True(fortytwo.IsValueCreated);
+            Assert.Equal(fortytwo.Value, "42");


stephentoub · 2017-02-05T17:40:48Z

@manofstick, thank you for adding these (and addressing my previous comments)! I've left a few more comments on things I think it would be good to address.

manofstick · 2017-02-07T08:47:06Z

@stephentoub

OK modified to use the Theory attribute (and other changes you had mentioned). Theory didn't seem to work with generic arguments, so had to duplicate some of the code with type specific methods; not sure if it ends up clearer than the original (But I'm not a huge fan of attributes so maybe just my personal preference!)

danmoseley · 2017-02-13T22:56:01Z

@stephentoub is this addressed to your satisfaction?

danmoseley · 2017-02-13T22:56:13Z

@dotnet-bot test this please (logs gone)

stephentoub · 2017-02-16T04:53:22Z

src/System.Runtime/tests/System/LazyTests.cs

 using System.Threading;
+using System.Reflection;
+using System.IO;
+using System.Runtime.Serialization.Formatters.Binary;


This is missing a using System.Runtime.Serialization.Formatters.Tests;... I believe that's the cause of the CI failures.

stephentoub · 2017-02-16T04:54:14Z

src/System.Runtime/tests/System/LazyTests.cs

+        private static void Value_Invalid_Impl<T>(ref Lazy<T> x, Lazy<T> lazy)
+        {
+            x = lazy;
+            Assert.Throws<InvalidOperationException>(() => { var dummy = lazy.Value; });


Nit: the () => { var dummy = lazy.Value; } can I believe be simplified to just () => lazy.Value

stephentoub · 2017-02-16T04:56:32Z

src/System.Runtime/tests/System/LazyTests.cs

+        public static void Ctor_ExceptionRecovery(Lazy<InitiallyExceptionThrowingCtor> lazy, int? expected)
+        {
+            InitiallyExceptionThrowingCtor.counter = 0;
+            var result = default(InitiallyExceptionThrowingCtor);


Nit: InitialiallyExceptionthrowingCtor result = null;

stephentoub · 2017-02-16T04:57:17Z

src/System.Runtime/tests/System/LazyTests.cs

+                try { result = lazy.Value; } catch (Exception) { }
+            }
+            if (expected == null)
+                Assert.Null(result);


null doesn't appear to ever be expected, is it?

stephentoub · 2017-02-16T04:57:59Z

src/System.Runtime/tests/System/LazyTests.cs

+        private static void Value_ExceptionRecovery_IntImpl(Lazy<int> lazy, ref int counter, int expected)
+        {
+            counter = 0;
+            var result = 0;


Nit: var => int

stephentoub · 2017-02-16T05:10:27Z

src/System.Runtime/tests/System/LazyTests.cs

+            if (expected == null)
+                Assert.Null(result);
+            else
+                Assert.Equal(result, expected);


Nit: this could just be Assert.Equal(expected, result) rather than special-casing expected == null... if expected is null, we expect result to be null, so Assert.Equal will work fine.

Also note that expected should be the first argument to Equal, here and elsewhere

stephentoub · 2017-02-16T05:10:59Z

src/System.Runtime/tests/System/LazyTests.cs

+        [Fact]
+        public static void Value_ExceptionRecovery()
+        {
+            var counter = default(int); // set in test function


Nit: int counter = 0;

stephentoub

Thanks for adding all of these. A few comments, but otherwise LGTM.

stephentoub · 2017-02-19T22:52:23Z

Thanks!

* System.Lazy regression tests This is a prelude to an alternative Lazy implementation in a PR at dotnet/coreclr#8963 Commit migrated from dotnet/corefx@029f4c4

System.Lazy regression tests

4e93c2e

This is a prelude to an alternative Lazy implementation in a PR at dotnet/coreclr#8963

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

manofstick mentioned this pull request Jan 28, 2017

Alternative System.Lazy implementation dotnet/coreclr#8963

Merged

7 tasks

stephentoub reviewed Jan 28, 2017

View reviewed changes

karelz added the area-System.Runtime label Jan 28, 2017

karelz assigned manofstick, AlexGhiondea and joperezr Jan 28, 2017

AlexGhiondea approved these changes Feb 2, 2017

View reviewed changes

Consolidated tests; added serialization test.

75ca36c

stephentoub reviewed Feb 5, 2017

View reviewed changes

Modified to utilise TheoryAttribute

0ea75c5

stephentoub reviewed Feb 16, 2017

View reviewed changes

stephentoub approved these changes Feb 16, 2017

View reviewed changes

Minor clean up as per review

77bfcd7

stephentoub merged commit 029f4c4 into dotnet:master Feb 19, 2017

karelz modified the milestone: 2.0.0 Feb 22, 2017

System.Lazy regression tests #15577

System.Lazy regression tests #15577

Uh oh!

Conversation

manofstick commented Jan 28, 2017

Uh oh!

stephentoub Jan 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub commented Jan 28, 2017

Uh oh!

AlexGhiondea left a comment

Choose a reason for hiding this comment

Uh oh!

manofstick commented Feb 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexGhiondea commented Feb 2, 2017

Uh oh!

manofstick commented Feb 5, 2017

Uh oh!

stephentoub Feb 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub Feb 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub commented Feb 5, 2017

Uh oh!

manofstick commented Feb 7, 2017

Uh oh!

danmoseley commented Feb 13, 2017

Uh oh!

danmoseley commented Feb 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

stephentoub commented Feb 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

stephentoub Jan 28, 2017 •

edited

Loading

manofstick commented Feb 2, 2017 •

edited

Loading

stephentoub Feb 5, 2017 •

edited

Loading

stephentoub Feb 5, 2017 •

edited

Loading