Update 'UseExpressionBody' tests to the new test library by Youssef1313 · Pull Request #51623 · dotnet/roslyn

Youssef1313 · 2021-03-03T14:48:25Z

No description provided.

…brary

…rary

Youssef1313 · 2021-03-03T16:09:35Z

Converting to a draft until I fix test failures. Meanwhile, I'd like to get an initial feedback on this and whether you want to do this.

src/Analyzers/CSharp/Tests/UseExpressionBody/UseExpressionBodyForPropertiesAnalyzerTests.cs

sharwell · 2021-03-03T17:22:35Z

src/Analyzers/CSharp/Tests/UseExpressionBody/UseExpressionBodyForMethodsAnalyzerTests.cs

+        UseExpressionBodyDiagnosticAnalyzer,
+        UseExpressionBodyCodeFixProvider>;


📝 It may be possible to remove the MEF suppressions on the constructor in each of these types

sharwell · 2021-03-03T17:24:00Z

@Youssef1313 Definitely the direction we want to take the tests. I recommend converting to the syntax of the new tests first (which is exactly what you did here so far), and then make any corrections to the test (compiler errors, etc.) as separate commits in the PR so we can review those changes separately from the simple API transformation.

…t library

…w test library

…library

Youssef1313 · 2021-03-04T13:59:32Z

@sharwell Should we get UseFirstDescriptor removed (in a later PR)?

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

Lines 43 to 46 in 6da1274

    
           public Test() 
        
           { 
        
               MarkupOptions = Testing.MarkupOptions.UseFirstDescriptor;

Youssef1313 · 2021-03-04T15:01:56Z

[xUnit.net 00:00:11.98]     Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.UseExpressionBody.UseExpressionBodyForPropertiesAnalyzerTests.TestUseBlockBodyForAccessorEventWhenAccessorWantExpress
ion1 [FAIL]
  Failed Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.UseExpressionBody.UseExpressionBodyForPropertiesAnalyzerTests.TestUseBlockBodyForAccessorEventWhenAccessorWantExpression1 [31 ms]
  Error Message:
   Microsoft.CodeAnalysis.Testing.Verifiers.EqualWithMessageException : Context: Diagnostics of fixed state
Mismatch between number of diagnostics returned, expected "0" actual "1"

Diagnostics:
// /0/Test0.cs(9,9): hidden IDE0027: Use expression body for accessors
VerifyCS.Diagnostic(UseExpressionBodyDiagnosticAnalyzer.IDE0027).WithSpan(9, 9, 12, 10).WithSpan(9, 9, 12, 10),

@sharwell I'm not sure why it expects "0" for fixed state? Here is the test:

        [WorkItem(20363, "https://github.com/dotnet/roslyn/issues/20363")]
        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseExpressionBody)]
        public async Task TestUseBlockBodyForAccessorEventWhenAccessorWantExpression1()
        {
            var code = @"
class C
{
    int Bar() { return 0; }

    {|IDE0025:int Goo => Bar();|}
}";
            var fixedCode = @"
class C
{
    int Bar() { return 0; }

    int Goo
    {
        // TODO: Likely a bug? It should have been expression body.
        {|IDE0027:get
        {
            return Bar();
        }|}
    }
}";
            await TestWithUseBlockBodyExceptAccessor(code, fixedCode);
        }

        private static async Task TestWithUseBlockBodyExceptAccessor(string code, string fixedCode)
        {
            await new VerifyCS.Test
            {
                TestCode = code,
                FixedCode = fixedCode,
                Options =
                {
                    { CSharpCodeStyleOptions.PreferExpressionBodiedProperties, ExpressionBodyPreference.Never },
                    { CSharpCodeStyleOptions.PreferExpressionBodiedAccessors, ExpressionBodyPreference.WhenPossible },
                },
                MarkupOptions = MarkupOptions.None,
            }.RunAsync();
        }

sharwell · 2021-03-04T15:08:16Z

Should we get UseFirstDescriptor removed (in a later PR)?

It wasn't viable prior to #46666. Now it probably is, but I'm also not sure it's hurting anything.

I'm not sure why it expects "0" for fixed state?

You need to set FixedState.MarkupMode to MarkupMode.Allow. The property and all the enumeration values have a bunch of documentation, so read through them and let me know if you still have questions.

src/Analyzers/CSharp/Tests/UseExpressionBody/UseExpressionBodyForPropertiesAnalyzerTests.cs

Youssef1313 · 2021-03-04T19:19:23Z

roslyn/src/Analyzers/CSharp/Tests/UseExpressionBody/UseExpressionBodyForAccessorsAnalyzerTests.cs

Lines 319 to 337 in 6da1274

    
                   public async Task TestUseBlockBody5() 
        
                   { 
        
                       var whenOnSingleLineWithNoneEnforcement = new CodeStyleOption2<ExpressionBodyPreference>(ExpressionBodyPreference.WhenOnSingleLine, NotificationOption2.None); 
        
                       var options = new OptionsCollection(GetLanguage()) 
        
                       { 
        
                           { CSharpCodeStyleOptions.PreferExpressionBodiedAccessors, whenOnSingleLineWithNoneEnforcement }, 
        
                           { CSharpCodeStyleOptions.PreferExpressionBodiedProperties, whenOnSingleLineWithNoneEnforcement }, 
        
                           { CSharpCodeStyleOptions.PreferExpressionBodiedIndexers, whenOnSingleLineWithNoneEnforcement }, 
        
                       }; 
        
                       await TestMissingInRegularAndScriptAsync( 
        
           @"class C 
        
           { 
        
               C this[int index] 
        
               { 
        
                   get [|=>|] default; 
        
               } 
        
           }", new TestParameters(options: options)); 
        
                   }

@sharwell Does "WithNoneEnforcement" in the original test the same as "Never"?

The test is failing with "ExpressionBodyPreference.WhenOnSingleLine" as it expects expression body for indexer, while the original test doesn't.

Youssef1313 · 2021-03-04T19:32:29Z

There is also TestAccessorListFormatting_FixAll which is failing locally:

        public async Task TestAccessorListFormatting_FixAll()
        {
            var code = @"
class C
{
    int Bar() { return 0; }

    int Goo { {|IDE0027:get => Bar();|} {|IDE0027:set => Bar();|} }
}";
            var fixedCode = @"
class C
{
    int Bar() { return 0; }

    int Goo
    {
        get
        {
            return Bar();
        }

        set
        {
            Bar();
        }
    }
}";
            await TestWithUseBlockBodyIncludingPropertiesAndIndexers(code, fixedCode);
        }

content of '/0/Test0.cs' did not match. Diff shown with expected as baseline:

 class C
 {
     int Bar() { return 0; }

     int Goo
     {
-        get
+        get { return Bar(); }
-        {
-            return Bar();
-        }
-
         set
         {
             Bar();
         }
     }
 }

I can't figure out how it's passing in main. The original test is:

        public async Task TestAccessorListFormatting_FixAll()
        {
            await TestInRegularAndScriptAsync(
@"class C
{
    int Goo { get {|FixAllInDocument:=>|} Bar(); set => Bar(); }
}",
@"class C
{
    int Goo
    {
        get
        {
            return Bar();
        }
        set
        {
            Bar();
        }
    }
}", options: UseBlockBodyIncludingPropertiesAndIndexers);
        }

sharwell · 2021-03-04T21:28:36Z

Does "WithNoneEnforcement" in the original test the same as "Never"?

No, you'll need to also pass the third parameter as NotifactionOption2.None.

Youssef1313 · 2021-03-05T07:13:07Z

@sharwell That worked. The only failing test now is the one I stated in #51623 (comment)

I couldn't figure out what's wrong. It's failing with the following diff.

 class C
 {
     int Bar() { return 0; }

     int Goo
     {
-        get { return Bar(); }
+        get
+        {
+            return Bar();
+        }
+
         set
         {
             Bar();
         }
     }
 }

However, when I update it, it fails with the exact opposite diff.

 class C
 {
     int Bar() { return 0; }

     int Goo
     {
-        get
+        get { return Bar(); }
-        {
-            return Bar();
-        }
-
         set
         {
             Bar();
         }
     }
 }

Do you know what's going on? I've tried to set NumberOfIncrementalIterations and NumberOfFixAllIterations but no luck.

sharwell · 2021-03-05T19:20:22Z

I'm guessing the iterative code fix and the Fix All are producing different results. You can specify both by setting FixedCode for the iterative result and BatchFixedCode for the Fix All result.

Youssef1313 · 2021-03-05T20:38:32Z

Thanks @sharwell. I think CI should pass now.

CyrusNajmabadi · 2021-03-09T07:30:56Z

Thanks!

Update UseExpressionBodyForPropertiesAnalyzerTests to the new test li…

d9a7fd4

…brary

ghost added the Area-Analyzers label Mar 3, 2021

Youssef1313 added 2 commits March 3, 2021 17:01

Update UseExpressionBodyForOperatorsAnalyzerTests to the new test lib…

0fe0438

…rary

Update UseExpressionBodyForMethodsAnalyzerTests to the new test library

e4c615f

Youssef1313 changed the title ~~Update UseExpressionBodyForPropertiesAnalyzerTests to the new test library~~ Update some 'UseExpressionBody' tests to the new test library Mar 3, 2021

Youssef1313 marked this pull request as draft March 3, 2021 16:09

sharwell reviewed Mar 3, 2021

View reviewed changes

src/Analyzers/CSharp/Tests/UseExpressionBody/UseExpressionBodyForPropertiesAnalyzerTests.cs Outdated Show resolved Hide resolved

sharwell approved these changes Mar 3, 2021

View reviewed changes

sharwell reviewed Mar 3, 2021

View reviewed changes

Base automatically changed from master to main March 3, 2021 23:53

Youssef1313 added 7 commits March 4, 2021 13:19

Update UseExpressionBodyForLocalFunctionsAnalyzerTests to the new tes…

cb37d12

…t library

Update UseExpressionBodyForIndexersAnalyzerTests to the new test library

119b075

Update UseExpressionBodyForConversionOperatorsAnalyzerTests to the ne…

7fafaf7

…w test library

Update UseExpressionBodyForConstructorsAnalyzerTests to the new test …

72f8aab

…library

Update UseExpressionBodyForAccessorsTests to the new test library

da746e0

Address feedback: Use ExpressionBodyPreference

5137c5d

Remove suppression: RS0033:Importing constructor should be obsolete

bbe74b5

Youssef1313 added 2 commits March 4, 2021 17:27

Fix UseExpressionBodyForPropertiesAnalyzerTests

d9af8d4

Fix UseExpressionBodyForOperatorsAnalyzerTests

a394069

sharwell reviewed Mar 4, 2021

View reviewed changes

src/Analyzers/CSharp/Tests/UseExpressionBody/UseExpressionBodyForPropertiesAnalyzerTests.cs Show resolved Hide resolved

Youssef1313 added 3 commits March 4, 2021 18:45

Fix UseExpressionBodyForMethodsAnalyzerTests

5a6391b

Fix UseExpressionBodyForLocalFunctionsAnalyzerTests

0a24e10

Fix UseExpressionBodyForIndexersAnalyzerTests

9dd8fba

Youssef1313 changed the title ~~Update some 'UseExpressionBody' tests to the new test library~~ Update 'UseExpressionBody' tests to the new test library Mar 4, 2021

Fix UseExpressionBodyForConversionOperatorsAnalyzerTests

c722ff8

Fix UseExpressionBodyForConstructorsAnalyzerTests

4695116

Youssef1313 marked this pull request as ready for review March 4, 2021 19:33

Fix UseExpressionBodyForAccessorsTests

9894b96

CyrusNajmabadi merged commit b0f9a5a into dotnet:main Mar 9, 2021

Youssef1313 deleted the update-tests branch March 9, 2021 14:41

allisonchou added this to the Next milestone Mar 12, 2021

allisonchou modified the milestones: Next, 16.10.P2 Mar 29, 2021

		UseExpressionBodyDiagnosticAnalyzer,
		UseExpressionBodyCodeFixProvider>;

Conversation

Youssef1313 commented Mar 3, 2021

Uh oh!

Youssef1313 commented Mar 3, 2021

Uh oh!

Uh oh!

sharwell Mar 3, 2021

Choose a reason for hiding this comment

Uh oh!

sharwell commented Mar 3, 2021

Uh oh!

Youssef1313 commented Mar 4, 2021

Uh oh!

Youssef1313 commented Mar 4, 2021

Uh oh!

sharwell commented Mar 4, 2021

Uh oh!

Uh oh!

Youssef1313 commented Mar 4, 2021

Uh oh!

Youssef1313 commented Mar 4, 2021

Uh oh!

sharwell commented Mar 4, 2021

Uh oh!

Youssef1313 commented Mar 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sharwell commented Mar 5, 2021

Uh oh!

Youssef1313 commented Mar 5, 2021

Uh oh!

CyrusNajmabadi commented Mar 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Youssef1313 commented Mar 5, 2021 •

edited

Loading