Fix Few UWP VC++ unit tests are not executing #1649
Conversation
| @@ -152,6 +152,13 @@ public string SerializePayload(string messageType, object payload, int version) | |||
| return JsonConvert.SerializeObject(message); | |||
| } | |||
|
|
|||
| /// <inheritdoc/> | |||
There was a problem hiding this comment.
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.
There are lot of discussion over stackoverflow, how to do deep copy.
- https://stackoverflow.com/questions/129389/how-do-you-do-a-deep-copy-of-an-object-in-net-c-specifically
- https://stackoverflow.com/questions/78536/deep-cloning-objects
- BinaryFormatter
This very generic and easy to implement, But it is not available in NS14. - 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.
| /// <inheritdoc/> | ||
| public T Clone<T>(T obj) | ||
| { | ||
| var stringObj = this.Serialize<T>(obj); |
There was a problem hiding this comment.
Suggestion: Check for ICloneable implemention and if so use it. else fallback
There was a problem hiding this comment.
Please see #1649 (comment)
| { | ||
| tr.TestCase.Source = package; | ||
| } | ||
| tr.TestCase.Source = package; |
There was a problem hiding this comment.
tr.TestCase.Source = package; [](start = 16, length = 29)
Is this safe ?
There was a problem hiding this comment.
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.
| @@ -557,6 +566,12 @@ private void SetAdapterLoggingSettings() | |||
| // 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.
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)
| @@ -557,6 +566,12 @@ private void SetAdapterLoggingSettings() | |||
| // Collecting Number of Adapters Used to run tests. | |||
| this.requestData.MetricsCollection.Add(TelemetryDataConstants.NumberOfAdapterUsedToRunTests, this.ExecutorUrisThatRanTests.Count()); | |||
|
|
|||
| if (lastChunk.Any() && string.IsNullOrEmpty(package) == false) | |||
| { | |||
| Tuple<ICollection<TestResult>, ICollection<TestCase>> updatedTestResultsAndInProgressTestCases = this.UpdateTestCaseSourceToPackage(lastChunk, null); | |||
There was a problem hiding this comment.
nitpick: can use var ? (also usage of Tuple indicates a missing struct)
| { | ||
| if (this.testRunEventsHandler != null) | ||
| { | ||
| UpdateTestResults(results, inProgressTests, this.package); | ||
| if (string.IsNullOrEmpty(package) == false) |
There was a problem hiding this comment.
i see this being repeated.. opportunity to have it in a meaningful method..
| if (!string.IsNullOrEmpty(package)) | ||
|
|
||
| EqtTrace.Verbose("BaseRunTests.UpdateTestCaseSourceToPackage: Update source details for testResults and testCases."); | ||
|
|
There was a problem hiding this comment.
remove unnecessary empty lines
|
@cltshivash I have to wait for one hour for PR checks to pass if I address the PR comments. As there no behavior change comments I'm going ahead with merge. Will address the comments in separate PR. |
|
Created #1651 to fix nits. |
Description
RCA
Changing the testcase's object source for inprogress tests for clients(VS Test explorer) causing the issue in UWP C++ scenario as the adapter and testplatform share same testcase object by changing the testcase source the testcase becoming invalid in UWP C++ adapter.
Related issue
#1642