Integration of vstest.console process with datacollector process. #418

harshjain2 · 2017-01-31T20:35:52Z

Integration of vstest.console process with datacollector process.

Related to :
#309

msftclas · 2017-01-31T20:35:57Z

Hi @harshjain2, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (Harsh Jain). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

codito · 2017-02-02T02:29:34Z

src/Microsoft.TestPlatform.Client/Execution/TestRunRequest.cs

@@ -291,7 +292,7 @@ public void HandleTestRunComplete(TestRunCompleteEventArgs runCompleteArgs, Test
                            runCompleteArgs.IsCanceled,
                            runCompleteArgs.IsAborted,
                            runCompleteArgs.Error,
-                            null,
+                            new Collection<AttachmentSet>(new List<AttachmentSet>(runContextAttachments)),


Why not pass runContextAttachments directly?

codito · 2017-02-02T02:29:58Z

src/Microsoft.TestPlatform.Client/Execution/TestRunRequest.cs

@@ -363,7 +364,7 @@ public virtual void HandleTestRunStatsChange(TestRunChangedEventArgs testRunChan
                EqtTrace.Info("TestRunRequest:SendTestRunStatsChange: Completed.");
            }
        }
-        
+


nit: remove blank line

codito · 2017-02-02T02:38:57Z

src/Microsoft.TestPlatform.Common/DataCollection/DataCollectionAttachmentManager.cs

        {
-            Task.WhenAll(this.attachmentTasks.ToArray()).Wait();
-            return this.AttachmentSets.Values.ToList();
+            if (isCancelled)


Should cancel be a responsibility of GetAttachments?

Implemented a new method Cancel that will take care of cancellation.

codito · 2017-02-02T02:46:45Z

src/Microsoft.TestPlatform.Common/DataCollection/DataCollectorLoader.cs

@@ -30,6 +29,7 @@ internal class DataCollectorLoader : IDataCollectorLoader
 #else
                assembly = AssemblyLoadContext.Default.LoadFromAssemblyPath(location);
 #endif
+                var dataCollectorType = assembly.GetTypes().ToList().Where(t => t.AssemblyQualifiedName.Equals(assemblyQualifiedName)).FirstOrDefault();


Please remove ToList.

codito · 2017-02-02T02:50:40Z

src/Microsoft.TestPlatform.Common/DataCollection/DataCollectionAttachmentManager.cs

+                EqtTrace.Error(ex.Message);
+            }
+
+            return this.AttachmentSets?.Values.ToList();


When will AttachmentSets be null?

codito · 2017-02-02T02:51:30Z

src/Microsoft.TestPlatform.Common/DataCollection/Interfaces/IDataCollectionManager.cs

-        /// <returns>Collection of session attachmentSet.</returns>
-        Collection<AttachmentSet> SessionEnded();
+        /// <param name="isCancelled">
+        /// Boolean to specify is the test ruun is cancelled or not.


nit: s/ruun/run

codito · 2017-02-02T02:54:23Z

src/Microsoft.TestPlatform.Common/Friends.cs

@@ -6,11 +6,16 @@
 #region Product Assemblies
 [assembly: InternalsVisibleTo("Microsoft.TestPlatform.CrossPlatEngine, PublicKey=002400000480000094000000060200000024000052534131000400000100010007d1fa57c4aed9f0a32e84aa0faefd0de9e8fd6aec8f87fb03766c834c99921eb23be79ad9d5dcc1dd9ad236132102900b723cf980957fc4e177108fc607774f29e8320e92ea05ece4e821c0a5efe8f1645c4c0c93c1ab99285d622caa652c1dfad63d745d6f2de5f17e5eaf0fc4963d261c8a12436518206dc093344d5ad293")]
 [assembly: InternalsVisibleTo("vstest.console, PublicKey =002400000480000094000000060200000024000052534131000400000100010007d1fa57c4aed9f0a32e84aa0faefd0de9e8fd6aec8f87fb03766c834c99921eb23be79ad9d5dcc1dd9ad236132102900b723cf980957fc4e177108fc607774f29e8320e92ea05ece4e821c0a5efe8f1645c4c0c93c1ab99285d622caa652c1dfad63d745d6f2de5f17e5eaf0fc4963d261c8a12436518206dc093344d5ad293")]
+[assembly: InternalsVisibleTo("datacollector, PublicKey =002400000480000094000000060200000024000052534131000400000100010007d1fa57c4aed9f0a32e84aa0faefd0de9e8fd6aec8f87fb03766c834c99921eb23be79ad9d5dcc1dd9ad236132102900b723cf980957fc4e177108fc607774f29e8320e92ea05ece4e821c0a5efe8f1645c4c0c93c1ab99285d622caa652c1dfad63d745d6f2de5f17e5eaf0fc4963d261c8a12436518206dc093344d5ad293")]


Why are internalsvisibleto required for these?

For exposing IMessageSink interface that is in common, datacollector implements IMessageSink.

codito · 2017-02-02T02:55:32Z

src/Microsoft.TestPlatform.Common/Friends.cs

@@ -6,11 +6,16 @@
 #region Product Assemblies
 [assembly: InternalsVisibleTo("Microsoft.TestPlatform.CrossPlatEngine, PublicKey=002400000480000094000000060200000024000052534131000400000100010007d1fa57c4aed9f0a32e84aa0faefd0de9e8fd6aec8f87fb03766c834c99921eb23be79ad9d5dcc1dd9ad236132102900b723cf980957fc4e177108fc607774f29e8320e92ea05ece4e821c0a5efe8f1645c4c0c93c1ab99285d622caa652c1dfad63d745d6f2de5f17e5eaf0fc4963d261c8a12436518206dc093344d5ad293")]
 [assembly: InternalsVisibleTo("vstest.console, PublicKey =002400000480000094000000060200000024000052534131000400000100010007d1fa57c4aed9f0a32e84aa0faefd0de9e8fd6aec8f87fb03766c834c99921eb23be79ad9d5dcc1dd9ad236132102900b723cf980957fc4e177108fc607774f29e8320e92ea05ece4e821c0a5efe8f1645c4c0c93c1ab99285d622caa652c1dfad63d745d6f2de5f17e5eaf0fc4963d261c8a12436518206dc093344d5ad293")]
+[assembly: InternalsVisibleTo("datacollector, PublicKey =002400000480000094000000060200000024000052534131000400000100010007d1fa57c4aed9f0a32e84aa0faefd0de9e8fd6aec8f87fb03766c834c99921eb23be79ad9d5dcc1dd9ad236132102900b723cf980957fc4e177108fc607774f29e8320e92ea05ece4e821c0a5efe8f1645c4c0c93c1ab99285d622caa652c1dfad63d745d6f2de5f17e5eaf0fc4963d261c8a12436518206dc093344d5ad293")]
+[assembly: InternalsVisibleTo("Microsoft.TestPlatform.CommunicationUtilities, PublicKey=002400000480000094000000060200000024000052534131000400000100010007d1fa57c4aed9f0a32e84aa0faefd0de9e8fd6aec8f87fb03766c834c99921eb23be79ad9d5dcc1dd9ad236132102900b723cf980957fc4e177108fc607774f29e8320e92ea05ece4e821c0a5efe8f1645c4c0c93c1ab99285d622caa652c1dfad63d745d6f2de5f17e5eaf0fc4963d261c8a12436518206dc093344d5ad293")]


~~Not sure why TP.Common needs a dependency on CommunicationUtilities.~~

codito · 2017-02-02T06:44:29Z

...oft.TestPlatform.CommunicationUtilities/Microsoft.TestPlatform.CommunicationUtilities.csproj

@@ -33,6 +33,11 @@
    <Reference Include="Microsoft.CSharp" />
  </ItemGroup>
  <ItemGroup>
+    <Reference Include="System.Dynamic.Runtime">


Why is this change required?

not required, removed.

codito · 2017-02-02T06:46:00Z

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

@@ -65,33 +58,18 @@ public void Initialize(Architecture architecture)
        public virtual int LaunchDataCollector(IDictionary<string, string> environmentVariables, IList<string> commandLineArguments)
        {
            var currentWorkingDirectory = Path.GetDirectoryName(typeof(DataCollectionLauncher).GetTypeInfo().Assembly.Location);
-            string dataCollectorProcessPath, processWorkingDirectory = null;
+            string dataCollectorProcessPath = null, processWorkingDirectory = null;


How are we handling scenarios where process doesn't start? Or process crashes?

This is handled in ProxyExecutionManagerWithDataCollection, the class responsible for initiating test execution when data collection is enabled.

codito · 2017-02-02T06:48:48Z

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

+
+            dataCollectorFileName = Path.GetFileNameWithoutExtension(dataCollectorAssemblyPath);
+
+            // .NET core host manager is not a shared host. It will expect a single test source to be provided.


This is not true. Datacollector is always shared?

codito · 2017-02-02T06:50:13Z

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

@@ -20,13 +19,8 @@ namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.DataCollection
    internal class DataCollectionLauncher : IDataCollectionLauncher


Please follow the convention:

DefaultDataCollectionLauncher

DotnetDataCollectionLauncher

codito · 2017-02-02T06:58:25Z

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

+                }
+            }
+
+            commandLineArguments.Insert(0, args);


IList.Insert is O(n). We're doing it multiple times in this logic. Why not use String.Concat and String.Join(commandLineArguments)?

codito · 2017-02-02T07:02:17Z

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

+            foreach (string path in pathString.Split(separator))
+            {
+                var exeFullPath = Path.Combine(path.Trim(), dotnetExeName);
+                if (File.Exists(exeFullPath))


How are we testing this logic? We have to use IFileHelper here.

codito · 2017-02-02T07:03:37Z

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

+        /// </summary>
+        /// <returns>Full path to <c>dotnet</c> executable</returns>
+        /// <remarks>Debuggers require the full path of executable to launch it.</remarks>
+        private string GetDotnetHostFullPath()


This functionality is DRY violation with DotnetTestHostManager. Please extract it to an independent unit.

Done. Extracted an interface IDotnetHostHelper.

codito · 2017-02-02T07:06:34Z

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

        {
        }

        /// <summary>
        /// Initializes a new instance of the <see cref="ProxyDataCollectionManager"/> class.
        /// </summary>
+        /// <param name="arch">


Boilerplate docs.

codito · 2017-02-02T07:16:16Z

test/datacollector.PlatformTests/CommunicationLayerIntegrationTests.cs

+            Assert.AreEqual("my://custom/datacollector", attachments[0].Uri.ToString());
+            Assert.IsTrue(attachments[0].Attachments[0].Uri.ToString().Contains("filename.txt"));
+
+            proxyDataCollectionManager.Dispose();


using () { ... } in all TestMethods.

codito · 2017-02-02T07:17:39Z

test/datacollector.PlatformTests/CommunicationLayerIntegrationTests.cs

+        {
+            var socketCommManager = new SocketCommunicationManager();
+            var dataCollectionRequestSender = new DataCollectionRequestSender(socketCommManager, JsonDataSerializer.Instance);
+#if NET46


Why not use this.dataCollectionLauncher?

IDataCollectorLauncher doesn't expose Process which I am using to monitor the process.
I don't see a need to expose the process in interface, therefore I am using specific instances here.

codito · 2017-02-02T07:18:56Z

test/datacollector.PlatformTests/CommunicationLayerIntegrationTests.cs

+            Assert.IsNull(attachments);
+
+            // Give time to datacollector process to exit.
+            Thread.Sleep(500);


Please use result.WaitForExit()

codito · 2017-02-02T07:22:37Z

test/datacollector.PlatformTests/datacollector.PlatformTests.csproj

+  <Import Project="$(TestPlatformRoot)scripts\build\TestPlatform.targets" />
+
+  <Target Name="AfterBuild" Condition=" '$(TargetFramework)' == 'netcoreapp1.0' ">
+    <Copy


Why is this required? Isn't the datacollector launched from artifacts\Debug\netXX\... directory in these tests?

These tests are executed from test\datacollector.PlatformTests\bin\Debug, therefore, we need to manually copy these files.

codito · 2017-02-02T07:28:05Z

src/Microsoft.TestPlatform.Common/DataCollection/DataCollectionAttachmentManager.cs

+                            }
+                        }
+                    },
+                this.cancellationTokenSource.Token);

            this.attachmentTasks.Add(task);


Just monitoring the continuationTask will suffice?

codito

Looks good with few suggestions.

codito · 2017-02-02T07:36:56Z

For the resx changes, please run build.cmd -xlf and include xlf files changes in PR.

Harsh Jain added 3 commits January 30, 2017 20:23

saving changes.

2d4b550

vstest.console process and datacollector process integration.

3de7770

Fixed failing test.

98d97a1

harshjain2 and others added 4 commits February 1, 2017 02:06

Merge branch 'master' into tpclientdatacollector

2b82cbe

Merge branch 'master' into tpclientdatacollector

d7f022c

Merge branch 'master' into tpclientdatacollector

3046cee

Updated Class name in eqttrace.

06f58ee

codito reviewed Feb 2, 2017

View reviewed changes

codito approved these changes Feb 2, 2017

View reviewed changes

Harsh Jain and others added 6 commits February 2, 2017 14:45

PR feedback.

0e4a6c2

PR feedback.

c06c004

xlf files.

a5d99c5

merged conflicts from master

c3430a3

Merge branch 'master' into tpclientdatacollector

24176de

fixing build issues that were introduced while merging.

464d316

harshjain2 merged commit 9e5a0f9 into microsoft:master Feb 2, 2017


		dataCollectorFileName = Path.GetFileNameWithoutExtension(dataCollectorAssemblyPath);

		// .NET core host manager is not a shared host. It will expect a single test source to be provided.

		@@ -20,13 +19,8 @@ namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.DataCollection
		internal class DataCollectionLauncher : IDataCollectionLauncher

Integration of vstest.console process with datacollector process. #418

Integration of vstest.console process with datacollector process. #418

Conversation

harshjain2 commented Jan 31, 2017 • edited Loading

msftclas commented Jan 31, 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

codito Feb 2, 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

codito Feb 2, 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

codito left a comment

Choose a reason for hiding this comment

codito commented Feb 2, 2017

harshjain2 commented Jan 31, 2017 •

edited

Loading

codito Feb 2, 2017 •

edited

Loading

codito Feb 2, 2017 •

edited

Loading