Data Collector Process #369

harshjain2 · 2017-01-22T19:19:27Z

Implemented following features :
Load Data Collectors.
Data Collection Events.
Data Collection Message Sink.
Data Collector Message Logger
Removed x86 version of DataCollector, added AnyCPU Version.

msftclas · 2017-01-22T19:19:33Z

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;

harshjain2 · 2017-01-24T10:21:41Z

@dotnet-bot test this please

codito · 2017-01-25T05:50:55Z

test/datacollector.UnitTests/DummyDataCollectors.cs

+        public static bool IsSessionStartedInvoked = false;
+        public static bool IsSessionEndedInvoked = false;
+        public static bool GetTestExecutionEnvironmentVariablesThrowException;
+        public static IEnumerable<KeyValuePair<string, string>> EnvVarList;


Since this is a spy, can we use Moq?

As we are loading the datacollectors through run settings, it is not possible to dynamically construct the object and load it through run settings.

codito · 2017-01-25T05:52:38Z

test/datacollector.UnitTests/DataCollectionManagerTests.cs

+            this.mockMessageSink = new DummyMessageSink();
+            this.dataCollectionManager = new DataCollectionManager(new DataCollectionAttachmentManager(), new DummyMessageSink());
+
+            this.dataCollectorSettings = string.Format("<DataCollector friendlyName=\"CustomDataCollector\" uri=\"my://custom/datacollector\" assemblyQualifiedName=\"Microsoft.VisualStudio.TestPlatform.DataCollector.UnitTests.CustomDataCollector, datacollector.UnitTests, Version=15.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a\" codebase=\"{0}\" />", typeof(CustomDataCollector).GetTypeInfo().Assembly.Location);


ft.VisualStudio.TestPlatform.DataCollector.UnitTests.CustomDataCollector, datacollector.UnitTests, Version=15.0.0.0, [](start = 173, length = 116)

Derive this from typeof(CustomDataCollector).GetTypeInfo().Assembly.AssemblyName

codito · 2017-01-25T06:00:47Z

src/datacollector/DataCollectionManager.cs

+            ValidateArg.NotNull(settingsXml, "settingsXml");
+
+            var sessionId = new SessionId(Guid.NewGuid());
+            var dataCollectionContext = new DataCollectionContext(sessionId);


DataCollectionContext [](start = 44, length = 21)

Please remove DataCollectionContext(TestCase) ctor.

This is getting used in InProcDataCollector. We can revisit this later.

codito · 2017-01-25T06:13:22Z

test/datacollector.UnitTests/DataCollectionManagerTests.cs

+
+
+        [TestInitialize]
+        public void Init()


Init [](start = 20, length = 4)

See if this can be put in ctor.

codito · 2017-01-25T06:24:02Z

src/datacollector/DataCollectionManager.cs

+
+            this.attachmentManager.Initialize(sessionId, sourceDirectory, this.messageSink);
+
+            var executionEnvironmentVariables = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);


executionEnvironmentVariables [](start = 16, length = 29)

Please add a comment with rationale for stringcomparer.

codito · 2017-01-25T06:25:08Z

test/datacollector.UnitTests/DataCollectionManagerTests.cs

+        [TestMethod]
+        public void InitializeDataCollectorsShouldReturnEmptyDictionaryIfDataCollectorsAreNotConfigured()
+        {
+            var RunSettings = string.Format(this.defaultRunSettings, string.Empty);


RunSettings [](start = 16, length = 11)

nit: camelCase

codito · 2017-01-25T06:25:47Z

src/datacollector/DataCollectionManager.cs

+        /// <inheritdoc/>
+        public IDictionary<string, string> InitializeDataCollectors(string settingsXml)
+        {
+            ValidateArg.NotNull(settingsXml, "settingsXml");


Add tracing

codito · 2017-01-25T06:27:43Z

src/datacollector/DataCollectionManager.cs

+            var runConfiguration = XmlRunSettingsUtilities.GetRunConfigurationNode(settingsXml);
+            sourceDirectory = RunSettingsUtilities.GetTestResultsDirectory(runConfiguration);
+
+            var dataCollectionRunSettings = XmlRunSettingsUtilities.GetDataCollectionRunSettings(settingsXml);


XmlRunSettingsUtilities, DataCollectionRunSettings does not have traces. Please add.

codito · 2017-01-25T06:29:41Z

test/datacollector.UnitTests/DataCollectionManagerTests.cs

+            var result = this.dataCollectionManager.InitializeDataCollectors(runSettings);
+
+            Assert.AreEqual(1, result.Count);
+            Assert.AreEqual("key", result.Keys.First());


Assert [](start = 12, length = 6)

Use CollectionAssert.

codito · 2017-01-25T06:37:11Z

test/datacollector.UnitTests/DataCollectionManagerTests.cs

+            Assert.IsTrue(CustomDataCollector.IsInitialized);
+        }
+
+        [TestMethod]


TestMethod [](start = 9, length = 10)

Add test for InitializeShouldNotAddDataCollectorIfItIsDisabled

codito · 2017-01-25T06:39:22Z

src/datacollector/DataCollectionManager.cs

+        private void LoadAndInitialize(DataCollectorSettings dataCollectorSettings)
+        {
+            var collectorTypeName = dataCollectorSettings.AssemblyQualifiedName;
+            var collectorDisplayName = string.IsNullOrWhiteSpace(dataCollectorSettings.FriendlyName) ? collectorTypeName : dataCollectorSettings.FriendlyName;


Add a test case for this

codito · 2017-01-25T06:41:35Z

src/datacollector/DataCollectionManager.cs

+        {
+            assembly = null;
+            Type dctype = null;
+            if (File.Exists(binaryPath))


Add test case for negative scenario. May need to use IFileHelper.

codito · 2017-01-25T06:43:42Z

src/datacollector/DataCollectionManager.cs

+                assembly = LoadAssemblyFromPath(binaryPath);
+            }
+
+            dctype = assembly?.GetTypes().FirstOrDefault(type => type.AssemblyQualifiedName != null && type.AssemblyQualifiedName.Equals(dataCollectorTypeName));


assembly.GetType(typeName)

codito · 2017-01-25T06:48:09Z

src/datacollector/DataCollectorConfig.cs

+            // The type uri can not be null or empty.
+            if (string.IsNullOrEmpty(typeUriAttribute.TypeUri))
+            {
+                throw new ArgumentException(string.Format(CultureInfo.CurrentCulture, Resources.Resources.DataCollector_TypeIsNull, dataCollectorType.FullName));


ArgumentException [](start = 26, length = 17)

Need tests for these

codito · 2017-01-25T06:48:41Z

test/datacollector.UnitTests/DataCollectionManagerTests.cs

+        }
+
+        [TestMethod]
+        public void InitializeDataCollectorsShouldReturnOnlyOneEnvirnmentVariableIfMoreThanOneVariablesWithSameKeyIsSpecified()


InitializeDataCollectorsShouldReturnOnlyOneEnvirnmentVariableIfMoreThanOneVariablesWithSameKeyIsSpecified [](start = 20, length = 105)

nit: spell

codito · 2017-01-25T06:50:04Z

src/datacollector/DataCollectionManager.cs

+                return;
+            }
+
+            if (this.RunDataCollectors.ContainsKey(dataCollectorConfig.DataCollectorType))


This check is not required since RunSettings already deduplicates

codito · 2017-01-25T07:59:58Z

src/datacollector/DataCollectorWrapper.cs

+    /// <summary>
+    /// Encapsulates datacollector object and other objects required to facilitate datacollection.
+    /// </summary>
+    internal class DataCollectorWrapper


DataCollectorWrapper [](start = 19, length = 20)

Rename to DataCollectorInformation

codito · 2017-01-25T08:01:47Z

test/datacollector.UnitTests/DataCollectionManagerTests.cs

+        }
+    }
+
+    internal class MockDataCollectionAttachmentManager : IDataCollectionAttachmentManager


MockDataCollectionAttachmentManager [](start = 19, length = 35)

Replace with Moq

codito · 2017-01-25T08:03:45Z

src/datacollector/DataCollectionManager.cs

+        }
+
+        /// <inheritdoc/>
+        public Collection<AttachmentSet> SessionEnded()


SessionEnded [](start = 41, length = 12)

This should not return null. If it does, every consumer has to add an extra null check. May be an empty set if better.

codito · 2017-01-25T08:05:28Z

src/datacollector/DataCollectionAttachmentManager.cs

+
+            if (string.IsNullOrEmpty(outputDirectory))
+            {
+                this.SessionOutputDirectory = Path.Combine(Path.GetTempPath(), DefaultOutputDirectoryName);


SessionOutputDirectory [](start = 21, length = 22)

Path.Combine takes N number of strings

codito · 2017-01-25T08:39:43Z

src/datacollector/DataCollectionManager.cs

+
+                try
+                {
+                    dataCollectorWrapper.Events.RaiseEvent(args);


dataCollectorWrapper [](start = 20, length = 20)

Should be this.events.RaiseEvent

codito · 2017-01-25T08:52:00Z

src/datacollector/DataCollectionAttachmentManager.cs

+        /// <summary>
+        /// Gets the list of attachment requests.
+        /// </summary>
+        internal List<AttachmentRequest> AttachmentRequests { get; private set; }


AttachmentRequests [](start = 41, length = 18)

It should be possible to replace this with async Tasks for Copy/Move. We could wait on Task.WhenAll(attachmentRequests)?

codito · 2017-01-25T08:55:51Z

src/datacollector/DataCollectionAttachmentManager.cs

+        public void Dispose()
+        {
+            // Dispose all attachment requests..
+            this.AttachmentRequests.ForEach(request => request.Dispose());


AttachmentRequests [](start = 17, length = 18)

If AttachmentRequests are not required (in case of Task), we may not need Dispose.

codito · 2017-01-25T09:19:08Z

🕐

codito · 2017-01-27T10:25:09Z

src/datacollector/DataCollectionAttachmentManager.cs

-        /// <summary>
-        /// Dispose event object
-        /// </summary>
+        /// <inheritdoc/>
        public void Dispose()


Why is this required? Do we not allocate a new DataCollectionAttachmentManager for data collection session?

codito · 2017-01-27T10:27:59Z

src/datacollector/DataCollectionAttachmentManager.cs

            if (!Directory.Exists(directoryName))
            {
                Directory.CreateDirectory(directoryName);
            }
-            else if (File.Exists(attachmentRequest.LocalFilePath))
+            else if (File.Exists(localFilePath))


Should deletion of existing file be responsibility of Validate?

This should be logged that we're removing existing file. What happens if we cannot remove the file?

codito · 2017-01-27T10:29:15Z

src/datacollector/DataCollectionAttachmentManager.cs

+
+            var directoryPath = Path.Combine(
+                this.SessionOutputDirectory,
+                testCaseId.ToString());


testCaseId is already a string

codito · 2017-01-27T10:30:00Z

src/datacollector/DataCollectionAttachmentManager.cs

+                 {
+                     if (fileTransferInfo.PerformCleanup)
+                     {
+                         File.Move(fileTransferInfo.FileName, localFilePath);


Please log these operations.

codito · 2017-01-27T10:33:53Z

src/datacollector/DataCollectionAttachmentManager.cs

+                 }
+             });
+
+            this.attachmentTasks.Add(task);


We're waiting on the task, not the continuationTask?

codito · 2017-01-27T10:35:01Z

src/datacollector/DataCollectionAttachmentManager.cs

+
+                 try
+                 {
+                     if (fileTransferInfo.PerformCleanup)


It is possible user may want to cancel a test run or the runner/communication channel crash during these operations. What is the strategy in such cases?

In order to handle user triggered test run cancellation, we will use cancellation token to cancel the in-progress file transfer tasks.
For crash, we need to detect the socket communication failure and then cancellation of in-progress transfer operations before exiting.

Added a todo for now, will handle this scenario with integration of vstest.console process with datacollector process.

harshjain2 · 2017-01-28T07:54:08Z

@dotnet-bot test this please.

codito · 2017-01-30T01:29:01Z

What is the change to fix build issues?

Please undo the change in TestPlatform.targets before merge. It is required for localization targets.

Harsh Jain added 2 commits January 20, 2017 02:07

DataCollector Process.

a9ccde0

Added tests.

b44a156

Doc changes.

82458cc

harshjain2 changed the base branch from future to master January 24, 2017 06:44

Harsh Jain added 2 commits January 24, 2017 12:27

Merged with master

fe79b2f

merged master, fixed build issues.

3ace0af

codito reviewed Jan 25, 2017

View reviewed changes

Harsh Jain and others added 3 commits January 26, 2017 22:34

PR feedback.

ed9f8ae

Merge branch 'master' into datacollector

e064a04

fixed build issues.

b40bccb

codito reviewed Jan 27, 2017

View reviewed changes

PR feedback.

eb999ac

Harsh Jain added 2 commits January 29, 2017 18:55

Fixed build issues.

563d8ea

fixed build issues.

e8963f3

Harsh Jain added 4 commits January 30, 2017 10:29

reverting testplatform.targets related change for loc.

5f07555

formatting changes in TestPlatform.targets.

a0427fe

removed extra space in TestPlatform.Targets.

784a2b9

added tracing for null or empty null runsettings.

9c38c8c

harshjain2 merged commit 86db2f3 into microsoft:master Jan 30, 2017


		this.attachmentManager.Initialize(sessionId, sourceDirectory, this.messageSink);

		var executionEnvironmentVariables = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);

Data Collector Process #369

Data Collector Process #369

Conversation

harshjain2 commented Jan 22, 2017

msftclas commented Jan 22, 2017

harshjain2 commented Jan 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

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 commented Jan 25, 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

harshjain2 commented Jan 28, 2017

codito commented Jan 30, 2017