-
Notifications
You must be signed in to change notification settings - Fork 325
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
Fix Few UWP VC++ unit tests are not executing #1649
Changes from all commits
6b563d8
658fa5c
005d4ff
4830785
e06d0df
b917d81
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Execution | ||
{ | ||
using System; | ||
using System.Collections; | ||
using System.Collections.Generic; | ||
using System.Collections.ObjectModel; | ||
using System.Diagnostics; | ||
|
@@ -13,7 +14,7 @@ namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Execution | |
using System.Linq; | ||
using System.Reflection; | ||
using System.Threading.Tasks; | ||
|
||
using CommunicationUtilities.Interfaces; | ||
using Microsoft.VisualStudio.TestPlatform.Common.ExtensionFramework; | ||
using Microsoft.VisualStudio.TestPlatform.Common.ExtensionFramework.Utilities; | ||
using Microsoft.VisualStudio.TestPlatform.Common.Interfaces; | ||
|
@@ -78,6 +79,11 @@ internal abstract class BaseRunTests | |
/// </summary> | ||
private RunConfiguration runConfiguration; | ||
|
||
/// <summary> | ||
/// The Serializer to clone testcase object incase of user input test source is package. E.g UWP scenario(appx/build.appxrecipe). | ||
/// </summary> | ||
private IDataSerializer dataSerializer; | ||
|
||
#endregion | ||
|
||
#region Constructor | ||
|
@@ -107,7 +113,8 @@ protected BaseRunTests(IRequestData requestData, | |
testRunEventsHandler, | ||
testPlatformEventSource, | ||
testCaseEventsHandler as ITestEventsPublisher, | ||
new PlatformThread()) | ||
new PlatformThread(), | ||
JsonDataSerializer.Instance) | ||
{ | ||
} | ||
|
||
|
@@ -131,7 +138,8 @@ protected BaseRunTests(IRequestData requestData, | |
ITestRunEventsHandler testRunEventsHandler, | ||
ITestPlatformEventSource testPlatformEventSource, | ||
ITestEventsPublisher testEventsPublisher, | ||
IThread platformThread) | ||
IThread platformThread, | ||
IDataSerializer dataSerializer) | ||
{ | ||
this.package = package; | ||
this.runSettings = runSettings; | ||
|
@@ -144,6 +152,7 @@ protected BaseRunTests(IRequestData requestData, | |
this.testPlatformEventSource = testPlatformEventSource; | ||
this.testEventsPublisher = testEventsPublisher; | ||
this.platformThread = platformThread; | ||
this.dataSerializer = dataSerializer; | ||
this.SetContext(); | ||
} | ||
|
||
|
@@ -557,6 +566,12 @@ private void RaiseTestRunComplete( | |
// Collecting Number of Adapters Used to run tests. | ||
this.requestData.MetricsCollection.Add(TelemetryDataConstants.NumberOfAdapterUsedToRunTests, this.ExecutorUrisThatRanTests.Count()); | ||
|
||
if (lastChunk.Any() && string.IsNullOrEmpty(package) == false) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: inaddition to package being null should we also compare with source.. also depending on package being null for a business logic seems a little odd.. ideally there shold be a flag setup upstream (to indicate this behavior) |
||
{ | ||
Tuple<ICollection<TestResult>, ICollection<TestCase>> updatedTestResultsAndInProgressTestCases = this.UpdateTestCaseSourceToPackage(lastChunk, null); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: can use var ? (also usage of Tuple indicates a missing struct) |
||
lastChunk = updatedTestResultsAndInProgressTestCases.Item1; | ||
} | ||
|
||
var testRunChangedEventArgs = new TestRunChangedEventArgs(runStats, lastChunk, Enumerable.Empty<TestCase>()); | ||
|
||
// Adding Metrics along with Test Run Complete Event Args | ||
|
@@ -569,10 +584,6 @@ private void RaiseTestRunComplete( | |
attachments, | ||
elapsedTime); | ||
testRunCompleteEventArgs.Metrics = this.requestData.MetricsCollection.Metrics; | ||
if (lastChunk.Any()) | ||
{ | ||
UpdateTestResults(lastChunk, null, this.package); | ||
} | ||
|
||
this.testRunEventsHandler.HandleTestRunComplete( | ||
testRunCompleteEventArgs, | ||
|
@@ -586,13 +597,19 @@ private void RaiseTestRunComplete( | |
} | ||
} | ||
|
||
private void OnCacheHit(TestRunStatistics testRunStats, ICollection<TestResult> results, ICollection<TestCase> inProgressTests) | ||
private void OnCacheHit(TestRunStatistics testRunStats, ICollection<TestResult> results, ICollection<TestCase> inProgressTestCases) | ||
{ | ||
if (this.testRunEventsHandler != null) | ||
{ | ||
UpdateTestResults(results, inProgressTests, this.package); | ||
if (string.IsNullOrEmpty(package) == false) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i see this being repeated.. opportunity to have it in a meaningful method.. |
||
{ | ||
Tuple<ICollection<TestResult>, ICollection<TestCase>> updatedTestResultsAndInProgressTestCases | ||
= this.UpdateTestCaseSourceToPackage(results, inProgressTestCases); | ||
results = updatedTestResultsAndInProgressTestCases.Item1; | ||
inProgressTestCases = updatedTestResultsAndInProgressTestCases.Item2; | ||
} | ||
|
||
var testRunChangedEventArgs = new TestRunChangedEventArgs(testRunStats, results, inProgressTests); | ||
var testRunChangedEventArgs = new TestRunChangedEventArgs(testRunStats, results, inProgressTestCases); | ||
this.testRunEventsHandler.HandleTestRunStatsChange(testRunChangedEventArgs); | ||
} | ||
else | ||
|
@@ -624,22 +641,50 @@ private bool TryToRunInSTAThread(Action action, bool waitForCompletion) | |
} | ||
|
||
|
||
private static void UpdateTestResults(IEnumerable<TestResult> testResults, IEnumerable<TestCase> testCases, string package) | ||
private Tuple<ICollection<TestResult>, ICollection<TestCase>> UpdateTestCaseSourceToPackage( | ||
ICollection<TestResult> testResults, | ||
ICollection<TestCase> inProgressTestCases) | ||
{ | ||
// Before sending the testresults back, update the test case objects with source provided by IDE/User. | ||
if (!string.IsNullOrEmpty(package)) | ||
|
||
EqtTrace.Verbose("BaseRunTests.UpdateTestCaseSourceToPackage: Update source details for testResults and testCases."); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove unnecessary empty lines |
||
var updatedTestResults = UpdateTestResults(testResults, package); | ||
|
||
var updatedInProgressTestCases = UpdateInProgressTests(inProgressTestCases, package); | ||
|
||
return new Tuple<ICollection<TestResult>, ICollection<TestCase>>(updatedTestResults, updatedInProgressTestCases); | ||
} | ||
|
||
private ICollection<TestResult> UpdateTestResults(ICollection<TestResult> testResults, string package) | ||
{ | ||
ICollection<TestResult> updatedTestResults = new List<TestResult>(); | ||
|
||
foreach (var testResult in testResults) | ||
{ | ||
foreach (var tr in testResults) | ||
{ | ||
tr.TestCase.Source = package; | ||
} | ||
var updatedTestResult = this.dataSerializer.Clone<TestResult>(testResult); | ||
updatedTestResult.TestCase.Source = package; | ||
updatedTestResults.Add(updatedTestResult); | ||
} | ||
|
||
// TestCases can be empty, enumerate on EmptyList then | ||
foreach (var tc in testCases ?? Enumerable.Empty<TestCase>()) | ||
{ | ||
tc.Source = package; | ||
} | ||
return updatedTestResults; | ||
} | ||
|
||
private ICollection<TestCase> UpdateInProgressTests(ICollection<TestCase> inProgressTestCases, string package) | ||
{ | ||
if (inProgressTestCases == null) | ||
{ | ||
return null; | ||
} | ||
|
||
ICollection<TestCase> updatedTestCases = new List<TestCase>(); | ||
foreach (var inProgressTestCase in inProgressTestCases) | ||
{ | ||
var updatedTestCase = this.dataSerializer.Clone<TestCase>(inProgressTestCase); | ||
updatedTestCase.Source = package; | ||
updatedTestCases.Add(updatedTestCase); | ||
} | ||
|
||
return updatedTestCases; | ||
} | ||
|
||
#endregion | ||
|
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.
Please write a comment on why we chose this approach, just for reference.
Also consider making it an extension method.
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.
There are lot of discussion over stackoverflow, how to do deep copy.
This very generic and easy to implement, But it is not available in NS14.
This required changes in all the class which are involved and difficult to maintain(complicated code).
As we are already using
JsonDataSeializer
to transfer the object over network. We can use the same functionality to do clone Which is well tested.