Enable xunit1024 #34512

alexvy86 · 2020-04-03T17:53:38Z

There's a couple of recurring patterns in this PR:

A simple rename of a test method (e.g. add a suffix; If there are conventions around that that I'm not aware of, I'm happy to adjust the renames).
Replace a Fact that does nothing but call a helper method with different parameter values, with a Theory that uses InlineData (or just "inline" the redundant method, if there was only one call).
Replace a helper method with the same name as a Fact/Theory method, with a local function.

I'll make particular notes about changes that do not fall in one of the above patterns, or other comments, for easier review:

src/libraries/System.Text.Json/tests/Serialization/Value.ReadTests.GenericCollections.cs
- Removed ReadKeyValuePairOfKeyValuePair Fact since the Theory right after it already covers that case. The code is identical, and the inline data for the Theory already includes the value that the Fact used.
src/libraries/System.Private.Xml/tests/XmlSerialize/XmlSerializerTests.RuntimeOnly.cs
- Xml_GuidAsRoot_Helper and Xml_ListGenericRoot_Helper were just renames, but I noticed that they don't use their only parameter so maybe there's a problem there. To be handled in a separate issue if they actually are.
src/libraries/System.Diagnostics.StackTrace/tests/StackTraceTests.cs
- Ctor_ThrownException_GetFramesReturnsExpected might be redundant? Depending on the default value that fNeedFileInfo gets when instantiating a StackTrace, but I couldn't easily find where to check that.

Will fix #34503

The very next method is a Theory with inline data that tests this exact scenario.

dnfclas · 2020-04-03T17:54:45Z

All CLA requirements met.

danmoseley · 2020-04-03T18:36:59Z

Looks like there are some more instances to fix, in different configurations.

stephentoub · 2020-04-03T18:39:45Z

This is only fixing occurrences in System.Text.Json. Presumably there are others in many other test projects? Or is Json really the only one?

src/libraries/System.Text.Json/tests/Utf8JsonReaderTests.TryGet.cs

alexvy86 · 2020-04-03T19:08:58Z

Looks like there are some more instances to fix, in different configurations.

Right, I've only looked at netcoreapp, I'll address net472 hopefully during the weekend.

This is only fixing occurrences in System.Text.Json. Presumably there are others in many other test projects? Or is Json really the only one?

I was expecting to find more, and my intention with the PR was to fix them all... but I just rebuilt all of the libraries subcategory after fixing System.Text.Json and it apparently succeeded, so maybe there was much less work to do than I thought.

alexvy86 · 2020-04-03T19:19:58Z

but I just rebuilt all of the libraries subcategory after fixing System.Text.Json and it apparently succeeded, so maybe there was much less work to do than I thought.

Nvm, I built the libraries themselves, not their test projects, which are the ones with issues; now I'm seeing more errors. So yeah, I'll keep working on the rest of the test projects.

For RemoveNonExistingTest, just moved the logic from the single-use helper into the method with the Fact attribute.

In Math.cs, removed Round_Decimal_Digits Fact which manually tested cases included in the MemberData of a Theory with the same name.

Theory EndInit_ResumesPausedEnableRaisingEvents alredy tests the same scenario as the removed Fact of the same name.

stephentoub · 2020-04-07T13:53:15Z

src/libraries/Common/tests/System/IO/Compression/CompressionStreamUnitTestBase.cs

@@ -662,7 +662,7 @@ public static IEnumerable<object[]> RoundtripCompressDecompressOuterData
        }

        [Fact]
-        public async Task CompressDecompress_RoundTrip()
+        public async Task CompressDecompress_RoundTrip_BaseTests()


I'd prefer to see this test's name left as-is, and the method it delegates to instead be renamed to something like CompressDecompress_RoundTrip_OuterLoop. Same goes for the other similar cases here. My reason is "BaseTests" doesn't really speak to what the test is doing, plus this is the test that runs 99% of the time, and the one it delegates to is relegated to only running when we run "OuterLoop".

stephentoub · 2020-04-07T13:53:57Z

src/libraries/Common/tests/Tests/System/StringTests.cs

@@ -1666,7 +1666,7 @@ public static void MakeSureNoSequenceEqualChecksGoOutOfRange_Char()
        [InlineData("Hello", "llo" + SoftHyphen, StringComparison.OrdinalIgnoreCase, false)]
        [InlineData("", "", StringComparison.OrdinalIgnoreCase, true)]
        [InlineData("", "a", StringComparison.OrdinalIgnoreCase, false)]
-        public static void EndsWith(string s, string value, StringComparison comparisonType, bool expected)
+        public static void EndsWith_ExplicitComparisonType(string s, string value, StringComparison comparisonType, bool expected)


Suggested change

public static void EndsWith_ExplicitComparisonType(string s, string value, StringComparison comparisonType, bool expected)

public static void EndsWith_StringComparison(string s, string value, StringComparison comparisonType, bool expected)

stephentoub · 2020-04-07T13:54:09Z

src/libraries/Common/tests/Tests/System/StringTests.cs

@@ -4639,7 +4639,7 @@ public void Replace_EmptyOldValue_ThrowsArgumentException()
        [InlineData("Hello", SoftHyphen + "Hel", StringComparison.OrdinalIgnoreCase, false)]
        [InlineData("", "", StringComparison.OrdinalIgnoreCase, true)]
        [InlineData("", "hello", StringComparison.OrdinalIgnoreCase, false)]
-        public static void StartsWith(string s, string value, StringComparison comparisonType, bool expected)
+        public static void StartsWith_WithExplicitComparisonType(string s, string value, StringComparison comparisonType, bool expected)


Suggested change

public static void StartsWith_WithExplicitComparisonType(string s, string value, StringComparison comparisonType, bool expected)

public static void StartsWith_StringComparison(string s, string value, StringComparison comparisonType, bool expected)

stephentoub · 2020-04-07T13:55:17Z

src/libraries/Common/tests/Tests/System/StringTests.cs

@@ -5222,7 +5222,7 @@ public static void LengthMismatchToLower()
        }

        [Fact]
-        public static void ToLower()
+        public static void ToLower_2()


What is the other ToLower test this is conflicting with? "ToLower_2" isn't descriptive about what makes this test different from a presumed "ToLower_1", so we should either pick descriptive names or combine the tests.

It's this one, using strings + Span instead of char[] + Span throughout. How about naming them ToLower_String and ToLower_CharArray?

How about naming them ToLower_String and ToLower_CharArray?

Sounds fine. Thanks.

stephentoub · 2020-04-07T13:55:25Z

src/libraries/Common/tests/Tests/System/StringTests.cs

@@ -5434,7 +5434,7 @@ public static void LengthMismatchToUpper()
        }

        [Fact]
-        public static void ToUpper()
+        public static void ToUpper_2()


...raries/System.Collections.Concurrent/tests/ConcurrentDictionary/ConcurrentDictionaryTests.cs

stephentoub · 2020-04-07T14:01:31Z

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

@@ -838,7 +838,7 @@ public static IEnumerable<object[]> EnumeratorTraversalNullData()

        [Theory]
        [MemberData(nameof(EqualsData))]
-        public void Equals(ImmutableArray<int> first, ImmutableArray<int> second, bool expected)
+        public void EqualsBaseTest(ImmutableArray<int> first, ImmutableArray<int> second, bool expected)


Elsewhere you used EqualsTest. Why EqualsBaseTest here?

I thought it had been because of some class that inherited from this and had its own EqualsTest, but apparently it's not the case here, I'll simplify to EqualsTest.

src/libraries/System.Collections.Immutable/tests/ImmutableSetTest.cs

stephentoub · 2020-04-07T14:10:42Z

src/libraries/System.Diagnostics.StackTrace/tests/StackTraceTests.cs

@@ -107,6 +107,7 @@ public void Ctor_LargeSkipFramesFNeedFileInfo_GetFramesReturnsEmpty(bool fNeedFi
        [Fact]
        public void Ctor_ThrownException_GetFramesReturnsExpected()
        {
+            // TODO: is this test redundant? See Ctor_ThrownException_GetFramesReturnsExpected_2 below


It's not redundant, it's testing a different StackTrace constructor.

Good point, I'll remove the comment

stephentoub · 2020-04-07T14:10:54Z

src/libraries/System.Diagnostics.StackTrace/tests/StackTraceTests.cs

@@ -124,7 +125,7 @@ public void Ctor_EmptyException_GetFramesReturnsEmpty()
        [Theory]
        [InlineData(true)]
        [InlineData(false)]
-        public void Ctor_ThrownException_GetFramesReturnsExpected(bool fNeedFileInfo)
+        public void Ctor_ThrownException_GetFramesReturnsExpected_2(bool fNeedFileInfo)


Suggested change

public void Ctor_ThrownException_GetFramesReturnsExpected_2(bool fNeedFileInfo)

public void Ctor_Bool_ThrownException_GetFramesReturnsExpected(bool fNeedFileInfo)

src/libraries/System.Drawing.Common/tests/mono/System.Drawing/GraphicsTests.cs

stephentoub · 2020-04-07T14:14:15Z

src/libraries/System.IO.FileSystem/tests/FileStream/WriteAsync.cs

@@ -279,7 +279,7 @@ public static IEnumerable<object[]> MemberData_FileStreamAsyncWriting()
        }

        [Fact]
-        public Task ManyConcurrentWriteAsyncs()
+        public Task ManyConcurrentWriteAsyncs_BaseTest()


Same feedback as in a previous similar case... let's leave this one as ManyConcurrentWriteAsyncs, and change the one below to ManyConcurrentWriteAsyncs_OuterLoop.

stephentoub · 2020-04-07T14:14:53Z

src/libraries/System.Linq/tests/AnyTests.cs

@@ -59,7 +59,7 @@ public static IEnumerable<object[]> TestData()

        [Theory]
        [MemberData(nameof(TestData))]
-        public void Any(IEnumerable<int> source, bool expected)
+        public void AnyTest(IEnumerable<int> source, bool expected)


This should stay Any, and the test below should be changed to be Any_Predicate, or something like that.

stephentoub · 2020-04-07T14:19:45Z

src/libraries/System.Runtime/tests/System/StringTests.cs

@@ -200,7 +200,7 @@ public static void Contains(string s, char value, bool expected)
        [InlineData("Hello", 'e', StringComparison.OrdinalIgnoreCase, true)]
        [InlineData("Hello", 'E', StringComparison.OrdinalIgnoreCase, true)]
        [InlineData("", 'H', StringComparison.OrdinalIgnoreCase, false)]
-        public static void Contains(string s, char value, StringComparison comparisionType, bool expected)
+        public static void Contains_Char_ExplicitComparisonType(string s, char value, StringComparison comparisionType, bool expected)


Suggested change

public static void Contains_Char_ExplicitComparisonType(string s, char value, StringComparison comparisionType, bool expected)

public static void Contains_Char_StringComparison(string s, char value, StringComparison comparisionType, bool expected)

stephentoub · 2020-04-07T14:19:53Z

src/libraries/System.Runtime/tests/System/StringTests.cs

@@ -272,7 +272,7 @@ public static void Contains(string s, char value, StringComparison comparisionTy
        [InlineData("Hello", "", StringComparison.OrdinalIgnoreCase, true)]
        [InlineData("Hello", "ell" + SoftHyphen, StringComparison.OrdinalIgnoreCase, false)]
        [InlineData("Hello", "Ell" + SoftHyphen, StringComparison.OrdinalIgnoreCase, false)]
-        public static void Contains(string s, string value, StringComparison comparisonType, bool expected)
+        public static void Contains_String_ExplicitComparisonType(string s, string value, StringComparison comparisonType, bool expected)


Suggested change

public static void Contains_String_ExplicitComparisonType(string s, string value, StringComparison comparisonType, bool expected)

public static void Contains_String_StringComparison(string s, string value, StringComparison comparisonType, bool expected)

stephentoub · 2020-04-07T14:23:16Z

src/libraries/System.Security.Cryptography.Algorithms/tests/RijndaelTests.cs

@@ -19,38 +19,38 @@ public class RijndaelTests
        [Fact]
        public static void VerifyDefaults()
        {
+            static void verifyDefaults(Rijndael alg)


In situations like this, where the test method differs from the local function by the casing of a single letter, it'd be really easy to accidentally get a stack overflow or an infinite loop by calling the wrong thing. Could you rename such local functions to something a little more different?

Sure, I actually started using test for the local function at some point, I missed updating some of the ones I did earlier.

stephentoub · 2020-04-07T14:24:55Z

src/libraries/System.Security.Cryptography.Xml/tests/SignedXmlTest.cs

@@ -1413,9 +1413,12 @@ static void CheckErratum(SignedXml signed, KeyedHashAlgorithm hmac, string messa
            }
        }

-        private void HmacMustBeMultipleOfEightBits(int bits)
+        [Fact(Skip = "https://github.com/dotnet/runtime/issues/20429")]


Nit: as long as you're changing this, can you make it instead be:

[ActiveIssue("https://github.com/dotnet/runtime/issues/20429")] [Fact]

?

stephentoub · 2020-04-07T14:26:10Z

src/libraries/System.Text.Json/tests/Serialization/Value.ReadTests.GenericCollections.cs

@@ -997,16 +997,6 @@ public static void ReadKeyValuePairOfList()
            Assert.Equal(3, input.Value[2]);
        }

-        [Fact]
-        public static void ReadKeyValuePairOfKeyValuePair()


Was this a dup? Wondering why it's being deleted.

Yes, see the first bullet point of the second set in the description, there's a Theory testing the exact same thing, and the data that this method uses is already part of the InlineData of the Theory.

stephentoub

Thanks for cleaning all this up!

alexvy86 · 2020-04-08T03:39:57Z

The check failures don't seem related to the changes, right? Is it possible to retrigger only the failed builds?

stephentoub · 2020-04-08T10:58:44Z

Thanks!

alexvy86 added 3 commits April 3, 2020 12:39

Enable analyzer rule xUnit1024

e8331c7

Remove redundant test in System.Text.Json

8e80f75

The very next method is a Theory with inline data that tests this exact scenario.

Rename test methods in System.Text.Json

e79f8a0

alexvy86 requested review from ahsonkhan, jozkee, layomia and steveharter as code owners April 3, 2020 17:53

Dotnet-GitSync-Bot added the area-System.Text.Json label Apr 3, 2020

danmoseley approved these changes Apr 3, 2020

View reviewed changes

stephentoub reviewed Apr 3, 2020

View reviewed changes

src/libraries/System.Text.Json/tests/Utf8JsonReaderTests.TryGet.cs Outdated Show resolved Hide resolved

stephentoub reviewed Apr 3, 2020

View reviewed changes

src/libraries/System.Text.Json/tests/Utf8JsonReaderTests.TryGet.cs Outdated Show resolved Hide resolved

alexvy86 added 14 commits April 5, 2020 16:58

Private methods -> local functions in System.Text.Json

42742c0

Rename test methods in System.ComponentModel.Annotations

f39c409

Rename test methods in System.Collections.Immutable

a25a75f

For RemoveNonExistingTest, just moved the logic from the single-use helper into the method with the Fact attribute.

Rename test methods in System.Globalization

eb5596a

Rename test methods in System.Runtime.Extensions

f1a4c49

In Math.cs, removed Round_Decimal_Digits Fact which manually tested cases included in the MemberData of a Theory with the same name.

Rename test methods in System.Private.Uri

6a6b739

Rename test methods in System.Runtime.WindowsRuntime.UI.Xaml

f773c3b

Rename test methods in System.Security.Cryptography.X509Certificates

d04193c

Rename test methods in System.Reflection

d7e6307

Remove redundant test in System.IO.FileSystem.Watcher

093eb26

Theory EndInit_ResumesPausedEnableRaisingEvents alredy tests the same scenario as the removed Fact of the same name.

Rename test methods in System.IO.FileSystem.Watcher

7e9cece

Rename test methods in System.Composition.Runtime

87bae0f

Rename test methods in System.Drawing.Primitives

999eac4

Rename test methods in System.Reflection.Emit

9f3a8ae

stephentoub reviewed Apr 7, 2020

View reviewed changes

...raries/System.Collections.Concurrent/tests/ConcurrentDictionary/ConcurrentDictionaryTests.cs Show resolved Hide resolved

stephentoub reviewed Apr 7, 2020

View reviewed changes

src/libraries/System.Collections.Immutable/tests/ImmutableSetTest.cs Outdated Show resolved Hide resolved

stephentoub reviewed Apr 7, 2020

View reviewed changes

src/libraries/System.Drawing.Common/tests/mono/System.Drawing/GraphicsTests.cs Show resolved Hide resolved

stephentoub reviewed Apr 7, 2020

View reviewed changes

stephentoub approved these changes Apr 7, 2020

View reviewed changes

alexvy86 added 3 commits April 7, 2020 09:41

Improve some test method renames

e3e0049

Minor nit fixing

3d29c9f

More improvements to test method names

2f0bc0c

stephentoub merged commit 61733e2 into dotnet:master Apr 8, 2020

danmoseley added the area-Meta label Apr 16, 2020

ghost locked as resolved and limited conversation to collaborators Dec 9, 2020

alexvy86 deleted the enable-xunit1024 branch April 20, 2024 23:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable xunit1024 #34512

Enable xunit1024 #34512

alexvy86 commented Apr 3, 2020 •

edited

Loading

dnfclas commented Apr 3, 2020 •

edited

Loading

danmoseley commented Apr 3, 2020

stephentoub commented Apr 3, 2020

alexvy86 commented Apr 3, 2020

alexvy86 commented Apr 3, 2020

stephentoub Apr 7, 2020

stephentoub Apr 7, 2020

stephentoub Apr 7, 2020

stephentoub Apr 7, 2020

alexvy86 Apr 7, 2020

stephentoub Apr 7, 2020

stephentoub Apr 7, 2020

stephentoub Apr 7, 2020

alexvy86 Apr 7, 2020

stephentoub Apr 7, 2020

alexvy86 Apr 7, 2020

stephentoub Apr 7, 2020

stephentoub Apr 7, 2020

stephentoub Apr 7, 2020

stephentoub Apr 7, 2020

stephentoub Apr 7, 2020

stephentoub Apr 7, 2020

alexvy86 Apr 7, 2020

stephentoub Apr 7, 2020

stephentoub Apr 7, 2020

alexvy86 Apr 7, 2020

stephentoub left a comment

alexvy86 commented Apr 8, 2020

stephentoub commented Apr 8, 2020

	public static void EndsWith_ExplicitComparisonType(string s, string value, StringComparison comparisonType, bool expected)
	public static void EndsWith_StringComparison(string s, string value, StringComparison comparisonType, bool expected)

	public static void StartsWith_WithExplicitComparisonType(string s, string value, StringComparison comparisonType, bool expected)
	public static void StartsWith_StringComparison(string s, string value, StringComparison comparisonType, bool expected)

	public void Ctor_ThrownException_GetFramesReturnsExpected_2(bool fNeedFileInfo)
	public void Ctor_Bool_ThrownException_GetFramesReturnsExpected(bool fNeedFileInfo)

	public static void Contains_Char_ExplicitComparisonType(string s, char value, StringComparison comparisionType, bool expected)
	public static void Contains_Char_StringComparison(string s, char value, StringComparison comparisionType, bool expected)

	public static void Contains_String_ExplicitComparisonType(string s, string value, StringComparison comparisonType, bool expected)
	public static void Contains_String_StringComparison(string s, string value, StringComparison comparisonType, bool expected)

Enable xunit1024 #34512

Enable xunit1024 #34512

Conversation

alexvy86 commented Apr 3, 2020 • edited Loading

dnfclas commented Apr 3, 2020 • edited Loading

danmoseley commented Apr 3, 2020

stephentoub commented Apr 3, 2020

alexvy86 commented Apr 3, 2020

alexvy86 commented Apr 3, 2020

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

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

stephentoub left a comment

Choose a reason for hiding this comment

alexvy86 commented Apr 8, 2020

stephentoub commented Apr 8, 2020

alexvy86 commented Apr 3, 2020 •

edited

Loading

dnfclas commented Apr 3, 2020 •

edited

Loading