Tests should use theory data #11171

woefulpines · 2024-04-06T17:47:25Z

Makes progress to fix #11041

Proposed changes

Changed untyped test data to use typed TheoryData objects.
Allowing us to enable xUnit1024 errors in .editorconfig.
Currently fixes test cases in Windows form analyzer (CSharp) with one exception
for the file reader font converter.

Customer Impact

No customer impact, just refactoring old test cases.

Regression?

No

Risk

-There should be no risk in moving to strongly typed parameterized test methods.

Test methodology

Because the tests are functionally identical, I used the tests that were already present.

Test environment(s)

Microsoft Reviewers: Open in CodeFlow

A typed theory data object is used in the test for building the apps configuration.

The test now uses a typed theory data object for the configuration generator tests.

A typed TheoryData Obejct is now used to pass in parameters to the Font converter test.

…winforms into Tests_should_use_TheoryData

…t1024.

woefulpines · 2024-04-06T17:48:51Z

@dotnet-policy-service agree

woefulpines · 2024-04-06T17:49:53Z

For the testing environments, would you like me to paste my dotnet --info or could I remove it?

codecov · 2024-04-06T18:17:15Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.44649%. Comparing base (d9e9d93) to head (9bed97d).
Report is 120 commits behind head on main.

Additional details and impacted files

@@                 Coverage Diff                 @@
##                main      #11171         +/-   ##
===================================================
- Coverage   73.44664%   73.44649%   -0.00016%     
===================================================
  Files           3098        3098                 
  Lines         632225      632229          +4     
  Branches       46653       46653                 
===================================================
+ Hits          464348      464350          +2     
- Misses        164485      164487          +2     
  Partials        3392        3392

Flag	Coverage Δ
Debug	`73.44649% <100.00000%> (-0.00016%)`	⬇️
integration	`18.36830% <ø> (+0.00637%)`	⬆️
production	`46.91082% <ø> (-0.00071%)`	⬇️
test	`94.98604% <100.00000%> (+0.00005%)`	⬆️
unit	`43.87654% <ø> (-0.00107%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

RussKie · 2024-04-09T01:35:47Z

...rp/tests/UnitTests/System/Windows/Forms/Generators/ApplicationConfigurationGeneratorTests.cs

    {
+        var testData = new TheoryData<OutputKind>();


[nit]

Suggested change

var testData = new TheoryData<OutputKind>();

TheoryData<OutputKind> testData = new();

lonitra

Left a few comments, thanks for helping out in this area!

lonitra · 2024-04-09T16:19:12Z

...harp/tests/UnitTests/System/Windows/Forms/Generators/ProjectFileReaderTests.FontConverter.cs

        [MemberData(nameof(TestConvertFormData))]
+        #pragma warning restore xUnit1042 // The member referenced by the MemberData attribute returns untyped data rows


We typically do not want this comment twice for suppressions

Suggested change

#pragma warning restore xUnit1042 // The member referenced by the MemberData attribute returns untyped data rows

#pragma warning restore xUnit1042

The comment in both locations is automatically added by Roslyn's code fix

Yep, for the most part we've been manually removing the comment on restore when adding these warnings

lonitra · 2024-04-09T16:25:20Z

.../UnitTests/System/Windows/Forms/Generators/ApplicationConfigurationInitializeBuilderTests.cs

+            testData.Add
+            (


nit:

Suggested change

testData.Add

(

testData.Add(

I would have expected this one to be flagged by analyzers

lonitra · 2024-04-09T16:30:01Z

...harp/tests/UnitTests/System/Windows/Forms/Generators/ProjectFileReaderTests.FontConverter.cs

@@ -57,7 +57,9 @@ public static IEnumerable<object[]> TestConvertFormData()
        }

        [Theory]
+        #pragma warning disable xUnit1042 // The member referenced by the MemberData attribute returns untyped data rows


What change caused this warning?

Sorry for the delay. Disabling the warning addressed the function uses an enum that is marked as internal. So I was not able to put it into a TheoryData<...> object getting a CS0050 error. I thought disabling was the best course of action for this specific test case.

Ah I see. Thank you for your explanation. I think disabling for this case is fine.

If this is a common situation (it may be), we could implement a DiagnosticSuppressor that automatically suppresses xUnit1042 for these cases.

Thank you all for the help and comments.

sharwell · 2024-04-09T16:50:05Z

...harp/tests/UnitTests/System/Windows/Forms/Generators/ProjectFileReaderTests.FontConverter.cs

@@ -113,4 +115,4 @@ public void EmptyStringInput()
            Assert.Null(font);
        }
    }
-}
+}


This seems undesirable. I can understand not forcing it to move the other direction to fix a problem, but this is creating a problem where there wasn't one.

lonitra · 2024-04-25T15:57:24Z

@woefulpines could you update PR with feedback addressed so we can take this to completion? Let me know if there are any concerns :)

woefulpines and others added 11 commits April 3, 2024 16:28

Converted the test for the configuration builder to use TheoryData.

4c27d23

A typed theory data object is used in the test for building the apps configuration.

Converted the unsuported project types test to use TheoryData.

190195a

The test now uses a typed theory data object for the configuration generator tests.

Converted the convert form data test to use TheoryData.

e64354b

A typed TheoryData Obejct is now used to pass in parameters to the Font converter test.

Added culture info type to the theory data object.

424fd3e

Changed TheoryData types to proper types used in the test.

0c34cd3

Merge branch 'dotnet:main' into Tests_should_use_TheoryData

2aa4dec

Merge branch 'Tests_should_use_TheoryData' of github.com:woefulpines/…

c2db893

…winforms into Tests_should_use_TheoryData

Fixed the formatting for some testData creation in file reader test.

6887caa

Merge branch 'main' into Tests_should_use_TheoryData

4c8ded3

Reverted to the old test case without theory data and suppressed xUni…

e0b07c2

…t1024.

Merge branch 'main' into Tests_should_use_TheoryData

9bed97d

woefulpines requested a review from a team as a code owner April 6, 2024 17:47

dotnet-policy-service bot assigned woefulpines Apr 6, 2024

RussKie reviewed Apr 9, 2024

View reviewed changes

lonitra reviewed Apr 9, 2024

View reviewed changes

lonitra added the 📭 waiting-author-feedback The team requires more information from the author label Apr 9, 2024

sharwell reviewed Apr 9, 2024

View reviewed changes

dotnet-policy-service bot removed the 📭 waiting-author-feedback The team requires more information from the author label Apr 18, 2024

lonitra added the 📭 waiting-author-feedback The team requires more information from the author label Apr 18, 2024

dotnet-policy-service bot removed the 📭 waiting-author-feedback The team requires more information from the author label Apr 20, 2024

lonitra added the 📭 waiting-author-feedback The team requires more information from the author label Apr 25, 2024

dotnet-policy-service bot added 💤 no-recent-activity and removed 💤 no-recent-activity labels May 9, 2024

lonitra added the 💤 no-recent-activity label May 14, 2024

dotnet-policy-service bot removed the 💤 no-recent-activity label May 14, 2024

lonitra added the 💤 no-recent-activity label May 14, 2024

dotnet-policy-service bot removed the 💤 no-recent-activity label May 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tests should use theory data #11171

Tests should use theory data #11171

woefulpines commented Apr 6, 2024 •

edited by dotnet-policy-service bot

woefulpines commented Apr 6, 2024

woefulpines commented Apr 6, 2024

codecov bot commented Apr 6, 2024 •

edited

RussKie Apr 9, 2024

lonitra left a comment

lonitra Apr 9, 2024

sharwell Apr 9, 2024

lonitra Apr 18, 2024

lonitra Apr 9, 2024

sharwell Apr 9, 2024

lonitra Apr 9, 2024

woefulpines Apr 18, 2024

lonitra Apr 18, 2024

sharwell Apr 18, 2024

woefulpines Apr 20, 2024

sharwell Apr 9, 2024 •

edited

lonitra commented Apr 25, 2024

	var testData = new TheoryData<OutputKind>();
	TheoryData<OutputKind> testData = new();

		[MemberData(nameof(TestConvertFormData))]
		#pragma warning restore xUnit1042 // The member referenced by the MemberData attribute returns untyped data rows

	#pragma warning restore xUnit1042 // The member referenced by the MemberData attribute returns untyped data rows
	#pragma warning restore xUnit1042

Tests should use theory data #11171

Are you sure you want to change the base?

Tests should use theory data #11171

Conversation

woefulpines commented Apr 6, 2024 • edited by dotnet-policy-service bot

Proposed changes

Customer Impact

Regression?

Risk

Test methodology

Test environment(s)

Microsoft Reviewers: Open in CodeFlow

woefulpines commented Apr 6, 2024

woefulpines commented Apr 6, 2024

codecov bot commented Apr 6, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

lonitra left a 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

sharwell Apr 9, 2024 • edited

Choose a reason for hiding this comment

lonitra commented Apr 25, 2024

woefulpines commented Apr 6, 2024 •

edited by dotnet-policy-service bot

codecov bot commented Apr 6, 2024 •

edited

sharwell Apr 9, 2024 •

edited