Add helper methods to Verify operation tree for annonated text within… #17856

mavasani · 2017-03-14T21:29:51Z

… a source file.

These APIs will be used in autogenerated unit test for Operation tree verification.

… a source file. These APIs will be used in autogenerated unit test for Operation tree verification.

CyrusNajmabadi · 2017-03-14T21:30:53Z

In general, can you tag me on these? Thanks!

jinujoseph · 2017-03-14T21:33:20Z

cc/ @CyrusNajmabadi @genlu

mavasani · 2017-03-14T21:36:16Z

Tagging @dotnet/roslyn-analysis @dotnet/roslyn-compiler for review.

mavasani · 2017-03-14T21:37:36Z

@jinujoseph Can you create a new GitHub group for all IOperation folks (4 of us in team and Cyrus and Gen)?

mavasani · 2017-03-14T21:50:48Z

@dotnet-bot retest ubuntu_14_debug_prtest please

mavasani · 2017-03-14T21:51:25Z

@dotnet-bot retest windows_release_vs-integration_prtest please

333fred · 2017-03-14T22:01:02Z

@dotnet/analyzer-ioperation is the new team name for this.

genlu · 2017-03-14T22:09:21Z

👍

heejaechang · 2017-03-14T22:13:04Z

src/Compilers/Test/Utilities/CSharp/SemanticModelTestBase.cs

+                return null;
+            }
+
+            var operation = model.GetOperationInternal(syntaxNode);


can a node have multiple operations?

Yes, and we need to test that SemanticModel.GetOperation does give us the expected bound node, lowest or highest. I see that VB and C# seem to have different defaults. C# picks the lowest bound node when there are multiple bound nodes for a syntax (see here) and VB picks the highest (see here). I will open a separate work item to track why this difference.

so when testing.. do we want to explicit on IOperation we get from the API? or we use the default behavior?

can user get IOperation off a node? or is it only internal, testing stuff?

That is a API discussion I believe. SemanticModel.GetOperation doesn't seem to take an options, but we have an internal enum GetOperationOptions that decides what bound node to choose for clashes. Until we expose that publically, unit tests should just use the default that the GetOperation API passes down.

Filed #17861

so for users, they expect 1 node to have 1 IOperation 1:1 matching?

hmm.. but when they examine SyntaxNode of IOperation, they might get multiple IOperation with same node right? how users get to those IOperations? only through walker?

Yes, seems we need to expose the options when there are conflict - needs design discussion. You can probably add comment to #17861

heejaechang · 2017-03-14T22:13:36Z

src/Compilers/Test/Utilities/CSharp/SemanticModelTestBase.cs

+        {
+            var tree = compilation.SyntaxTrees[0];
+            var model = compilation.GetSemanticModel(tree);
+            SyntaxNode syntaxNode = GetSyntaxNodeOfTypeForBinding<TSyntaxNode>(GetSyntaxNodeList(tree));


what happen if tree has multiple nodes matching given type?

We will behave similar to the compiler binding tests for those cases. Seems that this method adds the nodes from bottom up, and picks the first node that matches full text and is of expected type. Also I am not sure if we can have 2 nodes of same kind with same span in tree.

but this test doesn't give in span? so it will just pick the first node in the tree?

The unit test generator will add comments denoting the span, like following:

VB:

Dim x1 = New F() Dim x2 = New F() With {.Field = 2}'BIND:"Dim x2 = New F() With {.Field = 2}" Dim x3 = New F() With {.Property1 = ""} Dim x4 = New F() With {.Property1 = "", .Field = 2} Dim x5 = New F() With {.Property2 = New B() With {.Field = True}}

C#

class C { public void M1() /*<bind>*/{ var x1 = new F(); var x2 = new F() { Field = 2 }; var x3 = new F() { Property1 = """""""" }; var x4 = new F() { Property1 = """""""", Field = 2 }; var x5 = new F() { Property2 = new B { Field = true } }; var e1 = new F() { Property2 = 1 }; var e2 = new F() { """""""" }; }/*</bind>*/ }

bind comments specify the span that the test helper will use.

See example unit test: https://github.com/dotnet/roslyn-internal/pull/1692#issuecomment-286570489

ah, I see, no span argument is needed since it will get those from source, got it.

heejaechang · 2017-03-14T22:17:05Z

src/Compilers/Test/Utilities/VisualBasic/SemanticModelTestBase.vb

@@ -151,4 +152,32 @@ Public MustInherit Class SemanticModelTestBase : Inherits BasicTestBase
            ).Where(Function(s) anyArity OrElse DirectCast(s, Symbol).GetArity() = arity.Value).ToList()
    End Function

+    Friend Function GetOperationTreeForTest(Of TSyntaxNode As SyntaxNode)(compilation As VisualBasicCompilation, fileName As String, Optional which As Integer = 0) As String


interesting so compiler test framework between VB and C# is different slightly.

Yes, it seems there are differences between binding test framework helpers, and I had to retain them so I can re-use it.

heejaechang · 2017-03-14T22:18:52Z

src/Test/Utilities/Portable/Compilation/OperationTreeVerifier.cs

+            char[] newLineChars = Environment.NewLine.ToCharArray();
+            string actual = actualOperationTree.Trim(newLineChars);
+            expectedOperationTree = expectedOperationTree.Trim(newLineChars);
+            expectedOperationTree = Regex.Replace(expectedOperationTree, "([^\r])\n", "$1" + Environment.NewLine);


what does it actually trying to do?

This is due to the CDATA for VB. It seems to have \n characters instead of new lines, and this causes equality check to fail. This line replaces \n characters, that do not have a \r before it, with \r\n

hmm.. but you trimmed new lines right above, regex so, there will be no \r or \n in text anymore?

It will trim just \r\n from start and end. We need to retain the newline characters within the text so that assert failure displays the actual and expected IOperation tree in a nicely formatted manner in the test window.

I see. so escaped char is retained but actually "\r\n" will be removed. okay.

heejaechang · 2017-03-14T22:19:48Z

src/Test/Utilities/Portable/Extensions/SemanticModelExtensions.cs

+    {
+        public static IOperation GetOperationInternal(this SemanticModel model, SyntaxNode node)
+        {
+            return model.GetOperationInternal(node);


I am not sure what this extension method is trying to do.

is it calling itself?

I believe this is to bypass the feature flag check?

GetOperationInternal is internal method on semantic model that by-passes the IOperation feature flag check. Only core TestUtilties have IVT, so this just exposes it to other test layers, such as ETA.

ah, got it. some comment would help :) since it looks like it is calling itself ha ha.

heejaechang · 2017-03-14T22:48:31Z

👍

mavasani · 2017-03-14T22:49:04Z

@dotnet-bot test windows_debug_unit32_prtest please

mavasani · 2017-03-14T22:49:22Z

@dotnet test windows_release_vs-integration_prtest please

mavasani · 2017-03-14T22:54:29Z

windows_release_vs-integration_prtest is failing due to unrelated failure:

error MSB4018: The "FindInstalledExtension" task failed unexpectedly. [D:\j\workspace\windows_relea---8a39a417\src\Samples\VisualBasic\ConvertToAutoProperty\Impl\ConvertToAutoPropertyVB.vbproj]

mavasani · 2017-03-15T00:04:27Z

@dotnet-bot retest ubuntu_14_debug_prtest please

Add helper methods to Verify operation tree for annonated text within…

9d77278

… a source file. These APIs will be used in autogenerated unit test for Operation tree verification.

dnfclas added the cla-already-signed label Mar 14, 2017

heejaechang reviewed Mar 14, 2017

View reviewed changes

Address PR feedback and add comment

cb01f0b

jinujoseph approved these changes Mar 14, 2017

View reviewed changes

mavasani mentioned this pull request Mar 15, 2017

[Flaky Test] Microsoft.CodeAnalysis.CSharp.Scripting.UnitTests.InteractiveSessionTests.CompilationChain_NestedTypesClass #16907

Open

Don't assert for OperationKind.None in OperationTreeVerifier

715e6d7

mavasani merged commit c950069 into dotnet:master Mar 15, 2017

mavasani deleted the IOperationUnitTestGenerator branch March 15, 2017 00:54

Add helper methods to Verify operation tree for annonated text within… #17856

Add helper methods to Verify operation tree for annonated text within… #17856

Conversation

mavasani commented Mar 14, 2017

CyrusNajmabadi commented Mar 14, 2017

jinujoseph commented Mar 14, 2017

mavasani commented Mar 14, 2017

mavasani commented Mar 14, 2017

mavasani commented Mar 14, 2017

mavasani commented Mar 14, 2017

333fred commented Mar 14, 2017

genlu commented Mar 14, 2017

Choose a reason for hiding this comment

mavasani Mar 14, 2017 • 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

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

mavasani Mar 14, 2017 • 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

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

heejaechang commented Mar 14, 2017

mavasani commented Mar 14, 2017

mavasani commented Mar 14, 2017

mavasani commented Mar 14, 2017

mavasani commented Mar 15, 2017

mavasani Mar 14, 2017 •

edited

Loading

mavasani Mar 14, 2017 •

edited

Loading