[features/interpolated-constant-strings] Adding Tests#46828
Conversation
6bd4fc6 to
26a4227
Compare
26a4227 to
8333853
Compare
src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelAPITests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelAPITests.cs
Outdated
Show resolved
Hide resolved
|
@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. |
|
Done review pass (commit 3) |
| } | ||
|
|
||
| var expected = new[] { true, false }; | ||
| var actual = tree.GetRoot().DescendantNodes().OfType<InterpolatedStringExpressionSyntax>().ToArray(); |
There was a problem hiding this comment.
I would just remove this array and inline the constants. Assert.True and Assert.False.
|
@kevinsun-dev it looks like you have some trailing whitespace in the string constant for |
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
Show resolved
Hide resolved
| [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() |
There was a problem hiding this comment.
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}""|])
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yup, added a test for that. A int constant should register as invalid and cause us to suggest a variable instead.
There was a problem hiding this comment.
I think Cyrus wanted a test for when the interpolation includes things that would still make it a constant value.
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
IDE side LGTM with the addition of a test. thanks!
| void M(string S1 = $""Testing"") | ||
| const string S0 = $""Faaaaaaaaaaaaaaaaaaaaaaaaall""; | ||
|
|
||
| class Namae |
There was a problem hiding this comment.
Namae [](start = 10, length = 5)
nit: That's a weird namae ;-)
There was a problem hiding this comment.
nit: That's a weird 名前 ;-)
Indeed it is.
| } | ||
| } | ||
| }"; | ||
| var comp = CreateCompilation(source, parseOptions: TestOptions.RegularPreview); |
There was a problem hiding this comment.
nit: usage of Preview should be replaced with CSharp9 at some point
|
|
||
| var actual = tree.GetRoot().DescendantNodes().OfType<InterpolatedStringExpressionSyntax>().ToArray(); | ||
| Assert.True(model.GetConstantValue(actual[0]).HasValue); | ||
| Assert.Equal("Hello, world!", model.GetConstantValue(actual[0]).Value); |
There was a problem hiding this comment.
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.
Test Plan: #46780
Added Tests: