-
Notifications
You must be signed in to change notification settings - Fork 253
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
[WIP] In assembly parallel #296
Conversation
…hronous. Improve E2E test sample.
Remove logic to add stdout/stderr messages to last run test result since it is error prone. We won't know if the test method that ran last is the same class for which cleanup failed.
Todo: Add unit tests.
Class level parallel and E2E tests.
Support for DoNotParallelize at assembly level. Fixed up a loc'ing issue with test assembly being loaded in the main app domain. Runsettings parallel level taking precedence over assembly level setting.
Few more code changes to align with the spec.
Need to add UTs for the main functionality in TestExecutionManager.
In assembly parallel: Few more updates and UTs
scripts/test.ps1
Outdated
if ($lastExitCode -ne 0) | ||
{ | ||
throw "Tests failed." | ||
} |
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.
nit: indent
scripts/test.ps1
Outdated
if ($lastExitCode -ne 0) | ||
{ | ||
throw "Tests failed." | ||
} |
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.
nit: indent
using Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Helpers; | ||
using Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.ObjectModel; | ||
|
||
internal class TestAssemblySettingsProvider : MarshalByRefObject |
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.
Should we build an abstraction for TestSource
? There seem to be multiple data associated with a test assembly and it is currently fragmented. E.g. we pass in source
, maintain a state isDeploymentCompleted
etc.. And now we're adding parallelization related configuration. These are effectively a per source property.
The TestSource could be serializable.
@@ -295,36 +300,39 @@ public string RunClassCleanup() | |||
return null; | |||
} | |||
|
|||
try | |||
lock (this.testClassExecuteSyncObject) |
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 think we can skip the lock here since cleanups run at the end.
// Default test set is filtered based on user provided filter criteria | ||
IEnumerable<TestCase> testsToRun = Enumerable.Empty<TestCase>(); | ||
var filterExpression = this.TestMethodFilter.GetFilterExpression(runContext, frameworkHandle, out var filterHasError); | ||
if (!filterHasError) |
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.
Add check for filterExpression != null
@@ -113,6 +115,9 @@ | |||
<HintPath>..\..\..\packages\Microsoft.TestPlatform.ObjectModel.11.0.0\lib\portable-net45+win8+wpa81+wp8\Microsoft.VisualStudio.TestPlatform.ObjectModel.dll</HintPath> | |||
<Private>False</Private> | |||
</Reference> | |||
<Reference Include="System.Collections.Concurrent"> |
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.
Need to remove this before merge.
@@ -299,4 +299,7 @@ | |||
<data name="DiscoveryWarning" xml:space="preserve"> | |||
<value>[MSTest][Discovery][{0}] {1}</value> | |||
</data> | |||
<data name="TestParallelizationBanner" xml:space="preserve"> | |||
<value>MSTest Executor: Test Parallelization enabled. Parallel Level {0}, Parallel Mode {1}</value> |
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.
Need to update string before merge. It may not have MSTest Executor
.
/// Specification to disable parallelization. | ||
/// </summary> | ||
[AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = false)] | ||
public class DoNotParallelizeAttribute : Attribute |
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.
Suggest ExcludeFromParallel
attribute. It is similar to ExcludeFromCodeCoverage
. /cc: @pvlakshm
/// Specification for parallelization level for a test run. | ||
/// </summary> | ||
[AttributeUsage(AttributeTargets.Assembly, AllowMultiple = false)] | ||
public class TestParallelizationLevelAttribute : Attribute |
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.
Here's how current configuration looks like:
[assembly: TestParallelizationLevel(2)]
[assembly: TestParallelizationMode(TestParallelizationMode.MethodLevel)]
Few suggestions
[assembly: ParallelExecution(Workers: 5, Scope: ExecutionScope.Method)]
or,
[assembly: TestExecutionWorkers(2)]
[assembly: TestExecutionScope(ExecutionScope.Method)]
/// <summary> | ||
/// Test execution is sequential. This is default. | ||
/// </summary> | ||
None = 0, |
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.
Call it Sequence
?
In assembly parallel- PR comments
Todo: Modify unit tests.
In assembly parallel
In assembly parallel
…latest version yet.
Reverting the UWP sdk upgrade since PR build machines are not on the latest version yet.
@dotnet-bot: test Windows / Release Build please. |
Updating localized files.
Resource string updates.
…t we need a platform service wrapped around threads for this. I'm leaning to get this done in a separate PR cause its not very often we see tests requiring STA to be parallelized - They can use an STA attribute in that case.
In assembly parallel
@@ -124,20 +126,23 @@ public void RunAssemblyInitialize(TestContext testContext) | |||
throw new NullReferenceException(Resource.TestContextIsNull); | |||
} | |||
|
|||
// If assembly initialization is not done, then do it. | |||
if (!this.IsAssemblyInitializeExecuted) | |||
lock (this.assemblyInfoExecuteSyncObject) |
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.
Should check for IsAssemblyInitializeExecuted
before attempting to request lock.
// Default test set is filtered tests based on user provided filter criteria | ||
IEnumerable<TestCase> testsToRun = Enumerable.Empty<TestCase>(); | ||
var filterExpression = this.TestMethodFilter.GetFilterExpression(runContext, frameworkHandle, out var filterHasError); | ||
if (!filterHasError) |
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.
Should stop the process on filterHasError?
switch (parallelScope) | ||
{ | ||
case ExecutionScope.MethodLevel: | ||
queue = new ConcurrentQueue<IEnumerable<TestCase>>(parallelizableTestSet.Select(t => new[] { t })); |
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.
Can we avoid creating new array for each test case?
} | ||
} | ||
}, | ||
CancellationToken.None, |
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.
@AbhitejJohn In that case we will try to run all the tests even cancellation requested. Add check for global cancellation request while checking for queue.IsEmpty()?
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.
Create an issue for Console/Debug traces + Log.LogMessage are not associated with the right test case
.
} | ||
|
||
[TestMethodV1] | ||
[Ignore] |
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.
Create an issue and add as message.
|
||
[TestMethodV1] | ||
[Ignore] | ||
public void RunTestsForTestShouldRunTestsInTheParentDomainsApartmentState() |
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.
Add tests for parallel enabled with test case filter.
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 could but that would not improve code coverage. We do not do anything special for parallel. That test would be for filtering only. I think we can take that as part of filling in the test gaps.
…ed by locks. Was seeing intermittent failures in PR tests.
PR comments addressed.
@dotnet-bot test Windows / Release Build please |
Allow TranslationLayer to specify Diag parameters
This addresses #142 and is work in progress.
Todo:
Running Desktop Platform Service unit tests seem to be locking files and fails build.This is by design post 15.3 and isn't related to this change. The recommendation here is to turn offTest -> Test Settings -> Keep Execution Engine Alive
Performance numbers with 1.2.0 and the bits from this PR.
Discovery
Execution