Porting fix for Design mode clients hang for errors #1451
Conversation
abhishkk
changed the title
| this.LogMessage(TestMessageLevel.Error, exception.ToString()); | ||
|
|
||
| // Send a run complete to caller. Similar logic is also used in ParallelProxyExecutionManager.StartTestRunOnConcurrentManager | ||
| // Aborted is `true`: in case of parallel run (or non shared host), an aborted message ensures another execution manager | ||
| // created to replace the current one. This will help if the current execution manager is aborted due to irreparable error | ||
| // and the test host is lost as well. | ||
| var completeArgs = new TestRunCompleteEventArgs(null, false, true, exception, new Collection<AttachmentSet>(), TimeSpan.Zero); | ||
| var completeArgs = new TestRunCompleteEventArgs(null, false, true, null, new Collection<AttachmentSet>(), TimeSpan.Zero); |
There was a problem hiding this comment.
We are not sending exception in TestRunCompleteEvent here to avoid duplicacy. (as we are sending error as separate message.)
| this.LogMessage(TestMessageLevel.Error, exception.ToString()); | ||
|
|
||
| // Send a run complete to caller. Similar logic is also used in ParallelProxyExecutionManager.StartTestRunOnConcurrentManager | ||
| // Aborted is `true`: in case of parallel run (or non shared host), an aborted message ensures another execution manager | ||
| // created to replace the current one. This will help if the current execution manager is aborted due to irreparable error | ||
| // and the test host is lost as well. | ||
| var completeArgs = new TestRunCompleteEventArgs(null, false, true, exception, new Collection<AttachmentSet>(), TimeSpan.Zero); | ||
| var completeArgs = new TestRunCompleteEventArgs(null, false, true, null, new Collection<AttachmentSet>(), TimeSpan.Zero); | ||
| var testRunCompletePayload = new TestRunCompletePayload { TestRunCompleteArgs = completeArgs }; |
There was a problem hiding this comment.
Can this change is IDE behavior? What if IDE was displaying some warning if it receives Exception in TestRunComplete.
I'm not saying that this change is not correct, but can me sure IDE honors it as well?
There was a problem hiding this comment.
We are sending TestMessage of error which IDE will show as error. After that we don't need to send exception in TestRunComplete. This change is done for IDE only as IDE was showing error twice in this case, once because of TestMessage of error. and second because of exception in TestRunComplete.
| // Send discovery complete. Similar logic is also used in ProxyDiscoveryManager.DiscoverTests. | ||
| // Differences: | ||
| // Total tests must be zero here since parallel discovery events handler adds the count | ||
| // Keep `lastChunk` as null since we don't want a message back to the IDE (discovery didn't even begin) | ||
| // Set `isAborted` as true since we want this instance of discovery manager to be replaced | ||
| var discoveryCompleteEventsArgs = new DiscoveryCompleteEventArgs(-1, true); | ||
|
|
||
| this.GetHandlerForGivenManager(proxyDiscoveryManager).HandleDiscoveryComplete(discoveryCompleteEventsArgs, null); | ||
| handler.HandleDiscoveryComplete(discoveryCompleteEventsArgs, null); |
There was a problem hiding this comment.
Does the handler's "HandleDiscoveryComplete" calls HandRawMessage for discovery complete?
There was a problem hiding this comment.
No. we don't need to send handleRawMessage of complete in case of parallel as handleRawMessage is sent itself by parallel discovery manager when all managers completes discovery.
| // Send a run complete to caller. Similar logic is also used in ProxyExecutionManager.StartTestRun | ||
| // Differences: | ||
| // Aborted is sent to allow the current execution manager replaced with another instance | ||
| // Ensure that the test run aggregator in parallel run events handler doesn't add these statistics | ||
| // (since the test run didn't even start) | ||
| var completeArgs = new TestRunCompleteEventArgs(null, false, true, null, new Collection<AttachmentSet>(), TimeSpan.Zero); | ||
| this.GetHandlerForGivenManager(proxyExecutionManager).HandleTestRunComplete(completeArgs, null, null, null); | ||
| handler.HandleTestRunComplete(completeArgs, null, null, null); |
There was a problem hiding this comment.
Same comment as in parallel discovery
There are few error scenarios in which we were not sending discovery/run completion event and thus causing Design mode to hang.
In this fix, sending discovery complete for those scenarios.
15.6 PR: #1444