Moving Desktop,& Dotnet runtime providers to separate dll #659

mayankbansal018 · 2017-03-24T13:39:44Z

Using this dll as extension, to load appropriate run-times for session

Treating Using this dll as extension, to load appropriate runtimes for

msftclas · 2017-03-24T13:39:48Z

@mayankbansal018,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

singhsarab · 2017-03-27T05:30:47Z

src/Microsoft.TestPlatform.Client/TestPlatform.cs


    /// <summary>
    /// Implementation for TestPlatform
    /// </summary>
    public class TestPlatform : ITestPlatform
    {
-        private IFileHelper fileHelper;
+        private readonly TestRuntimeProviderManager testHostProviderManager;


nit: rename, maybe ?

will do this in clean up PR. Have to rename lot of variables, & a few classes, & namespaces as well

singhsarab · 2017-03-27T05:32:28Z

src/Microsoft.TestPlatform.Client/TestPlatform.cs

        /// <summary>
        /// Initializes a new instance of the <see cref="TestPlatform"/> class.
        /// </summary>
-        public TestPlatform() : this(new TestEngine(), new FileHelper())
+        public TestPlatform() : this(new TestEngine(), new FileHelper(), TestRuntimeProviderManager.Instance)
        {
            this.testHostProviderManager = TestRuntimeProviderManager.Instance;


We don't need this statement anymore.

singhsarab · 2017-03-27T13:54:29Z

src/Microsoft.TestPlatform.CoreUtilities/Helpers/DotnetHostHelper.cs

@@ -58,7 +57,7 @@ public string GetDotnetHostFullPath()
                }
            }

-            string errorMessage = String.Format(Resources.NoDotnetExeFound, dotnetExeName);
+            string errorMessage = String.Empty; //String.Format(Resources.NoDotnetExeFound, dotnetExeName);


Please explain this change.
If it is required, we should clean it up.

I would need to add resource file for this project to accommodate for this resource.

It should be part of this PR please.

singhsarab · 2017-03-27T13:55:58Z

src/Microsoft.TestPlatform.CrossPlatEngine/DataCollection/DefaultDataCollectionLauncher.cs

@@ -66,7 +66,7 @@ public virtual int LaunchDataCollector(IDictionary<string, string> environmentVa

            var argumentsString = string.Join(" ", commandLineArguments);

-            this.DataCollectorProcess = this.processHelper.LaunchProcess(dataCollectorProcessPath, argumentsString, processWorkingDirectory, environmentVariables, null, null);
+            this.DataCollectorProcess = this.processHelper.LaunchProcess(dataCollectorProcessPath, argumentsString, processWorkingDirectory, environmentVariables, null, null) as Process;
            return this.DataCollectorProcess.Id;


Null check ?

…mayankbansal018/vstest-1 into desktop-dotnet-runtimeproviders

singhsarab · 2017-03-27T15:16:30Z

src/Microsoft.TestPlatform.PlatformAbstractions/common/Process/ProcessHelper.cs

                }

-                EqtTrace.Verbose("ProcessHelper: Starting process '{0}' with command line '{1}'", processPath, arguments);
+                // EqtTrace.Verbose("ProcessHelper: Starting process '{0}' with command line '{1}'", processPath, arguments);


Reason for removing diagnostics ?

Need to add back in this PR.

limitation since platform abstraction cannot take dependency on coreutils, which contain eqttrace implementation

singhsarab · 2017-03-27T15:18:38Z

src/Microsoft.TestPlatform.PlatformAbstractions/common/Process/ProcessHelper.cs

        {
-            if (process.HasExited)
+            var proc = process as Process;


null check here as well

singhsarab · 2017-03-27T15:21:31Z

src/package/VSIXProject/source.extension.vsixmanifest

@@ -1,7 +1,7 @@
 <?xml version="1.0" encoding="utf-8"?>
 <PackageManifest Version="2.0.0" xmlns="http://schemas.microsoft.com/developer/vsx-schema/2011" xmlns:d="http://schemas.microsoft.com/developer/vsx-schema-design/2011">
  <Metadata>
-    <Identity Id="Microsoft.TestPlatform.VSIX.Microsoft.0e51abf6-9f5f-401e-a812-4043b87acabf" Version="$version$" Language="en-US" Publisher="Microsoft" />
+    <Identity Id="Microsoft.TestPlatform.VSIX.Microsoft.0e51abf6-9f5f-401e-a812-4043b87acabf" Version="15.1.0" Language="en-US" Publisher="Microsoft" />


Is this intentional ?

Requires revert.

singhsarab · 2017-03-27T15:25:58Z

test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/TestEngineTests.cs

+            public event EventHandler<HostProviderEventArgs> HostExited;
+
+            public bool Shared { get; private set; }
+


nit: extra line

codito · 2017-03-28T04:30:16Z

scripts/build.ps1

@@ -257,6 +257,16 @@ function Publish-Package
        Write-Verbose "Move-Item $coreCLR20PackageDir\$file $coreCLRExtensionsDir -Force"
        Move-Item $coreCLR20PackageDir\$file $coreCLRExtensionsDir -Force
    }
+
+	# Note Note: If there are some dependencies for the TestHostProvider assemblies, those need to be moved too. 
+    $runtimeproviders = @("Microsoft.TestPlatform.TestHostProvider.dll", "Microsoft.TestPlatform.TestHostProvider.pdb")


Do we want to keep the name generic? Should the name say that these are NET runtime providers?

codito · 2017-03-28T04:31:22Z

src/Microsoft.TestPlatform.Client/TestPlatform.cs

+        /// <param name="testHostProviderManager">
+        /// The data.
+        /// </param>
+        internal TestPlatform(ITestEngine testEngine, IFileHelper filehelper, TestRuntimeProviderManager testHostProviderManager)


Why not protected? We already use the Testable approach for the unit tests.

codito · 2017-03-28T04:36:32Z

src/Microsoft.TestPlatform.CoreUtilities/Helpers/Interfaces/IDotnetHostHelper.cs

@@ -6,7 +6,7 @@ namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Helpers.Interfaces
    /// <summary>
    /// Helper class for getting info about dotnet host.
    /// </summary>
-    internal interface IDotnetHostHelper
+    public interface IDotnetHostHelper


Why should this be public? I would suggest moving this into TestHostProvider implementation.

We need to discuss how DataCollector can consume this.

leaving here for now, may be clean up later

codito · 2017-03-28T04:37:46Z

src/Microsoft.TestPlatform.CrossPlatEngine/TestEngine.cs

        private ITestExtensionManager testExtensionManager;

        #endregion

+        public TestEngine() : this(TestRuntimeProviderManager.Instance)


Require doc.

codito · 2017-03-28T04:38:04Z

src/Microsoft.TestPlatform.CrossPlatEngine/TestEngine.cs

+        {
+        }
+
+        internal TestEngine(TestRuntimeProviderManager testHostProviderManager)


Can be protected?

codito · 2017-03-28T04:40:18Z

src/Microsoft.TestPlatform.CrossPlatEngine/TestEngine.cs

-        /// </returns>
-        public ITestRuntimeProvider GetDefaultTestHostManager(RunConfiguration runConfiguration)
+        /// <inheritdoc/>
+        public ITestRuntimeProvider GetDefaultTestHostManager(string runSettingsXml)


May be this need not be an interface method? It's not getting the Default test host manager, it just gets the runtime provider which can understand a runsettings.

yes, it gets the appropriate runtime provider for current settings, but I let it be in Interface since that would require a lot of test changes as well

All the test changes should be in one test class, right? It is a smell if it require changes to multiple entities.

codito · 2017-03-28T06:44:48Z

src/Microsoft.TestPlatform.PlatformAbstractions/common/Process/ProcessHelper.cs


    /// <summary>
    /// Helper class to deal with process related functionality.
    /// </summary>
-    internal class ProcessHelper : IProcessHelper
+    public class ProcessHelper : IProcessHelper


Does it help to make this IProcessHelper<T>? If the consumer is aware of Process, it may as well call the ProcessHelper<Process> (may be do not require to do type casts).

Can be taken in separate PR

codito · 2017-03-30T04:28:28Z

test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Hosting/DotnetTestHostManagerTests.cs

@@ -19,6 +19,7 @@ namespace TestPlatform.CrossPlatEngine.UnitTests.Hosting
    using Microsoft.VisualStudio.TestPlatform.ObjectModel.Client.Interfaces;
    using Microsoft.VisualStudio.TestPlatform.ObjectModel.Host;
    using Microsoft.VisualStudio.TestPlatform.ObjectModel.Logging;
+    using Microsoft.VisualStudio.TestPlatform.PlatformAbstractions.Helpers.Interfaces;


These tests should move out to TestPlatformProvider.UnitTests. Kindly enable static analysis on new project.

codito · 2017-03-30T04:28:44Z

test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Hosting/DefaultTestHostManagerTests.cs

    using Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Hosting;
    using Microsoft.VisualStudio.TestPlatform.ObjectModel;
    using Microsoft.VisualStudio.TestPlatform.ObjectModel.Client.Interfaces;
    using Microsoft.VisualStudio.TestPlatform.ObjectModel.Host;
    using Microsoft.VisualStudio.TestPlatform.ObjectModel.Logging;
+    using Microsoft.VisualStudio.TestPlatform.PlatformAbstractions.Helpers.Interfaces;


Tests need to move out of crossplatengine.

codito · 2017-03-30T04:31:34Z

src/Microsoft.TestPlatform.PlatformAbstractions/netstandard1.0/Process/ProcessHelper.cs

+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+namespace Microsoft.VisualStudio.TestPlatform.PlatformAbstractions.Helpers


Not sure if there should be any Helper in the namespace. Process abstraction directly belongs to PlatformAbstractions since it is a platform specific concept?

codito

Request few changes.

codito · 2017-03-30T04:36:21Z

New assembly should be added to sign.proj.

Refactoring tests

Removing API to get test runtime provider from TestEngine

AbhitejJohn · 2017-03-30T16:48:14Z

src/Microsoft.TestPlatform.Client/TestPlatform.cs

        /// <summary>
        /// Initializes a new instance of the <see cref="TestPlatform"/> class.
        /// </summary>
-        public TestPlatform() : this(new TestEngine(), new FileHelper())
+        public TestPlatform() : this(new TestEngine(), new FileHelper(), TestRuntimeProviderManager.Instance)


I'm super confused: Whose responsibility is the TestRuntimeProvider? Not the engine's anymore? How would this look if another engine came in instead of the CrossPlatEngine? Is that even interesting anymore? We might need a doc for this please.

The responsibility to provide TestRuntime, is moved to TestRuntimeProviderManager, which for a given session identifies all available TestRuntime, for instance (Desktop, DotNet), & then returns back a particular instance of Runtime, which would work for current settings.

I see. And is this tied to the test engine in any way? What happens if a substitute engine wants to provide its own host process as a default?(say TAEF) Is that supported?

yes, If Taef wants to be the test runtime(i.e replace testhost), then they can provide their runtime whose job is to understand how to launch TAEF, once that is discovered, Taef can continue to host our crossplat engine to do the communication, & running test work, or they can implement their own communication framework which understands the protocol send across wire.

Moving Desktop,& Dotnet runtime providers to separate dll

fd93ee8

Treating Using this dll as extension, to load appropriate runtimes for

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

mayankbansal018 requested a review from codito March 24, 2017 14:11

singhsarab reviewed Mar 27, 2017

View reviewed changes

mayankbansal018 added 3 commits March 27, 2017 15:08

Merge branch 'master' into desktop-dotnet-runtimeproviders

1b61e28

Merge branch 'master' into desktop-dotnet-runtimeproviders

5cb7d2c

Minor changes

db3a328

singhsarab reviewed Mar 27, 2017

View reviewed changes

mayankbansal018 added 2 commits March 27, 2017 19:31

Merge branch 'desktop-dotnet-runtimeproviders' of https://github.com/…

335fdd8

…mayankbansal018/vstest-1 into desktop-dotnet-runtimeproviders

PR suggestions

160529d

singhsarab reviewed Mar 27, 2017

View reviewed changes

codito reviewed Mar 28, 2017

View reviewed changes

Faizan2304 mentioned this pull request Mar 28, 2017

Remove binding redirect of Newtonsoft.Json from testhost config file #663

Merged

codito reviewed Mar 30, 2017

View reviewed changes

mayankbansal018 added 3 commits March 30, 2017 20:00

Merge branch 'master' into desktop-dotnet-runtimeproviders

701029e

Refactoring namespace

3289513

Refactoring tests

Test Refactoring

e634f3f

Removing API to get test runtime provider from TestEngine

AbhitejJohn reviewed Mar 30, 2017

View reviewed changes

Static analysis rules fixes

3c5574e

codito approved these changes Mar 31, 2017

View reviewed changes

mayankbansal018 merged commit 876f819 into microsoft:master Mar 31, 2017

mayankbansal018 deleted the desktop-dotnet-runtimeproviders branch March 31, 2017 09:49

		public event EventHandler<HostProviderEventArgs> HostExited;

		public bool Shared { get; private set; }

Moving Desktop,& Dotnet runtime providers to separate dll #659

Moving Desktop,& Dotnet runtime providers to separate dll #659

Conversation

mayankbansal018 commented Mar 24, 2017

msftclas commented Mar 24, 2017

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

Choose a reason for hiding this comment

singhsarab Mar 27, 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

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

codito left a comment

Choose a reason for hiding this comment

codito commented Mar 30, 2017

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

singhsarab Mar 27, 2017 •

edited

Loading