[Iss#6180] Change TestController implementation #6651

vladimir-shcherbakov · 2018-07-12T18:25:33Z

Description

Change TestController implementation.
Issue #6180

Checklist

I have read the Submitting Changes section of CONTRIBUTING.md
The title of the PR is clear and informative
The appropriate change log has been updated
The PR does not introduce breaking changes
If applicable, the changes made in the PR have proper test coverage
For public API changes to cmdlets:
- the changes have gone through a cmdlet design review
- the cmdlet markdown files were generated using the platyPS module

markcowl · 2018-07-21T01:05:31Z

src/Common/Commands.Common.Compute.Tests/packages.config

@@ -1,6 +1,11 @@
 <?xml version="1.0" encoding="utf-8"?>
 <packages>
+  <package id="Microsoft.Azure.Test.HttpRecorder" version="1.8.1" targetFramework="net452" />


I think these should come from the common targets file, shouldn't they?

the line removed

markcowl · 2018-07-21T01:06:40Z

src/ResourceManager/Common/Commands.TestFw/Commands.TestFw.Netcore.csproj

@@ -0,0 +1,45 @@
+<Project Sdk="Microsoft.NET.Sdk">


Commands.TestFramework, or TestFx (this is a more common abbreviation for framework)

Renamed to TestFx

markcowl · 2018-07-21T01:07:34Z

src/ResourceManager/Common/Commands.TestFw/Properties/AssemblyInfo.cs

+[assembly: AssemblyTitle("Commands.TestFw")]
+[assembly: AssemblyDescription("")]
+[assembly: AssemblyConfiguration("")]
+[assembly: AssemblyCompany("HP Inc.")]


markcowl · 2018-07-21T01:07:44Z

src/ResourceManager/Common/Commands.TestFw/Properties/AssemblyInfo.cs

+[assembly: AssemblyConfiguration("")]
+[assembly: AssemblyCompany("HP Inc.")]
+[assembly: AssemblyProduct("Commands.TestFw")]
+[assembly: AssemblyCopyright("Copyright © HP Inc. 2018")]


markcowl · 2018-07-21T01:12:53Z

src/ResourceManager/Common/Commands.TestFw/TestManager.cs

+        /// Initial pair is {"Microsoft.Azure.Management.Resources.ResourceManagementClient", "2016-02-01"}
+        /// </param>
+        /// <returns>self</returns>
+        public IBuildable WithExtraUserAgentsToIgnore(Dictionary<string, string> userAgentsToIgnore)


SHould this add to the set of ignored user agents, or replace it? The method name suggests it should add to it.

In the builder pattern:

methods that have prefix WithExtra add a specifies value to a default value;

methods that have prefix WithNew replace/set default value with a new one.

markcowl · 2018-07-21T01:23:50Z

src/ResourceManager/Resources/Commands.Resources.Test/ScenarioTests/ResourceTests.cs

 using Xunit;
 using Xunit.Abstractions;

 namespace Microsoft.Azure.Commands.Resources.Test.ScenarioTests
 {
-    public class ResourceTests : RMTestBase
+    public class ResourceTests : TestManagerBuilder// : RMTestBase


same comment

markcowl · 2018-07-21T01:24:09Z

src/ResourceManager/Resources/Commands.Resources.Test/ScenarioTests/RoleAssignmentTests.cs

 using Microsoft.Rest.ClientRuntime.Azure.TestFramework;
 using Xunit;
 using Xunit.Abstractions;

 namespace Microsoft.Azure.Commands.Resources.Test.ScenarioTests
 {
-    public class RoleAssignmentTests : RMTestBase
+    public class RoleAssignmentTests : TestManagerBuilder


same comment

markcowl · 2018-07-21T01:24:23Z

src/ResourceManager/Resources/Commands.Resources.Test/ScenarioTests/RoleDefinitionTests.cs

 using Xunit;
 using Xunit.Abstractions;

 namespace Microsoft.Azure.Commands.Resources.Test.ScenarioTests
 {
-    public class RoleDefinitionTests : RMTestBase
+    public class RoleDefinitionTests : TestManagerBuilder


same comment

markcowl · 2018-07-21T01:27:08Z

src/ResourceManager/Resources/Commands.Resources.Test/ScenarioTests/TestManagerBuilder.cs

+        /// </summary>
+        /// <param name="credentials">The credentials.</param>
+        /// <param name="headerValues">The headers.</param>
+        public override HttpClientHelper CreateHttpClientHelper(SubscriptionCloudCredentials credentials, IEnumerable<ProductInfoHeaderValue> headerValues, Dictionary<string, string> cmdletHeaderValues)


why not just implement this in the ClientFactory, so there doesn't have to be a special implementation?

No modules, other than Resources, needed this implementation so far.

This could change in the future... As of now only one module uses it, but other modules might want to use it (if the functionality is generic enough).

If the functionality here is super tied to resource manager and we cant foresee anyone ussing this ever, then lets jsut call this class ResourceManagerTestBase.

markcowl · 2018-07-21T01:28:11Z

src/ResourceManager/Storage/Commands.Management.Storage.Test/App.config

+  <runtime> 
+      <assemblyBinding xmlns="urn:schemas-microsoft-com:asm.v1"> 
+          <dependentAssembly> 
+              <assemblyIdentity name="Newtonsoft.Json" publicKeyToken="30ad4fe6b2a6aeed" culture="neutral" /> 


I think thsi should be 10.0 if using the latest version of the .net test library

praries880 · 2018-07-18T20:51:26Z

src/Common/Commands.Common.Compute.Tests/Commands.Common.Compute.Tests.csproj

    <Reference Include="Microsoft.IdentityModel.Clients.ActiveDirectory">
      <HintPath>..\..\packages\Microsoft.IdentityModel.Clients.ActiveDirectory.2.28.3\lib\net45\Microsoft.IdentityModel.Clients.ActiveDirectory.dll</HintPath>
      <Private>True</Private>
    </Reference>
    <Reference Include="Microsoft.Rest.ClientRuntime, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
      <SpecificVersion>False</SpecificVersion>
-      <HintPath>..\..\packages\Microsoft.Rest.ClientRuntime.2.3.11\lib\net452\Microsoft.Rest.ClientRuntime.dll</HintPath>
+      <HintPath>..\..\packages\Microsoft.Rest.ClientRuntime.2.3.7\lib\net452\Microsoft.Rest.ClientRuntime.dll</HintPath>


why is the version regressing here?

This happened after I added Microsoft.Rest.ClientRuntime.Azure.TestFramework dependency to the project with NuGet.

can this be reverted to the newer version or is that because nuget has the older version?

Why is it so important? After a newer version of TestFramework release all dependencies will be updated. At the moment the 2.3.7 version of Microsoft.Rest.ClientRuntime is sufficient to get tests pass.

praries880 · 2018-07-26T18:50:34Z

src/ResourceManager/Common/Commands.TestFx/TestClientFactory.cs

+            }
+
+            var graphClient = _mockContext.GetGraphServiceClient<GraphRbacManagementClient>();
+            graphClient.TenantID = context.Tenant.Id;


Does _mockContext.GetGraphServiceClient<GraphRbacManagementClient>(); not return a graphClient with the TenaniId initi'd?

When I debugged this - the TenantId was not initialized, that's why I added the line #48.

Is it the same behavior in Playback, REcord and Live modes?
Might be better to just add a check to see if the graphClient.TenantID is non null.. in case its null only then set it to the tenentId in the context...

Context initialization happens in TestManager and depends on a test mode. I doubt mock context does any client initialization.

praries880 · 2018-07-26T19:50:08Z

src/ResourceManager/Resources/Commands.Resources.Test/ScenarioTests/TestManagerBuilder.cs

+
+namespace Microsoft.Azure.Commands.Resources.Test.ScenarioTests
+{
+    public class TestManagerBuilder


Can you remane this to something that implies that the class is intended to be a base class for all the resourceManager Tests?

Something like "ResourceManagerTestBase", TestManageVuilder sounds more like a builder class that should be packaged with TestManager.

@praries880,
I didn't mean TestManagerBuilder to be a base class for all the Resources module tests,

I would say it's just a concise way to share a common test infrastructure setup among test classes. The idea was to hide the TestManager instance initialization in the base class constructor and share it via protected member. Otherwise I had to copy the same initialization sequence to each test class constructor.

If a customer needs a different infrastructure setup they can build a specific version of TestManager instance and inheritance would be redundant.

markcowl

This is really nice work.

A few comments - I think we want to use a single pattern for consistency, I think we need to include some support for Hyak clients.

markcowl · 2018-08-03T00:14:31Z

src/ResourceManager/Common/Commands.TestFx/TestClientFactory.cs

+
+        #region Hyak
+
+        public TClient CreateClient<TClient>(IAzureContext context, string endpoint) where TClient : Hyak.Common.ServiceClient<TClient>


Weneed to support these, one way or another, as there are still some tests with Hyak clients. Here is a sample of how we can support these with minimal additions without having to do any elaborate mocking: https://github.com/Azure/azure-powershell/blob/preview/src/Common/Commands.ScenarioTests.Common/Mocks/MockClientFactory.cs#L111

This will be implemented as part of the #6181 task.

markcowl · 2018-08-03T00:18:09Z

src/ResourceManager/Common/Commands.TestFx/packages.config

+<?xml version="1.0" encoding="utf-8"?>
+<packages>
+  <package id="Hyak.Common" version="1.1.0" targetFramework="net461" />
+  <package id="Microsoft.Azure.Test.HttpRecorder" version="1.8.1" targetFramework="net461" />


why are these targets net461? just curious.

markcowl · 2018-08-03T00:54:32Z

src/ResourceManager/Common/Commands.TestFx/TestManager.cs

+
+    #region Builder interfaces
+
+    public interface IPreBuildable


public interfaces should be in separate files

I woould rename this ITestRunner and ITestRunnerFactory, to make the names closer to the functions.

markcowl · 2018-08-03T01:02:18Z

src/ResourceManager/Profile/Commands.Profile.Test/ArgumentCompleterTests.cs

    {
-        private XunitTracingInterceptor xunitLogger;
+        private readonly ITestRunnable _testManager;


_testRunner. Let's use a common pattern in all the places we are using the new faclity, wither:

(1) Have all the test classes extend TestManager or
(2) Have all the test classes create an ITestRunner in their constructor.

This will make things simpler for our developers - they will have only one pattern to follow.

markcowl · 2018-08-03T01:07:56Z

src/ResourceManager/Resources/Commands.Resources.Test/ScenarioTests/TestManagerBuilder.cs

+    {
+        protected readonly ITestRunnable TestManager;
+
+        protected TestManagerBuilder(ITestOutputHelper output)


I wonder if we shouldn't have a constructor overload that allows passing in an ITestRunnerFactory (IBuildable). This would enable tests to have their own implementation of TestManager, and still add in the default options as below.

MiYanni · 2018-08-16T22:55:52Z

Set this to Do Not Merge until we get the associated azure-powershell-common changes in.

vladimir-shcherbakov · 2018-09-20T18:20:06Z

Evolved into the PR #7314 .

azuresdkci added the needs-review label Jul 12, 2018

vladimir-shcherbakov added 6 commits July 12, 2018 16:52

New test contoller impl

6fa519b

Profile switched to the new test controller

412da22

Storage management switched to the new test controller

670af31

Resources switched to the new test controller

92bdf2f

Common.Compute switched to TestFramework

948981f

Netcore sln files update.

cf43fb6

vladimir-shcherbakov force-pushed the iss#6180 branch from 5e78cc8 to cf43fb6 Compare July 12, 2018 23:58

vladimir-shcherbakov and others added 3 commits July 12, 2018 18:15

AZURE_TEST_MODE=Playback added

d9e97be

travis-ci build fix

514785c

travis ci-build fix

ba186b6

vladimir-shcherbakov force-pushed the iss#6180 branch from 514785c to ba186b6 Compare July 13, 2018 07:05

vladimir-shcherbakov requested review from markcowl, cormacpayne, maddieclayton, MiYanni and praries880 July 13, 2018 16:58

maddieclayton assigned markcowl and cormacpayne Jul 16, 2018

markcowl requested changes Jul 21, 2018

View reviewed changes

markcowl removed their assignment Jul 21, 2018

vladimir-shcherbakov and others added 4 commits July 24, 2018 18:50

PR comments fixed.

4046697

Merge branch 'preview' into iss#6180

9c91135

Merge remote-tracking branch 'ustr/preview' into iss#6180

6ac1116

Upstream merge fix.

52c28d4

vladimir-shcherbakov assigned markcowl and unassigned cormacpayne Jul 25, 2018

praries880 reviewed Jul 25, 2018

View reviewed changes

praries880 reviewed Jul 26, 2018

View reviewed changes

vladimir-shcherbakov and others added 3 commits July 26, 2018 16:43

Rest.ClientRuntime version fix

b37ced2

Merge remote-tracking branch 'ustr/preview' into iss#6180

960b307

Merge remote-tracking branch 'ustr/preview' into iss#6180

7baa23e

markcowl requested changes Aug 3, 2018

View reviewed changes

markcowl added needs-revision and removed needs-review labels Aug 3, 2018

markcowl assigned vladimir-shcherbakov and unassigned markcowl Aug 3, 2018

vladimir-shcherbakov and others added 3 commits August 5, 2018 23:28

PR comments fix

0a98705

PR comments fix

bc09432

removed magically added test duplicates.

698a8bf

vladimir-shcherbakov assigned markcowl and unassigned vladimir-shcherbakov Aug 6, 2018

vladimir-shcherbakov added needs-review and removed needs-revision labels Aug 6, 2018

markcowl added Post Common and removed Post Common labels Aug 13, 2018

maddieclayton removed the Post Common label Aug 16, 2018

MiYanni added the Do Not Merge 🚫 label Aug 16, 2018

markcowl removed the Do Not Merge 🚫 label Aug 16, 2018

maddieclayton added Merge Conflicts needs-revision and removed needs-review labels Sep 10, 2018

maddieclayton assigned vladimir-shcherbakov and unassigned markcowl Sep 10, 2018

maddieclayton added the Do Not Merge 🚫 label Sep 11, 2018

vladimir-shcherbakov closed this Sep 20, 2018


		#region Hyak

		public TClient CreateClient<TClient>(IAzureContext context, string endpoint) where TClient : Hyak.Common.ServiceClient<TClient>

[Iss#6180] Change TestController implementation #6651

[Iss#6180] Change TestController implementation #6651

Uh oh!

Conversation

vladimir-shcherbakov commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

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!

vladimir-shcherbakov Jul 25, 2018 • 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!

praries880 Jul 26, 2018 • 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!

vladimir-shcherbakov Jul 26, 2018 • 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!

vladimir-shcherbakov Jul 26, 2018 • 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!

praries880 Jul 26, 2018 • 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!

praries880 Jul 26, 2018 • 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!

vladimir-shcherbakov Jul 26, 2018 • 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!

markcowl left a comment

Choose a reason for hiding this comment

Uh oh!

vladimir-shcherbakov commented Jul 12, 2018 •

edited

Loading

vladimir-shcherbakov Jul 25, 2018 •

edited

Loading

praries880 Jul 26, 2018 •

edited

Loading

vladimir-shcherbakov Jul 26, 2018 •

edited

Loading

vladimir-shcherbakov Jul 26, 2018 •

edited

Loading

praries880 Jul 26, 2018 •

edited

Loading

praries880 Jul 26, 2018 •

edited

Loading

vladimir-shcherbakov Jul 26, 2018 •

edited

Loading