Adding E2E test for VM Put bug for protected members by shahabhijeet · Pull Request #2945 · Azure/azure-sdk-for-net

shahabhijeet · 2017-03-16T22:12:35Z

Adding E2E test for VM Creation and Update.
Testing PUT request for VM succeeds even if Protected Properties are changed.

hovsepm · 2017-03-16T22:31:12Z

using System.Reflection;

Something is missing at the top of the file :)

Refers to: src/ClientRuntime/Microsoft.Rest.ClientRuntime.E2E.Tests/Properties/AssemblyInfo.cs:1 in 3179142. [](commit_id = 3179142, deletion_comment = False)

hovsepm · 2017-03-16T22:31:41Z

+                    //            StorageAccountType = StorageAccountTypes.StandardLRS
+                    //        }
+                    //    }
+                    //},


why do you keep this commented code?

@hovsepm I was planning on to use it, but then decided not to. Will remove.

hovsepm · 2017-03-16T22:31:55Z

namespace Microsoft.Rest.ClientRuntime.E2E.Tests.ScenarioTests

Missing license

Refers to: src/ClientRuntime/Microsoft.Rest.ClientRuntime.E2E.Tests/ScenarioTests/E2ETestBase.cs:1 in 3179142. [](commit_id = 3179142, deletion_comment = False)

hovsepm · 2017-03-16T22:32:05Z

namespace Microsoft.Rest.ClientRuntime.E2E.Tests.ScenarioTests

Missing license

Refers to: src/ClientRuntime/Microsoft.Rest.ClientRuntime.E2E.Tests/ScenarioTests/VMTests.cs:1 in 3179142. [](commit_id = 3179142, deletion_comment = False)

hovsepm · 2017-03-16T22:32:20Z

+
+        /// <summary>
+        /// Constructor
+        /// </summary>


useless comments. please remove

@hovsepm I am used to StyleCop, so I keep adding xml comments for public functions.

hovsepm · 2017-03-16T22:32:32Z

using Microsoft.Azure.Management.Compute;

Missing license

Refers to: src/ClientRuntime/Microsoft.Rest.ClientRuntime.E2E.Tests/TestAssets/ExtendingTypes.cs:1 in 3179142. [](commit_id = 3179142, deletion_comment = False)

hovsepm · 2017-03-16T22:32:46Z

+        /// <summary>
+        /// Constructor
+        /// </summary>
+        /// <param name="vm"></param>


please remove these comments.

@hovsepm will add more text to it

hovsepm · 2017-03-16T22:33:38Z

+                    //if (vmCustomizer != null)
+                    //{
+                    //    vmCustomizer(inputVM);
+                    //}


please remove all the commented code in this file

hovsepm · 2017-03-16T22:35:03Z

+    "Microsoft.Rest.ClientRuntime.Azure.TestFramework": {
+      "target": "project",
+      "type": "build"
+    },


please decouple ClientRutnimes tests and TestFramework. Do a nuget package reference instead.

@hovsepm we are writing E2E tests for client runtime, so we will need to take dependency on Test Framework.
Having nugget for Testframework does not add any value.
Taking project dependency help in, testing the latest version that is in repo as well as helps with debugging.
Unless there is something I am failing to understand.

hovsepm · 2017-03-16T22:35:32Z

-  "projects": [ "Microsoft.Rest.ClientRuntime", "Microsoft.Rest.ClientRuntime.Azure.Authentication", "Microsoft.Rest.ClientRuntime.Etw", 
-                "Microsoft.Rest.ClientRuntime.Tests", "Microsoft.Rest.ClientRuntime.Azure", "Microsoft.Rest.ClientRuntime.Azure.Tests",
-                "Microsoft.Rest.ClientRuntime.Log4Net", "Microsoft.Rest.ClientRuntime.Tracing.Tests" ]
+  "projects": [ "../TestFramework" ]


../TestFramework [](start = 17, length = 16)

we shouldn't create this dependency.

@hovsepm maybe I am not sure I understand your concern here, for test projects we will have to take dependency on TestFramework.

markcowl · 2017-03-16T22:39:28Z

+EndProject
+Project("{8BB2217D-0F2D-49D1-97BC-3654ED321F3B}") = "TestFramework", "..\TestFramework\Microsoft.Rest.ClientRuntime.Azure.TestFramework\TestFramework.xproj", "{C4C4E1C8-B99D-4D90-8C27-6D0C0A268BA5}"
+EndProject
+Project("{8BB2217D-0F2D-49D1-97BC-3654ED321F3B}") = "Microsoft.Azure.Management.Compute", "..\ResourceManagement\Compute\Microsoft.Azure.Management.Compute\Microsoft.Azure.Management.Compute.xproj", "{CBE97730-45F5-448E-88E9-55DF94D65B77}"


I don't think we need this in there, we should just be able to take a nuget dependency.

@markcowl I missed removing this, I will remove Compute.

markcowl · 2017-03-16T22:39:50Z

@@ -0,0 +1,88 @@
+namespace Microsoft.Rest.ClientRuntime.E2E.Tests.ScenarioTests


license header

markcowl · 2017-03-16T22:40:04Z

@@ -0,0 +1,793 @@
+namespace Microsoft.Rest.ClientRuntime.E2E.Tests.ScenarioTests


license header

markcowl · 2017-03-16T22:40:38Z

+
+        MockContext _mockContext;
+        ResourceManagementClient _resourceClient;
+        //ComputeManagementClient _computeClient;


remove commented code

markcowl · 2017-03-16T22:59:21Z

+                    bool created = false;
+                    while (!created)
+                    {
+                        System.Threading.Thread.Sleep(TimeSpan.FromSeconds(10));


Use the Wait method from the test framework: https://github.com/Azure/azure-sdk-for-net/blob/AutoRest/src/TestFramework/Microsoft.Rest.ClientRuntime.Azure.TestFramework/Utilities/TestUtilities.cs#L84

markcowl · 2017-03-16T23:00:49Z

+                                });
+                }
+            }
+            catch


why catch the exception to throw it? Just let the exception bubble up

@markcowl will remove, lot of code is remnant of trial and error in this really long running test.

markcowl · 2017-03-16T23:02:48Z

+        protected string CreateAvailabilitySet(string rgName, string asName, bool hasManagedDisks = false)
+        {
+            // Setup availability set
+            AvailabilitySet inputAvailabilitySet = null;


Why are we including all of this complication in the test - I don't think we need network to run the test effectively - we need to be able to create and update a VM, but we don't care that it's a functional VM.

@markcowl I modeled it after a simple test from compute. So I don't see a point in now removing it. Plus we don't know for sure at this stage if that is possible. So I am not going to experiment with this, as this is a really long running test and doing anything to experiment at this stage is not something I am planning on to do. If you feel strongly, we can create a issue and plan it.

markcowl · 2017-03-16T23:07:55Z

+                }
+                catch(Exception ex)
+                {
+                    Debug.WriteLine(ex.ToString());


I don't think we should catch exceptions, exceptions should cause the test to fail, right?

@markcowl yes I should not simply swallow exception, will throw. Catching helps during debugging, hence will leave it, but will throw.

markcowl · 2017-03-16T23:10:46Z

+                    Assert.False(requestContentStr.Contains("VmId"));
+
+                    //Get VM Object
+                    VirtualMachine getVm = res.Body;


So, this is going to be null, right? The VM update hasn't completed because we called BeginCreateOrUpdate, which returns immediately. I don't think we need this extra check, because the BeginUpdate request would have failed with a 400 otherwise, and we already checked the request.

@markcowl request returned by BeginCreateOrUpdateWithHttpMessages will not have empty body, plus it will not return immediately. Also this check will help to verify if my updated property was rejected (hence not serialized).

markcowl · 2017-03-16T23:11:15Z

@@ -0,0 +1,70 @@
+using Microsoft.Azure.Management.Compute;


license header

shahabhijeet · 2017-03-17T05:59:52Z

@azuresdkci retest this please

Adding E2E test for VM Put bug for protected members

3179142

msftclas added the cla-not-required label Mar 16, 2017

hovsepm reviewed Mar 16, 2017

View reviewed changes

markcowl requested changes Mar 16, 2017

View reviewed changes

markcowl added the needs-revision label Mar 16, 2017

markcowl assigned shahabhijeet Mar 16, 2017

Addressing PR feedback, re-recording test after addressing feedback

3fb91e6

cormacpayne added needs-review and removed needs-revision labels Mar 17, 2017

cormacpayne assigned markcowl and unassigned shahabhijeet Mar 17, 2017

shahabhijeet mentioned this pull request Mar 17, 2017

Verify runtime fixes for protected model property setters in autoirest-generated clients #2932

Closed

2 tasks

markcowl approved these changes Mar 20, 2017

View reviewed changes

markcowl merged commit 7303669 into Azure:AutoRest Mar 20, 2017

shahabhijeet deleted the VmPutBugTest branch September 4, 2017 15:44

		@@ -0,0 +1,88 @@
		namespace Microsoft.Rest.ClientRuntime.E2E.Tests.ScenarioTests

		@@ -0,0 +1,793 @@
		namespace Microsoft.Rest.ClientRuntime.E2E.Tests.ScenarioTests

Conversation

shahabhijeet commented Mar 16, 2017

Uh oh!

hovsepm commented Mar 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hovsepm commented Mar 16, 2017

Uh oh!

hovsepm commented Mar 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hovsepm commented Mar 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shahabhijeet Mar 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shahabhijeet Mar 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shahabhijeet Mar 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

shahabhijeet Mar 16, 2017 •

edited

Loading

shahabhijeet Mar 16, 2017 •

edited

Loading

shahabhijeet Mar 16, 2017 •

edited

Loading