Adding Unit tests for IfStatement #18060

jinujoseph · 2017-03-22T17:40:07Z

Fixes #17601 This adds couple of unit tests for Ifstatements

On the VB side i don't get Ioperation for IfStatment, There seems to be an ioperation for SingleLineIfStatement and MultiLineIfBlock.

Issues discovered
#18300
#18303

Tagging @dotnet/analyzer-ioperation @AlekseyTs for review.

mavasani · 2017-03-22T17:43:23Z

src/Compilers/CSharp/Test/Semantic/IOperation/IOperationTests_IIfStatement.cs

+    private void M()
+    /*<bind>*/{
+        bool condition=false;
+        if (true)


You can probably make the test more targeted by just binding the if statement instead of entire method body.

mavasani · 2017-03-22T17:48:39Z

src/Compilers/CSharp/Test/Semantic/IOperation/IOperationTests_IIfStatement.cs

+      IInvocationExpression (static void System.Console.WriteLine(System.String value)) (OperationKind.InvocationExpression, Type: System.Void)
+        IArgument (Matching Parameter: value) (OperationKind.Argument)
+          ILiteralExpression (OperationKind.LiteralExpression, Type: System.String, Constant: Result1)
+  IIfStatement (OperationKind.IfStatement)


Seems to be the case where if... elseif.. else has been converted into if... else (if... else) due to lowering.

Does IOperation allow to represent this in a different form? #Closed

mavasani · 2017-03-22T17:55:34Z

src/Compilers/VisualBasic/Test/Semantic/IOperation/IOperationTests_IIfStatement.vb

+    Partial Public Class IOperationTests
+        Inherits SemanticModelTestBase
+
+        <Fact(), WorkItem(17601, "https://github.com/dotnet/roslyn/issues/17601")>


Can we also port the C# tests where you have a constant expression for the if condition?

yes , will do once we figure out the mutiblock issue

mavasani · 2017-03-22T17:57:17Z

src/Compilers/VisualBasic/Test/Semantic/IOperation/IOperationTests_IIfStatement.vb

+
+            VerifyOperationTreeForTest(Of SingleLineIfStatementSyntax)(source, expectedOperationTree)
+        End Sub
+


Please port the test for ElseIf from C# to VB.

As a general guideline, it is recommended that you try to port all the C# tests to VB or vice versa for equivalent features.

AlekseyTs · 2017-03-22T18:56:35Z

src/Compilers/CSharp/Test/Semantic/IOperation/IOperationTests_IIfStatement.cs

+
+namespace Microsoft.CodeAnalysis.CSharp.UnitTests
+{
+    public partial class IOperationTests : SemanticModelTestBase


IOperationTests [](start = 25, length = 15)

Consider also testing scenarios:

with embedded statements that are not blocks;

with conditions declaring variables (out or pattern);

with embedded statements declaring variables (out or pattern). #Closed

mavasani

@jinujoseph I still don't see the unit test with constant if condition ported over. Apart from that, LGTM.

jinujoseph · 2017-03-28T06:38:50Z

thanks manish, i added few constant checks

heejaechang · 2017-03-28T19:17:06Z

src/Compilers/CSharp/Test/Semantic/IOperation/IOperationTests_IIfStatement.cs

+    private void M()
+    {
+        bool condition=false;
+        /*<bind>*/if (true)


can you test whether binding all nodes belong to if statement works? like condition, body and etc.

heejaechang · 2017-03-28T19:20:40Z

src/Compilers/CSharp/Test/Semantic/IOperation/IOperationTests_IIfStatement.cs

+";
+            string expectedOperationTree = @"
+IIfStatement (OperationKind.IfStatement)
+  Condition: IOperation:  (OperationKind.None)


not supported C# 7 feature yet I guess?

yes , we have a workitem tracking that

heejaechang · 2017-03-28T19:22:43Z

seems fine to me, but @mavasani is more expert on language than me so, you probably want him to take a look.

jinujoseph · 2017-03-28T19:25:26Z

@AlekseyTs tagging you to relook this.

AlekseyTs · 2017-03-29T21:57:32Z

src/Compilers/CSharp/Test/Semantic/IOperation/IOperationTests_IIfStatement.cs

+  IExpressionStatement (OperationKind.ExpressionStatement)
+    IInvocationExpression (static void System.Console.WriteLine(System.String value)) (OperationKind.InvocationExpression, Type: System.Void)
+      IArgument (Matching Parameter: value) (OperationKind.Argument)
+        IOperation:  (OperationKind.None)


IOperation: (OperationKind.None) [](start = 8, length = 33)

It looks like string interpolations are not supported, please create an issue to track that, unless we already have one. #Closed

filed issue #18300

AlekseyTs · 2017-03-29T21:58:48Z

src/Compilers/CSharp/Test/Semantic/IOperation/IOperationTests_IIfStatement.cs

+      IArgument (Matching Parameter: s) (OperationKind.Argument)
+        ILocalReferenceExpression: s (OperationKind.LocalReferenceExpression, Type: System.String)
+      IArgument (Matching Parameter: result) (OperationKind.Argument)
+        ILocalReferenceExpression: i (OperationKind.LocalReferenceExpression, Type: System.Int32)


ILocalReferenceExpression: i (OperationKind.LocalReferenceExpression, Type: System.Int32) [](start = 8, length = 89)

Design issue - should we have a special node for this declaration? #Closed

filed issue #18303

AlekseyTs · 2017-03-29T22:02:08Z

src/Compilers/CSharp/Test/Semantic/IOperation/IOperationTests_IIfStatement.cs

+    private void M()
+    {
+        /*<bind>*/if (true)
+            A();


A(); [](start = 12, length = 4)

Please add a test where embedded statements declare out or pattern variables. #Closed

AlekseyTs · 2017-03-29T22:05:18Z

src/Compilers/VisualBasic/Test/Semantic/IOperation/IOperationTests_IIfStatement.vb

+            returnValue = count
+        End If"
+            returnValue = count
+        End If


Is this code intentional? #Closed

AlekseyTs · 2017-03-29T22:09:50Z

src/Compilers/VisualBasic/Test/Semantic/IOperation/IOperationTests_IIfStatement.vb

+        Dim n As Integer = 7
+        If (m > 20) Then'BIND:"If (m > 20) Then"
+            Console.WriteLine("Result1")
+        ElseIf (n > 10) Then


ElseIf (n > 10) Then [](start = 8, length = 20)

Consider testing similar scenario for line if. #Closed

AlekseyTs · 2017-03-29T22:10:55Z

Done with review pass. #Closed

jinujoseph · 2017-03-29T23:25:37Z

Thanks @AlekseyTs , have addressed the feedback

AlekseyTs · 2017-03-29T23:43:03Z

src/Compilers/VisualBasic/Test/Semantic/IOperation/IOperationTests_IIfStatement.vb

+
+            VerifyOperationTreeForTest(Of SingleLineIfStatementSyntax)(source, expectedOperationTree)
+        End Sub
+        <Fact(), WorkItem(17601, "https://github.com/dotnet/roslyn/issues/17601")>


<Fact(), WorkItem(17601, "https://github.com/dotnet/roslyn/issues/17601")> [](start = 8, length = 74)

Please add an empty line between the methods. #Closed

jinujoseph · 2017-03-30T17:01:22Z

retest windows_eta_open_prtest please

AlekseyTs

LGTM

jinujoseph · 2017-03-30T20:40:23Z

eta failure due to #18250
retest windows_eta_open_prtest please

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

mavasani reviewed Mar 22, 2017

View reviewed changes

AlekseyTs reviewed Mar 22, 2017

View reviewed changes

AlekseyTs added Feature - IOperation IOperation Area-Compilers labels Mar 22, 2017

jinujoseph force-pushed the UnitTest branch from eaf8d49 to 655e410 Compare March 24, 2017 20:14

jinujoseph changed the title ~~[WIP] Adding Unit tests for IfStatement~~ Adding Unit tests for IfStatement Mar 24, 2017

mavasani approved these changes Mar 27, 2017

View reviewed changes

jinujoseph force-pushed the UnitTest branch from 655e410 to e52aa60 Compare March 27, 2017 17:57

Adding Unit tests for IfStatement

09064ab

jinujoseph force-pushed the UnitTest branch from e52aa60 to 09064ab Compare March 28, 2017 06:37

heejaechang reviewed Mar 28, 2017

View reviewed changes

AlekseyTs reviewed Mar 29, 2017

View reviewed changes

Addressing PR feedback

6e8aa13

AlekseyTs reviewed Mar 29, 2017

View reviewed changes

jinujoseph added 2 commits March 29, 2017 18:59

Addressing pr feedback

600f1b6

Adding implicit embedded statement test case

739f222

AlekseyTs approved these changes Mar 30, 2017

View reviewed changes

jinujoseph merged commit fa37d1b into dotnet:master Mar 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Unit tests for IfStatement #18060

Adding Unit tests for IfStatement #18060

jinujoseph commented Mar 22, 2017 •

edited

Loading

mavasani Mar 22, 2017

mavasani Mar 22, 2017

AlekseyTs Mar 22, 2017 •

edited

Loading

mavasani Mar 22, 2017

jinujoseph Mar 22, 2017

mavasani Mar 22, 2017

AlekseyTs Mar 22, 2017 •

edited

Loading

mavasani left a comment

jinujoseph commented Mar 28, 2017

heejaechang Mar 28, 2017

heejaechang Mar 28, 2017

jinujoseph Mar 28, 2017

heejaechang commented Mar 28, 2017

jinujoseph commented Mar 28, 2017

AlekseyTs Mar 29, 2017 •

edited

Loading

jinujoseph Mar 29, 2017

AlekseyTs Mar 29, 2017 •

edited

Loading

jinujoseph Mar 29, 2017

AlekseyTs Mar 29, 2017 •

edited

Loading

AlekseyTs Mar 29, 2017 •

edited

Loading

AlekseyTs Mar 29, 2017 •

edited

Loading

AlekseyTs commented Mar 29, 2017 •

edited

Loading

jinujoseph commented Mar 29, 2017

AlekseyTs Mar 29, 2017 •

edited

Loading

jinujoseph commented Mar 30, 2017

AlekseyTs left a comment

jinujoseph commented Mar 30, 2017


		VerifyOperationTreeForTest(Of SingleLineIfStatementSyntax)(source, expectedOperationTree)
		End Sub

Adding Unit tests for IfStatement #18060

Adding Unit tests for IfStatement #18060

Conversation

jinujoseph commented Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Mar 22, 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

AlekseyTs Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

mavasani left a comment

Choose a reason for hiding this comment

jinujoseph commented Mar 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heejaechang commented Mar 28, 2017

jinujoseph commented Mar 28, 2017

AlekseyTs Mar 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Mar 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Mar 29, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 29, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 29, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Mar 29, 2017 • edited Loading

jinujoseph commented Mar 29, 2017

AlekseyTs Mar 29, 2017 • edited Loading

Choose a reason for hiding this comment

jinujoseph commented Mar 30, 2017

AlekseyTs left a comment

Choose a reason for hiding this comment

jinujoseph commented Mar 30, 2017

jinujoseph commented Mar 22, 2017 •

edited

Loading

AlekseyTs Mar 22, 2017 •

edited

Loading

AlekseyTs Mar 22, 2017 •

edited

Loading

AlekseyTs Mar 29, 2017 •

edited

Loading

AlekseyTs Mar 29, 2017 •

edited

Loading

AlekseyTs Mar 29, 2017 •

edited

Loading

AlekseyTs Mar 29, 2017 •

edited

Loading

AlekseyTs Mar 29, 2017 •

edited

Loading

AlekseyTs commented Mar 29, 2017 •

edited

Loading

AlekseyTs Mar 29, 2017 •

edited

Loading