TestPlugin cache refactored. #709

harshjain2 · 2017-04-12T23:50:09Z

Refactored Test Plugin Cache to support loading for individual extensions.

msftclas · 2017-04-12T23:50:13Z

@harshjain2,
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

Faizan2304 · 2017-04-13T08:58:57Z

src/Microsoft.TestPlatform.Common/ExtensionFramework/TestPluginCache.cs

@@ -113,7 +104,7 @@ public IEnumerable<string> PathToAdditionalExtensions
        {


Should we rename it to PathToExtensions

Faizan2304 · 2017-04-13T09:31:06Z

src/Microsoft.TestPlatform.Common/ExtensionFramework/Utilities/TestExtensions.cs


+        /// <summary>
+        /// Gets or sets a value indicating whether are test discoverers cached.
+        /// </summary>


Where are we setting the value of variable AreTestDiscoverersCached

AbhitejJohn · 2017-04-13T10:43:11Z

src/Microsoft.TestPlatform.Common/Logging/TestLoggerExtensionManager.cs

            IEnumerable<LazyExtension<ITestLogger, Dictionary<string, object>>> unfilteredTestExtensions;

-            TestPluginManager.Instance.GetTestExtensions<ITestLogger, ITestLoggerCapabilities, TestLoggerMetadata>(
+            TestPluginManager.Instance.GetSpecificTestExtensions<TestLoggerPluginInformation, ITestLogger, ITestLoggerCapabilities, TestLoggerMetadata>(


TestLoggerPluginInformation [](start = 65, length = 27)

Would we ever call TestLoggerPluginInformation with a different regex pattern? Maybe the regex patter should be in TestPluginInfo itself. Dont see a reason for it to be propagated everywhere when TestPluginInfo is anyway passed through.

AbhitejJohn · 2017-04-13T10:44:18Z

src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyDiscoveryManager.cs

-            {
-                extensions.AddRange(TestPluginCache.Instance.PathToAdditionalExtensions);
-            }
+            var extensions = this.testHostManager.GetTestPlatformExtensions(sourceList, TestPluginCache.Instance.PathToAdditionalExtensions).ToList();


.ToList(); [](start = 140, length = 10)

nit: use IEnumerable.

AbhitejJohn · 2017-04-13T10:45:33Z

src/Microsoft.TestPlatform.TestHostProvider/Hosting/DotnetTestHostManager.cs

        /// Dependency resolution for .net core projects are pivoted by the test project. Hence each test
        /// project must be launched in a separate test host process.
-        /// </remarks>
+        /// </remarks>InitializeExtensions


InitializeExtensions [](start = 22, length = 20)

cleanup?

AbhitejJohn · 2017-04-13T10:53:36Z

src/Microsoft.TestPlatform.Common/ExtensionFramework/Utilities/TestExtensions.cs

+        /// </param>
+        /// <returns>
+        /// The <see cref="Dictionary"/> of extensions discovered
+        /// </returns>


AddExtension [](start = 49, length = 12)

misleading name: It says Add but we are setting it. Is that intended?

AbhitejJohn · 2017-04-13T10:53:59Z

public class TestExtensions

What exactly is the responsibility of this class?

Refers to: src/Microsoft.TestPlatform.Common/ExtensionFramework/Utilities/TestExtensions.cs:16 in b0dc0f3. [](commit_id = b0dc0f3, deletion_comment = False)

Faizan2304 · 2017-04-13T10:57:30Z

src/Microsoft.TestPlatform.Common/ExtensionFramework/TestPluginCache.cs

+            {
+                this.TestExtensions = new TestExtensions();
+            }
+


Should we update our central cache?

Faizan2304 · 2017-04-13T10:57:51Z

src/Microsoft.TestPlatform.Common/ExtensionFramework/TestPluginCache.cs

+                return extensions;
+            }
+
+            this.GetTestExtensions<TPluginInfo, TExtension>(new List<string>() { extensionAssembly });


I don't think we need this line.

Faizan2304 · 2017-04-13T12:06:16Z

src/vstest.console/Processors/TestAdapterPathArgumentProcessor.cs

@@ -204,7 +214,7 @@ public ArgumentProcessorResult Execute()
        /// <returns> The list of test adapter and logger assemblies. </returns>
        internal virtual IEnumerable<string> GetTestAdaptersFromDirectory(string directory)


Do we need this function now? I think we can remove it and we should add logic to find extension in testplatform.UpdateTestAdapterPaths( string runSettings )

Faizan2304

👍 LGTM

Please add following scenario in your test matrix:

Run cpp tests
Pass logger through /TestAdapterpath

…y to modify runsettings.

…n2/vstest into TestPluginCacheRefactor

Faizan2304 · 2017-04-18T06:15:38Z

src/vstest.console/Processors/TestAdapterPathArgumentProcessor.cs


-                customAdaptersPath = Path.GetFullPath(argument);
-                if (!fileHelper.DirectoryExists(customAdaptersPath))
+                var testadapterPaths = argument.Split(new[] { ';' }, StringSplitOptions.RemoveEmptyEntries).ToList();


TestAdapterPath can not have more than one path. If we want to change the behavior then we need to set property
public override bool AllowMultiple => true; and user has to specify /Testadapterpath multiple times if he wants to use multiple Testadapterpath

Reverted to original behaviour.

AbhitejJohn · 2017-04-18T06:20:39Z

src/Microsoft.TestPlatform.Common/ExtensionFramework/Utilities/TestExtensions.cs


+        /// <summary>
+        /// Gets or sets a value indicating whether are test settings providers cached.
+        /// </summary>


Are we planning to move these out to their respective plugin managers? This is becoming a God class.

AbhitejJohn · 2017-04-18T06:26:03Z

Please file an issue for the two broad level issues that came out as part of this PR:

The regex pattern that is passed all over which isn't needed.
The God level TestExtensions.cs class that needs to transfer responsibility over to each of the plugin managers.

…hs in runsettings.

Faizan2304 · 2017-04-18T08:35:27Z

src/vstest.console/Processors/TestAdapterPathArgumentProcessor.cs

-                this.output.Warning(CommandLineResources.NoAdaptersFoundInTestAdapterPath, argument);
-                this.output.WriteLine(string.Empty, OutputLevel.Information);
-            }
+            this.commandLineOptions.TestAdapterPath = customAdaptersPath;


Can we please check uses of this. earlier it were taking single path but now u are passing list of paths

harshjain2 · 2017-04-18T10:30:34Z

@dotnet-bot test this please

codito · 2017-04-18T11:10:44Z

@dotnet-bot test this please

TestPlugin cache refactored.

92f2883

msftclas added the cla-not-required label Apr 12, 2017

harshjain2 requested review from Faizan2304, codito and mayankbansal018 April 12, 2017 23:50

Fixed build error.

b0dc0f3

Faizan2304 reviewed Apr 13, 2017

View reviewed changes

AbhitejJohn reviewed Apr 13, 2017

View reviewed changes

Faizan2304 reviewed Apr 13, 2017

View reviewed changes

Faizan2304 approved these changes Apr 13, 2017

View reviewed changes

Harsh Jain and others added 7 commits April 13, 2017 19:28

PR feedback.

bc70834

Merge branch 'master' into TestPluginCacheRefactor

7760729

Resolved conflicts.

11153a6

Merge branch 'master' into TestPluginCacheRefactor

1ceb468

Fixed priority of testadapterpath arg processor, updated functionalit…

eba4217

…y to modify runsettings.

Merge branch 'TestPluginCacheRefactor' of https://github.com/harshjai…

0a3ec5c

…n2/vstest into TestPluginCacheRefactor

Renamed method.

036771c

Faizan2304 reviewed Apr 18, 2017

View reviewed changes

Fixed default extensions on testhost side.

24dd852

AbhitejJohn reviewed Apr 18, 2017

View reviewed changes

Merge branch 'master' into TestPluginCacheRefactor

37c164e

Harsh Jain and others added 4 commits April 18, 2017 12:35

Revereted supporting multiple paths using single argument processor.

bce2c25

Added Default Extensions Path in testadapterpaths in runsettings.

6ff8afe

Reverted changes for adding Default Extensions Path in testadapterpat…

8b9fd52

…hs in runsettings.

Merge branch 'master' into TestPluginCacheRefactor

3992fcf

Faizan2304 reviewed Apr 18, 2017

View reviewed changes

Merge branch 'master' into TestPluginCacheRefactor

2c6bc1e

harshjain2 merged commit 2043969 into microsoft:master Apr 18, 2017

MarcoRossignoli mentioned this pull request Jan 16, 2020

Pass coverlet codebase in runsettings for inproc data collector initialization #2288

Merged

		@@ -113,7 +104,7 @@ public IEnumerable<string> PathToAdditionalExtensions
		{

		@@ -204,7 +214,7 @@ public ArgumentProcessorResult Execute()
		/// <returns> The list of test adapter and logger assemblies. </returns>
		internal virtual IEnumerable<string> GetTestAdaptersFromDirectory(string directory)

TestPlugin cache refactored. #709

TestPlugin cache refactored. #709

Uh oh!

Conversation

harshjain2 commented Apr 12, 2017

Uh oh!

msftclas commented Apr 12, 2017

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!

AbhitejJohn commented Apr 13, 2017

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!

Faizan2304 left a comment

Choose a reason for hiding this comment

Uh oh!

Faizan2304 Apr 18, 2017 • 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!

AbhitejJohn commented Apr 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harshjain2 commented Apr 18, 2017

Uh oh!

codito commented Apr 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Faizan2304 Apr 18, 2017 •

edited

Loading