Distinguish between a test host crash or hang. #3984
Conversation
| else if (!BlameIsEnabled()) | ||
| { | ||
| EqtTrace.Error("TestRequestSender: Aborting test run"); | ||
| LogErrorMessage(string.Format(CultureInfo.CurrentCulture, "The active test run was aborted. Use blame for more details.")); |
There was a problem hiding this comment.
You should always add new messages into the resx so that entries gets translated (here and in the else below).
| @@ -609,7 +612,19 @@ private void OnDiscoveryMessageReceived(ITestDiscoveryEventsHandler2 discoveryEv | |||
| OnDiscoveryAbort(discoveryEventsHandler, ex, false); | |||
| } | |||
| } | |||
|
|
|||
| private static bool BlameIsEnabled() | |||
There was a problem hiding this comment.
Personal preference but I like to prefix boolean method with Is, Can or "action" prefixes. So I would recommend IsBlameEnabled.
|
|
||
| private static bool BlameIsEnabled() | ||
| { | ||
| string? s = RunSettingsManager.Instance.ActiveRunSettings.SettingsXml; |
There was a problem hiding this comment.
s isn't really meaningful, please rename into runSettingsXml or runSettingsString. Or you could also remove the variable and pass it directly to GetDataCollectionRunSettings call.
| private static bool BlameIsEnabled() | ||
| { | ||
| string? s = RunSettingsManager.Instance.ActiveRunSettings.SettingsXml; | ||
| DataCollectionRunSettings dataCollectionRunSettings = XmlRunSettingsUtilities.GetDataCollectionRunSettings(s)!; |
There was a problem hiding this comment.
Looks like the method can return null, I would recommend to handle it rather than "expecting" result to not be null.
| return true; | ||
| } | ||
| } | ||
| return false; |
There was a problem hiding this comment.
Nit
| return false; | |
| return false; |
| @@ -609,7 +612,19 @@ private void OnDiscoveryMessageReceived(ITestDiscoveryEventsHandler2 discoveryEv | |||
| OnDiscoveryAbort(discoveryEventsHandler, ex, false); | |||
| } | |||
| } | |||
|
|
|||
| private static bool BlameIsEnabled() | |||
There was a problem hiding this comment.
Is the content of this method copy-pasted from somewhere else or is it something you came up with?
| if (reason != "Test host process crashed") | ||
| { | ||
| EqtTrace.Error("TestRequestSender: Aborting test run because {0}", reason); | ||
| LogErrorMessage(string.Format(CultureInfo.CurrentCulture, CommonResources.AbortedTestRun, reason)); |
There was a problem hiding this comment.
I'm wondering if we should suggest to use blame in this case too.
| else if (!BlameIsEnabled()) | ||
| { | ||
| EqtTrace.Error("TestRequestSender: Aborting test run"); | ||
| LogErrorMessage(string.Format(CultureInfo.CurrentCulture, "The active test run was aborted. Use blame for more details.")); |
There was a problem hiding this comment.
I was thinking about a message (for the 2nd part) that reads like This is likely caused by a crash or hang, use the blame argument to get more details..
|
|
||
| foreach (var item in dataCollectionRunSettings.DataCollectorSettingsList) | ||
| { | ||
| if (item.FriendlyName?.ToLowerInvariant() == "blame" && item.IsEnabled) |
There was a problem hiding this comment.
Change the order of the && to make it fail early on isEnabled == false, and check the equality by .Equals with OrdinalCaseInsensitive (or do we have an extension method for that already)?
| @@ -622,8 +642,24 @@ private void OnTestRunAbort(IInternalTestRunEventsHandler testRunEventsHandler, | |||
| SetOperationComplete(); | |||
|
|
|||
| var reason = GetAbortErrorMessage(exception, getClientError); | |||
There was a problem hiding this comment.
This could probably return a tuple, or out result with a boolean to indicate if we got default result or not. Rather than checking the literal text.
| else | ||
| { | ||
| EqtTrace.Error("TestRequestSender: Aborting test run"); | ||
| LogErrorMessage(string.Format(CultureInfo.CurrentCulture, "The active test run was aborted.")); |
There was a problem hiding this comment.
Mention that blame was enabled and that they should look at the blame messages below?
|
One general comment, maybe we could replace the |
Description
Distinguish between a test host crash or hang by make the blame message as error and remove the generic reason.
Related issue
Fixes #2593.
