-
Notifications
You must be signed in to change notification settings - Fork 323
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Test adapter loading strategy #3380
Conversation
src/Microsoft.TestPlatform.ObjectModel/TestAdapterLoadingStrategy.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.TestPlatform.ObjectModel/TestAdapterLoadingStrategy.cs
Outdated
Show resolved
Hide resolved
src/vstest.console/Processors/TestAdapterLoadingStrategyArgumentProcessor.cs
Outdated
Show resolved
Hide resolved
bdeb9d9
to
b58979f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments
src/Microsoft.TestPlatform.Common/Utilities/RunSettingsUtilities.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.TestPlatform.ObjectModel/RunSettings/RunConfiguration.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.TestPlatform.ObjectModel/TestAdapterLoadingStrategy.cs
Outdated
Show resolved
Hide resolved
src/vstest.console/Processors/TestAdapterLoadingStrategyArgumentProcessor.cs
Outdated
Show resolved
Hide resolved
src/vstest.console/Processors/TestAdapterLoadingStrategyArgumentProcessor.cs
Outdated
Show resolved
Hide resolved
src/vstest.console/Processors/TestAdapterLoadingStrategyArgumentProcessor.cs
Show resolved
Hide resolved
src/vstest.console/Processors/TestAdapterLoadingStrategyArgumentProcessor.cs
Outdated
Show resolved
Hide resolved
src/vstest.console/Processors/TestAdapterLoadingStrategyArgumentProcessor.cs
Outdated
Show resolved
Hide resolved
src/vstest.console/Processors/TestAdapterPathArgumentProcessor.cs
Outdated
Show resolved
Hide resolved
src/vstest.console/Processors/TestAdapterPathArgumentProcessor.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Marco Rossignoli <marco.rossignoli@gmail.com>
src/vstest.console/Processors/TestAdapterLoadingStrategyArgumentProcessor.cs
Outdated
Show resolved
Hide resolved
src/vstest.console/Processors/TestAdapterLoadingStrategyArgumentProcessor.cs
Outdated
Show resolved
Hide resolved
src/vstest.console/Processors/TestAdapterLoadingStrategyArgumentProcessor.cs
Outdated
Show resolved
Hide resolved
src/vstest.console/Processors/TestAdapterLoadingStrategyArgumentProcessor.cs
Show resolved
Hide resolved
src/vstest.console/Processors/TestAdapterPathArgumentProcessor.cs
Outdated
Show resolved
Hide resolved
@@ -91,15 +90,15 @@ public TestPlatform() | |||
PopulateExtensions(discoveryCriteria.RunSettings, discoveryCriteria.Sources); | |||
|
|||
// Initialize loggers. | |||
var loggerManager = TestEngine.GetLoggerManager(requestData); | |||
ITestLoggerManager loggerManager = TestEngine.GetLoggerManager(requestData); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we all agree, I could change the var
rules in the repo to favor explicit type when not obvious.
My opinion: I usually prefer var
in VS but that's true that var
in GitHub review is a bit of a pain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love var, but wgen it's not obvious it hard do discern that while reviewing. I would vote for yes.
src/vstest.console/Processors/TestAdapterLoadingStrategyArgumentProcessor.cs
Outdated
Show resolved
Hide resolved
src/vstest.console/Processors/TestAdapterLoadingStrategyArgumentProcessor.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few extra minor remarks but otherwise LGTM. Might be worth to have a 2nd approval before merging.
@Evangelink thanks for the review, I'll add the tests and ask for another review. |
Description
The code here is cherry-picked from our 17.1 release.
Work left
TestAdapterLoadingStrategy
from.runsettings
file.