Load data collector #336
Load data collector #336
Conversation
| // 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.ObjectModel.DataCollector.Atrributes |
There was a problem hiding this comment.
s/Atrributes/Attributes.
Not sure if Attributes should be a new namespace.
| /// <summary> | ||
| /// Provides a friendly name for the data collector. | ||
| /// </summary> | ||
| public class DataCollecetorFriendlyNameAttribute : Attribute |
| } | ||
|
|
||
| [TestMethod] | ||
| public void InitializeDataCollectorShouldNotAddDataCollectorIfUriIsNotSpecified() |
There was a problem hiding this comment.
But in this test you are specifying the uri, am I missing here something?
There was a problem hiding this comment.
Renamed it to InitializeDataCollectorShouldNotAddDataCollectorIfUriIsNotSpecifiedByDataCollector
The scenario is that the data collector class hasn't declared value for attribute TypeUri.
|
|
||
| [DataCollecetorFriendlyName("CustomDataCollector")] | ||
| [DataCollectorTypeUri("my://custom/datacollector")] | ||
| public class CustomDataCollector : DataCollector |
There was a problem hiding this comment.
nitpick: can we rename it to CustomDataCollectorWithUri because the below one is CustomDataCollectorWithoutUri. What you say?
| /// <inheritdoc/> | ||
| public void Dispose() | ||
| { | ||
| throw new NotImplementedException(); |
There was a problem hiding this comment.
Please remove IDisposable if it is not implemented yet.
| } | ||
| catch (Exception ex) | ||
| { | ||
| if (EqtTrace.IsErrorEnabled) |
There was a problem hiding this comment.
How does error handling work for datacollector.exe? Which log file do these messages go in?
| } | ||
|
|
||
| /// <summary> | ||
| /// The get data collector information from binary. |
There was a problem hiding this comment.
Please replace the default text. Same comment for other methods.
| assembly = LoadAssemblyFromPath(dllPath); | ||
| } | ||
|
|
||
| if (assembly == null) |
There was a problem hiding this comment.
Is there a reason for supporting executable data collectors?
| assembly = null; | ||
| Type dctype = null; | ||
| var dllPath = string.Concat(binaryPath, ".dll"); | ||
| if (File.Exists(dllPath)) |
There was a problem hiding this comment.
How are we testing this logic with File.Exists?
| if (dataCollectorType == null) | ||
| { | ||
| // Try to find the data collector in the source directory | ||
| basePath = Path.Combine(SourceDirectory, collectorTypeName.Split(',')[1].Trim()); |
There was a problem hiding this comment.
This logic doesn't seem intuitive. If we're shipping CC DataCollector v1.0 in extensions folder; user can never use a v2.0 via nuget package?
| if (dataCollectorType == null) | ||
| { | ||
| // Try to find the data collector in the source directory | ||
| basePath = Path.Combine(SourceDirectory, collectorTypeName.Split(',')[1].Trim()); |
There was a problem hiding this comment.
What does this do? Is there an assumption that datacollector names are fullyqualified? Is there any base class API which parses an fully qualified AssemblyName?
Same comment for Line 132.
|
|
||
| // todo : get the config file. | ||
| // var configuration = DataCollectorDiscoveryHelper.GetConfigurationForAssembly(assembly); | ||
| var configuration = string.Empty; |
There was a problem hiding this comment.
Can we add these TODO as tasks in #309?
| var dataCollector = CreateDataCollector(dataCollectorConfig.DataCollectorType); | ||
|
|
||
| // Attempt to get the data collector information verifying that all of the required metadata for the collector is available. | ||
| testplatformDataCollector = dataCollectorConfig == null ? null : new TestPlatformDataCollector( |
|
|
||
| try | ||
| { | ||
| var dataCollector = CreateDataCollector(dataCollectorConfig.DataCollectorType); |
There was a problem hiding this comment.
What if dataCollector is null?
dataCollector and testplatformDataCollector names are confusing. Please consider rename.
| } | ||
| catch (Exception) | ||
| { | ||
| // data collector failed to initialize. Dispose it and mark it failed. |
There was a problem hiding this comment.
Please add todo here or log this with EqtTrace
| this.DataCollectorConfiguration = dataCollectorConfig; | ||
| this.TypeUri = GetTypeUri(type); | ||
|
|
||
| // todo : initialize FriendlyName. |
| @@ -1,8 +1,11 @@ | |||
| <Project ToolsVersion="15.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | |||
| <Project ToolsVersion="15.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | |||
There was a problem hiding this comment.
Please consider making datacollector executable AnyCpu.
| internal DataCollectionManager(IList<string> sources) | ||
| { | ||
| this.RunDataCollectors = new Dictionary<Type, TestPlatformDataCollector>(); | ||
| SourceDirectory = Path.GetDirectoryName(sources.First()); |
There was a problem hiding this comment.
It is not clear why we are setting the SourceDirectory to just the first source.
|
Closing this PR, will raise a new PR against future branch. |
Load data collector