Optimizations for test case serialization. #103
Conversation
Introduced two flags to control SourceInformation and Serialization of xunit test case as a TestProperty in VSTestCase object. Serialization is enabled for Discovery always and enabled for Run All or Run Specific test operation only if the test run is triggered from an IDE. Moved Discovery (in case of Run All flow) to use the same XunitFrontController as execution of an assembly. This removed the need for Serialize of XunitTestCase in Discovery[execution] followed by a Deserialization in actual Execution. Since the ITestCase objects are MarshalByRef, optimized the code in VsExecutionSink to lookup a VSTestCase based on ITestCase.UniqueId instead of entire ITestCase objects.
|
Hi @codito, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! The agreement was validated by .NET Foundation and real humans are currently evaluating your PR. TTYL, DNFBOT; |
|
Tests in |
|
This is a substantial re-write. Can you please provide an overview of what you've done and why? Also, can you please stop arbitrarily reformatting the file, and following our existing coding conventions? Thanks. |
| var fqTestMethodName = $"{xunitTestCase.TestMethod.TestClass.Class.Name}.{xunitTestCase.TestMethod.Method.Name}"; | ||
| var result = new TestCase(fqTestMethodName, uri, source) { DisplayName = Escape(xunitTestCase.DisplayName) }; | ||
| result.SetPropertyValue(VsTestRunner.SerializedTestCaseProperty, serializedTestCase); |
There was a problem hiding this comment.
Issue: XunitTestCase property is required only for scenarios where VSTest can send a TestCase object later (e.g. Discovery in IDE, followed by Run Selected tests). A command line test run is a fire-and-forget operation, test property need not be serialized in this case.
Solution: add a RequireXunitTestProperty flag to control this.
|
|
||
| result = testCases.Where(tc => tc.Key.UniqueID == testCase.UniqueID).Select(kvp => kvp.Value).FirstOrDefault(); |
There was a problem hiding this comment.
Issue: the test cases here are Proxy objects, comparison operations are costly since they require cross appdomain calls. This function gets called quite frequently (every testcase start, end).
Solution: ITestCase.UniqueID identifies a test case, we built a map with <UniqueID, VSTestCase>. All the lookup operations are O(1) and do not require cross appdomain calls.
| /// Provides contextual information on a test run/discovery based on runsettings | ||
| /// or the invocation (execution, discovery). | ||
| /// </summary> | ||
| public struct TestPlatformContext |
There was a problem hiding this comment.
In #100, support for checking context was added. In this PR, we are adding another context on when to serialize ITestCase. This object holds all such requirements from vstest for decisions during discovery and execution.
| @@ -85,9 +85,22 @@ void ITestDiscoverer.DiscoverTests(IEnumerable<string> sources, IDiscoveryContex | |||
| RunSettingsHelper.ReadRunSettings(discoveryContext?.RunSettings?.SettingsXml); | |||
| #endif | |||
|
|
|||
| var testPlatformContext = new TestPlatformContext | |||
There was a problem hiding this comment.
In all 3 operations from VSTest: Discover, Run All, Run Specific tests, this context is defined. The optimizations are as follows:
| Discovery | Run All | Run Specific | |
|---|---|---|---|
| RequiresSourceInformation | IDE - Yes, CLI - No | IDE - Yes, CLI - No | IDE - Yes, CLI - No |
| RequireXunitTestProperty | IDE - Yes, CLI - Yes* | IDE - Yes, CLI - No | IDE - Yes, CLI - No |
CLI can be optimized since it is fire and forget. Except for Discovery* since Run Specific Test in CLI also calls Discovery first and then followed by an execution.
| @@ -122,7 +143,17 @@ void ITestExecutor.RunTests(IEnumerable<string> sources, IRunContext runContext, | |||
| .Where(file => !platformAssemblies.Contains(Path.GetFileName(file)))); | |||
| } | |||
|
|
|||
| RunTests(runContext, frameworkHandle, logger, () => GetTests(sources, logger, runContext, RunSettingsHelper.DesignMode)); | |||
There was a problem hiding this comment.
Issue: in case of Run All, a Discovery was first triggered in separate appdomain, tests were serialized, Execution is triggered in another appdomain.
Solution: in case of Run All, delay the actual discovery. First create an appdomain (XunitFrontController), then do a discovery within that, serialization is not required (if RequireXunitTestProperty is false), do the execution within same appdomain.
Since it is the same assembly, discovery and execution may happen in same appdomain (similar to functionality of xunit.console).
| { | ||
| if (cancelled) | ||
| if (!DiscoverTestsInSource(framework, logger, testPlatformContext, visitorFactory, visitComplete, assemblyFileName, shadowCopy, configuration)) |
There was a problem hiding this comment.
DiscoverTestsInSource just extracts the common parts in Discovery and Discovery in case of Run All.
| } | ||
| } | ||
| catch (Exception e) | ||
| if (!IsXunitTestAssembly(assemblyFileName)) |
There was a problem hiding this comment.
There is no new code in this method, just refactoring to allow reuse.
| var configuration = LoadConfiguration(assemblyFileName); | ||
| var fileName = Path.GetFileNameWithoutExtension(assemblyFileName); | ||
| var shadowCopy = configuration.ShadowCopyOrDefault; | ||
|
|
||
| try | ||
| var diagnosticSink = new DiagnosticMessageSink(logger, fileName, configuration.DiagnosticMessagesOrDefault); | ||
| using (var framework = new XunitFrontController(AppDomainDefaultBehavior, |
There was a problem hiding this comment.
This XunitFrontController is only created for Discovery. We provide an existing controller to DiscoverTestsInSource in case of Discovery during Run All.
| (source, discoverer, discoveryOptions) => new VsExecutionDiscoverySink(() => cancelled), | ||
| (source, discoverer, discoveryOptions, visitor) => | ||
| { | ||
| if (discoveryOptions.GetDiagnosticMessagesOrDefault()) |
There was a problem hiding this comment.
This logic and the filtering logic is moved to RunTestsInAssembly to allow reuse of same XunitFrontController for discovery and execution.
| .ToDictionary(tc => tc.xunit, tc => tc.vs); | ||
| var testCasesMap = new Dictionary<string, TestCase>(); | ||
| var testCases = new List<ITestCase>(); | ||
| if (runInfo.TestCases == null || !runInfo.TestCases.Any()) |
There was a problem hiding this comment.
This indicates discovery has been delayed for a Run All operation; we already have the controller here. We do the discovery and filtering.
| { | ||
| var xunitTestCases = runInfo.TestCases.Select(tc => new { vs = tc, xunit = Deserialize(logger, controller, tc) }) |
There was a problem hiding this comment.
Issue: in case of Run All; there was an extra cost of Serialize after discovery and Deserialize before execution.
Solution: remove deserialize and make serialize conditional based on RequireXunitTestProperty.
| { | ||
| // PERF: the filteredTestCase.TestCase object is proxy to ITestCase in the discovery appdomain. | ||
| // We're using the UniqueId only (cached in this appdomain). | ||
| testCasesMap.Add(filteredTestCase.UniqueId, filteredTestCase.VSTestCase); |
There was a problem hiding this comment.
We pre-create a map of UniqueID, VSTestCase to allow faster lookups in VSExecutionSink.
| // PERF: note that TestCase is a proxy to actual object in the discovery appdomain. UniqueId is used multiple times, | ||
| // hence cached to reduce cross-appdomain access cost. | ||
| TestCase = testCase; | ||
| UniqueId = testCase.UniqueID; |
There was a problem hiding this comment.
We use UniqueID several times later, create a copy once since it is a cross domain access.
This PR does three things:
Added more details of issues and proposed solutions along side source changes as comments in this PR.
Sorry, this was not intentional. I will fix it in the next commit to this PR (along with any review feedback). |
|
@bradwilson @onovotny please let me know if there's any more feedback on these changes. |
|
I cleaned up the conflicts and merged the result. Thanks! |
Introduced two flags to control SourceInformation and Serialization of xunit test case
as a TestProperty in VSTestCase object. Serialization is enabled for Discovery always and
enabled for Run All or Run Specific test operation only if the test run is triggered from
an IDE.
Moved Discovery (in case of Run All flow) to use the same XunitFrontController as execution
of an assembly. This removed the need for Serialize of XunitTestCase in Discovery[execution]
followed by a Deserialization in actual Execution. A side effect of this is: if multiple assemblies are specified, for each assembly
{discovery, execution}be sequential instead of{discovery of all}followed by{execution of all}. This behavior is same asxunit.consolerunner.Since the ITestCase objects are MarshalByRef, optimized the code in VsExecutionSink to lookup
a VSTestCase based on ITestCase.UniqueId instead of entire ITestCase objects since each access to
ITestCasehas the cost of cross-appdomain proxy call.