Enable xunit1024 by alexvy86 · Pull Request #34512 · dotnet/runtime

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 ReadKeyValuePairOfKeyValuePairFact 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?

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


 [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

 [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
publicstaticvoidEndsWith_ExplicitComparisonType(strings,stringvalue,StringComparisoncomparisonType,boolexpected)
publicstaticvoidEndsWith_StringComparison(strings,stringvalue,StringComparisoncomparisonType,boolexpected)

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

 [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
publicstaticvoidStartsWith_WithExplicitComparisonType(strings,stringvalue,StringComparisoncomparisonType,boolexpected)
publicstaticvoidStartsWith_StringComparison(strings,stringvalue,StringComparisoncomparisonType,boolexpected)

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


 [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


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


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

 [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.

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

 [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

 [InlineData(true)]
 [InlineData(false)]
- public void Ctor_ThrownException_GetFramesReturnsExpected(bool fNeedFileInfo)
+ public void Ctor_ThrownException_GetFramesReturnsExpected_2(bool fNeedFileInfo)


Suggested change
publicvoidCtor_ThrownException_GetFramesReturnsExpected_2(boolfNeedFileInfo)
publicvoidCtor_Bool_ThrownException_GetFramesReturnsExpected(boolfNeedFileInfo)

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


 [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

 [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

 [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
publicstaticvoidContains_Char_ExplicitComparisonType(strings,charvalue,StringComparisoncomparisionType,boolexpected)
publicstaticvoidContains_Char_StringComparison(strings,charvalue,StringComparisoncomparisionType,boolexpected)

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

 [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
publicstaticvoidContains_String_ExplicitComparisonType(strings,stringvalue,StringComparisoncomparisonType,boolexpected)
publicstaticvoidContains_String_StringComparison(strings,stringvalue,StringComparisoncomparisonType,boolexpected)

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

 [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

 }

- 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

 }

- [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

Comment threadsrc/libraries/System.Text.Json/tests/Utf8JsonReaderTests.TryGet.cs Outdated

stephentoub reviewed Apr 3, 2020
View reviewed changes

Comment threadsrc/libraries/System.Text.Json/tests/Utf8JsonReaderTests.TryGet.cs Outdated

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

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

stephentoub reviewed Apr 7, 2020
View reviewed changes

Comment threadsrc/libraries/System.Collections.Immutable/tests/ImmutableSetTest.cs Outdated

stephentoub reviewed Apr 7, 2020
View reviewed changes

Comment threadsrc/libraries/System.Drawing.Common/tests/mono/System.Drawing/GraphicsTests.cs

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:masterApr 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

	publicstaticvoidEndsWith_ExplicitComparisonType(strings,stringvalue,StringComparisoncomparisonType,boolexpected)
	publicstaticvoidEndsWith_StringComparison(strings,stringvalue,StringComparisoncomparisonType,boolexpected)

	publicstaticvoidStartsWith_WithExplicitComparisonType(strings,stringvalue,StringComparisoncomparisonType,boolexpected)
	publicstaticvoidStartsWith_StringComparison(strings,stringvalue,StringComparisoncomparisonType,boolexpected)

	publicvoidCtor_ThrownException_GetFramesReturnsExpected_2(boolfNeedFileInfo)
	publicvoidCtor_Bool_ThrownException_GetFramesReturnsExpected(boolfNeedFileInfo)

	publicstaticvoidContains_Char_ExplicitComparisonType(strings,charvalue,StringComparisoncomparisionType,boolexpected)
	publicstaticvoidContains_Char_StringComparison(strings,charvalue,StringComparisoncomparisionType,boolexpected)

	publicstaticvoidContains_String_ExplicitComparisonType(strings,stringvalue,StringComparisoncomparisonType,boolexpected)
	publicstaticvoidContains_String_StringComparison(strings,stringvalue,StringComparisoncomparisonType,boolexpected)

Uh oh!

Conversation

alexvy86 commented Apr 3, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfclas commented Apr 3, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danmoseley commented Apr 3, 2020

Uh oh!

stephentoub commented Apr 3, 2020

Uh oh!

Uh oh!

Uh oh!

alexvy86 commented Apr 3, 2020

Uh oh!

alexvy86 commented Apr 3, 2020

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

alexvy86 commented Apr 8, 2020

Uh oh!

stephentoub commented Apr 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

alexvy86 commented Apr 3, 2020•
edited
Loading

dnfclas commented Apr 3, 2020•
edited
Loading