[features/interpolated-constant-strings] Adding Tests #46828

kevinsun-dev · 2020-08-14T16:52:42Z

Test Plan: #46780

Added Tests:

Usage in constant field initializers
Usage in nested patterns

src/Compilers/CSharp/Test/Semantic/Semantics/ConstantTests.cs

src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelAPITests.cs

333fred · 2020-08-14T21:34:05Z

@CyrusNajmabadi can you take a look at the introduce variable changes? I intend on updating the refactoring sometime next week to understand C# 9, so it's currently unconditional.

333fred · 2020-08-14T21:37:25Z

Done review pass (commit 3)

333fred · 2020-08-17T18:05:43Z

src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelAPITests.cs

-            }
+
+            var expected = new[] { true, false };
+            var actual = tree.GetRoot().DescendantNodes().OfType<InterpolatedStringExpressionSyntax>().ToArray();


I would just remove this array and inline the constants. Assert.True and Assert.False.

333fred · 2020-08-17T18:07:48Z

@kevinsun-dev it looks like you have some trailing whitespace in the string constant for IntroduceVariableTests.TestNoConstantForInterpolatedStrings, that's causing the test failure.

333fred · 2020-08-17T18:08:25Z

 using System;
 class TestClass
 {
     static void Test(string[] args)
     {
-        var value = $"{DateTime.Now.ToString()}Text{args[0]}";   //<- whitespace here
+        var value = $"{DateTime.Now.ToString()}Text{args[0]}";
         Console.WriteLine(value);
     }
 }

src/EditorFeatures/CSharpTest/CodeActions/IntroduceVariable/IntroduceVariableTests.cs

CyrusNajmabadi · 2020-08-17T18:13:14Z

src/EditorFeatures/CSharpTest/CodeActions/IntroduceVariable/IntroduceVariableTests.cs

-        [Fact(Skip = "PROTOTYPE(CONSTISTR) - Expected changed var into a private const."), Trait(Traits.Feature, Traits.Features.CodeActionsIntroduceVariable)]
-        public async Task TestNoConstantForInterpolatedStrings2()
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsIntroduceVariable)]
+        public async Task TestConstantForInterpolatedStrings()


~~could you add a test where the interpolated string references a real constant value? i.e. 0 instead of just the unbound s? thanks!~~

could you add a test where there is an actual interpolation and the interpolation contains a constant? i.e. ([|$""Text{0}""|])

Note: this would have to be ([|$""Text{""0""}""|]), as all constant values inside must be constant string expressions or it's not a constant string itself.

Yup, added a test for that. A int constant should register as invalid and cause us to suggest a variable instead.

I think Cyrus wanted a test for when the interpolation includes things that would still make it a constant value.

CyrusNajmabadi

IDE side LGTM with the addition of a test. thanks!

jcouv · 2020-08-17T19:54:04Z

src/Compilers/CSharp/Test/Semantic/Semantics/ConstantTests.cs

-    void M(string S1 = $""Testing"")
+    const string S0 = $""Faaaaaaaaaaaaaaaaaaaaaaaaall"";
+
+    class Namae


Namae [](start = 10, length = 5)

nit: That's a weird namae ;-)

nit: That's a weird 名前 ;-)

Indeed it is.

jcouv · 2020-08-17T19:55:34Z

src/Compilers/CSharp/Test/Semantic/Semantics/ConstantTests.cs

+                    break;
+                case $""Radio Noise"":
+                    goto case $""Number 3"";
+            }
        }
    }
 }";
            var comp = CreateCompilation(source, parseOptions: TestOptions.RegularPreview);


nit: usage of Preview should be replaced with CSharp9 at some point

jcouv · 2020-08-17T19:58:09Z

src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelAPITests.cs

+
+            var actual = tree.GetRoot().DescendantNodes().OfType<InterpolatedStringExpressionSyntax>().ToArray();
+            Assert.True(model.GetConstantValue(actual[0]).HasValue);
+            Assert.Equal("Hello, world!", model.GetConstantValue(actual[0]).Value);


I don't know if this was covered elsewhere or is planned for later, but more cases should be covered with GetConstantValue on constant interpolated strings.

Sounds good.

jcouv

LGTM Thanks (iteration 6)

jcouv

LGTM Thanks (iteration 7)

Kevin Sun added 2 commits August 14, 2020 14:12

Update IntroduceVariableTests.cs

1b3f196

Update ConstantTests.cs

4e2f8fe

kevinsun-dev force-pushed the features/interpolated-string-constants branch from 6bd4fc6 to 26a4227 Compare August 14, 2020 21:16

Update SemanticModelAPITests.cs

8333853

kevinsun-dev force-pushed the features/interpolated-string-constants branch from 26a4227 to 8333853 Compare August 14, 2020 21:17

333fred reviewed Aug 14, 2020

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/ConstantTests.cs Outdated Show resolved Hide resolved

333fred reviewed Aug 14, 2020

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/ConstantTests.cs Show resolved Hide resolved

333fred reviewed Aug 14, 2020

View reviewed changes

src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelAPITests.cs Outdated Show resolved Hide resolved

333fred reviewed Aug 14, 2020

View reviewed changes

src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelAPITests.cs Outdated Show resolved Hide resolved

333fred requested a review from CyrusNajmabadi August 14, 2020 21:34

PR changes

419ac8a

kevinsun-dev marked this pull request as ready for review August 17, 2020 16:12

kevinsun-dev requested review from a team as code owners August 17, 2020 16:12

333fred reviewed Aug 17, 2020

View reviewed changes

jcouv added the Area-Compilers label Aug 17, 2020

CyrusNajmabadi reviewed Aug 17, 2020

View reviewed changes

src/EditorFeatures/CSharpTest/CodeActions/IntroduceVariable/IntroduceVariableTests.cs Show resolved Hide resolved

CyrusNajmabadi reviewed Aug 17, 2020

View reviewed changes

CyrusNajmabadi approved these changes Aug 17, 2020

View reviewed changes

PR changes

54ce24c

333fred approved these changes Aug 17, 2020

View reviewed changes

Update IntroduceVariableTests.cs

43b2c9c

jcouv reviewed Aug 17, 2020

View reviewed changes

jcouv approved these changes Aug 17, 2020

View reviewed changes

jcouv self-assigned this Aug 17, 2020

Update SemanticModelAPITests.cs

219f155

333fred approved these changes Aug 19, 2020

View reviewed changes

jcouv approved these changes Aug 19, 2020

View reviewed changes

kevinsun-dev merged commit b2e7a73 into dotnet:features/interpolated-string-constants Aug 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[features/interpolated-constant-strings] Adding Tests #46828

[features/interpolated-constant-strings] Adding Tests #46828

kevinsun-dev commented Aug 14, 2020

333fred commented Aug 14, 2020 •

edited

Loading

333fred commented Aug 14, 2020

333fred Aug 17, 2020

333fred commented Aug 17, 2020

333fred commented Aug 17, 2020 •

edited

Loading

CyrusNajmabadi Aug 17, 2020 •

edited

Loading

333fred Aug 17, 2020

kevinsun-dev Aug 17, 2020

333fred Aug 17, 2020

CyrusNajmabadi left a comment

jcouv Aug 17, 2020

kevinsun-dev Aug 18, 2020

jcouv Aug 17, 2020

jcouv Aug 17, 2020

kevinsun-dev Aug 18, 2020

jcouv left a comment

jcouv left a comment

[features/interpolated-constant-strings] Adding Tests #46828

[features/interpolated-constant-strings] Adding Tests #46828

Conversation

kevinsun-dev commented Aug 14, 2020

333fred commented Aug 14, 2020 • edited Loading

333fred commented Aug 14, 2020

Choose a reason for hiding this comment

333fred commented Aug 17, 2020

333fred commented Aug 17, 2020 • edited Loading

CyrusNajmabadi Aug 17, 2020 • edited Loading

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

CyrusNajmabadi 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

jcouv left a comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

333fred commented Aug 14, 2020 •

edited

Loading

333fred commented Aug 17, 2020 •

edited

Loading

CyrusNajmabadi Aug 17, 2020 •

edited

Loading