Convert RemoveUnusedMembersTests to the new test framework by sharwell · Pull Request #41755 · dotnet/roslyn

sharwell · 2020-02-18T19:47:42Z

No description provided.

mavasani · 2020-02-18T22:47:55Z

src/EditorFeatures/CSharpTest/RemoveUnusedMembers/RemoveUnusedMembersTests.cs

    {
        _field!.ToString();
    }
-}", new TestParameters(retainNonFixableDiagnostics: true, parseOptions: new CSharpParseOptions(LanguageVersion.CSharp8)));


Does the test framework default to latest language version?

➡️ The test framework defaults to LanguageVersion.Default, but we use this instead:

roslyn/src/EditorFeatures/TestUtilities/CodeActions/CSharpCodeFixVerifier`2+Test.cs

Lines 70 to 74 in 5d8e2be

/// <summary>

/// Gets or sets the language version to use for the test. The default value is

/// <see cref="LanguageVersion.CSharp8"/>.

/// </summary>

public LanguageVersion LanguageVersion { get; set; } = LanguageVersion.CSharp8;

src/EditorFeatures/CSharpTest/RemoveUnusedMembers/RemoveUnusedMembersTests.cs

mavasani · 2020-02-18T22:55:39Z

src/EditorFeatures/CSharpTest/RemoveUnusedMembers/RemoveUnusedMembersTests.cs

+                },
+                FixedCode = source,
+            }.RunAsync();
+            //var testParameters = new TestParameters(retainNonFixableDiagnostics: true);


Is message validation an unsupported scenario? Any case, this test needs to be fixed to remove the commented out code, and if required skipped with a tracking issue to unskip.

Is message validation an unsupported scenario?

Messages are always validated for diagnostics in ExpectedDiagnostics, but ignored for diagnostics in markup.

This test was not converted in the intended manner. I wrote the full-form skeleton, but left ExpectedDiagnostics empty intending to replace it with values found in an actual test failure. Unfortunately, the use of markup ended up causing the test to pass and I never went back to fill in the details. I will fix this.

➡️ Fixed in cfbdcec

src/EditorFeatures/CSharpTest/RemoveUnusedMembers/RemoveUnusedMembersTests.cs

mavasani · 2020-02-18T22:58:54Z

src/EditorFeatures/CSharpTest/RemoveUnusedMembers/RemoveUnusedMembersTests.cs

+            await VerifyCS.VerifyCodeFixAsync(
 @"class MyClass
 {
-    private int [|_goo|] = 0, _bar = 0;


Interesting, so we cannot force the validation to a specific span within the document?

No, and that's actually a desirable characteristic of the framework. The feature I have considered adding is the ability to run just one specific fix, as opposed to the iterative process used by default.

Sounds good

src/EditorFeatures/CSharpTest/RemoveUnusedMembers/RemoveUnusedMembersTests.cs

sharwell · 2020-02-19T04:52:04Z

src/EditorFeatures/CSharpTest/RemoveUnusedMembers/RemoveUnusedMembersTests.cs

-    string? [|_field|] = null;
-    void M()
+    string? _field = null;
+    public void M()


📝 A bunch of members were changed to public to avoid having them get marked as unused, which ends up cascading to everything in the class getting removed.

sharwell · 2020-02-19T04:52:44Z

src/EditorFeatures/CSharpTest/RemoveUnusedMembers/RemoveUnusedMembersTests.cs

+        private async Task TestDiagnosticMissingAsync(string initialMarkup)
+            => await VerifyCS.VerifyCodeFixAsync(initialMarkup, initialMarkup);


💡 This should really be removed to follow the patterns from other types.

➡️ Fixed in 92fc348

src/EditorFeatures/CSharpTest/RemoveUnusedMembers/RemoveUnusedMembersTests.cs

sharwell · 2020-02-19T04:57:52Z

src/EditorFeatures/CSharpTest/RemoveUnusedMembers/RemoveUnusedMembersTests.cs

+                },
+                FixedCode = source,
+            }.RunAsync();
+            //var testParameters = new TestParameters(retainNonFixableDiagnostics: true);


This test was not converted in the intended manner. I wrote the full-form skeleton, but left ExpectedDiagnostics empty intending to replace it with values found in an actual test failure. Unfortunately, the use of markup ended up causing the test to pass and I never went back to fill in the details. I will fix this.

sharwell added 2 commits February 18, 2020 11:28

Convert RemoveUnusedMembersTests to the new test framework

83478cb

Fix compilation errors in tests

193ed9c

sharwell requested a review from a team as a code owner February 18, 2020 19:47

mavasani reviewed Feb 18, 2020

View reviewed changes

src/EditorFeatures/CSharpTest/RemoveUnusedMembers/RemoveUnusedMembersTests.cs Show resolved Hide resolved

mavasani reviewed Feb 18, 2020

View reviewed changes

src/EditorFeatures/CSharpTest/RemoveUnusedMembers/RemoveUnusedMembersTests.cs Show resolved Hide resolved

mavasani reviewed Feb 18, 2020

View reviewed changes

src/EditorFeatures/CSharpTest/RemoveUnusedMembers/RemoveUnusedMembersTests.cs Outdated Show resolved Hide resolved

mavasani reviewed Feb 18, 2020

View reviewed changes

src/EditorFeatures/CSharpTest/RemoveUnusedMembers/RemoveUnusedMembersTests.cs Outdated Show resolved Hide resolved

mavasani reviewed Feb 18, 2020

View reviewed changes

src/EditorFeatures/CSharpTest/RemoveUnusedMembers/RemoveUnusedMembersTests.cs Outdated Show resolved Hide resolved

mavasani reviewed Feb 18, 2020

View reviewed changes

src/EditorFeatures/CSharpTest/RemoveUnusedMembers/RemoveUnusedMembersTests.cs Show resolved Hide resolved

sharwell commented Feb 19, 2020

View reviewed changes

sharwell added 2 commits February 18, 2020 21:22

Inline TestDiagnosticMissingAsync

92fc348

Fix test for expected diagnostic message

cfbdcec

mavasani approved these changes Feb 19, 2020

View reviewed changes

sharwell merged commit 5478577 into dotnet:master Feb 19, 2020

sharwell deleted the unused-members branch February 19, 2020 17:14

sharwell added this to the 16.6.P1 milestone Feb 19, 2020

sharwell mentioned this pull request Feb 26, 2020

Update analyzer test library for "fix one" support #41953

Merged

	/// <summary>
	/// Gets or sets the language version to use for the test. The default value is
	/// <see cref="LanguageVersion.CSharp8"/>.
	/// </summary>
	public LanguageVersion LanguageVersion { get; set; } = LanguageVersion.CSharp8;

		private async Task TestDiagnosticMissingAsync(string initialMarkup)
		=> await VerifyCS.VerifyCodeFixAsync(initialMarkup, initialMarkup);

Conversation

sharwell commented Feb 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell Feb 19, 2020 • 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!

Uh oh!

Uh oh!

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!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sharwell Feb 19, 2020 •

edited

Loading