Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix Few UWP VC++ unit tests are not executing #1649

Merged
merged 6 commits into from
Jun 18, 2018
Merged

Fix Few UWP VC++ unit tests are not executing #1649

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6b563d8
Fix Few UWP VC++ unit tests are not executing
smadala Jun 15, 2018
658fa5c
Add updated test to list
smadala Jun 15, 2018
005d4ff
Refactor BaseRunTestsTests
smadala Jun 17, 2018
4830785
Add test for BaseRunTestsTests.cs
smadala Jun 17, 2018
e06d0df
Copy test results and add tests for serializer
smadala Jun 18, 2018
b917d81
Use version 2 protocol to clone
smadala Jun 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/Microsoft.TestPlatform.CommunicationUtilities/Interfaces/IDataSerializer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,5 +46,13 @@ public interface IDataSerializer
/// <param name="version">version to be sent</param>
/// <returns>Raw Serialized message</returns>
string SerializePayload(string messageType, object payload, int version);

/// <summary>
/// Creates cloned object for given object.
/// </summary>
/// <typeparam name="T"> The type of object to be cloned. </typeparam>
/// <param name="obj">Object to be cloned.</param>
/// <returns>Newly cloned object.</returns>
T Clone<T>(T obj);
}
}
7 changes: 7 additions & 0 deletions src/Microsoft.TestPlatform.CommunicationUtilities/JsonDataSerializer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,13 @@ public string SerializePayload(string messageType, object payload, int version)
return JsonConvert.SerializeObject(message);
}

/// <inheritdoc/>
Copy link
Contributor

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.

Copy link
Contributor Author

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.

  1. BinaryFormatter
    This very generic and easy to implement, But it is not available in NS14.
  2. ICloneable
    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.

public T Clone<T>(T obj)
{
var stringObj = this.Serialize<T>(obj);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Check for ICloneable implemention and if so use it. else fallback

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see #1649 (comment)

return this.Deserialize<T>(stringObj);
}

/// <summary>
/// Serialize an object to JSON using default serialization settings.
/// </summary>
Expand Down
52 changes: 37 additions & 15 deletions src/Microsoft.TestPlatform.CrossPlatEngine/Execution/BaseRunTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -571,7 +571,7 @@ private void RaiseTestRunComplete(
testRunCompleteEventArgs.Metrics = this.requestData.MetricsCollection.Metrics;
if (lastChunk.Any())
{
UpdateTestResults(lastChunk, null, this.package);
UpdateTestResultsAndInProgressTests(lastChunk, null, this.package);
}

this.testRunEventsHandler.HandleTestRunComplete(
Expand All @@ -586,13 +586,13 @@ 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);
inProgressTestCases = UpdateTestResultsAndInProgressTests(results, inProgressTestCases, this.package);

var testRunChangedEventArgs = new TestRunChangedEventArgs(testRunStats, results, inProgressTests);
var testRunChangedEventArgs = new TestRunChangedEventArgs(testRunStats, results, inProgressTestCases);
this.testRunEventsHandler.HandleTestRunStatsChange(testRunChangedEventArgs);
}
else
Expand Down Expand Up @@ -624,22 +624,44 @@ private bool TryToRunInSTAThread(Action action, bool waitForCompletion)
}


private static void UpdateTestResults(IEnumerable<TestResult> testResults, IEnumerable<TestCase> testCases, string package)
private static ICollection<TestCase> UpdateTestResultsAndInProgressTests(IEnumerable<TestResult> testResults, ICollection<TestCase> inProgressTestCases, string package)
{

// No change required to testcases and testresults.
if (string.IsNullOrEmpty(package))
{
return inProgressTestCases;
}

EqtTrace.Verbose("BaseRunTests.UpdateTestResultsAndInProgressTests: Update source details for testResults and testCases.");

// Before sending the testresults back, update the test case objects with source provided by IDE/User.
if (!string.IsNullOrEmpty(package))
foreach (var tr in testResults)
{
foreach (var tr in testResults)
{
tr.TestCase.Source = package;
}
tr.TestCase.Source = package;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tr.TestCase.Source = package; [](start = 16, length = 29)

Is this safe ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this not safe. As we discussed offline, currently there is no known scenario to which break because of not copying(better performance). However it is better to copy the test results(which adapter creates) to make sure correctness over performance.

I have made changes to copy the test results too.

}

// TestCases can be empty, enumerate on EmptyList then
foreach (var tc in testCases ?? Enumerable.Empty<TestCase>())
{
tc.Source = package;
}
return UpdateInProgressTests(inProgressTestCases, package);
}

private static ICollection<TestCase> UpdateInProgressTests(ICollection<TestCase> inProgressTestCases, string package)
{
if (inProgressTestCases == null)
{
return null;
}

EqtTrace.Verbose("BaseRunTests.UpdateInProgressTests: Updating source for inprogress tests.");

ICollection<TestCase> updatedTestCases = new List<TestCase>();
foreach (var inProgressTestCase in inProgressTestCases)
{
var updatedTestCase = JsonDataSerializer.Instance.Clone<TestCase>(inProgressTestCase);
updatedTestCase.Source = package;
updatedTestCases.Add(updatedTestCase);
}

return updatedTestCases;
}

#endregion
Expand Down