Exceptions flow to Translation layer #1434
Conversation
abhishkk
requested review from
acesiddhu,
smadala,
nigurr,
cltshivash,
singhsarab and
mayankbansal018
abhishkk
changed the title
| // In test run failure scenario, send failure message and completion event to transaltion layer. | ||
| if (!success) | ||
| { | ||
| var testMessagePayload = new TestMessagePayload { MessageLevel = TestMessageLevel.Error, Message = exception?.ToString() ?? "Exception in StartTestRun." }; |
There was a problem hiding this comment.
Suggestion for better error message in case success false comes from TestRequestManager (which today is not possible) #Closed
There was a problem hiding this comment.
| { | ||
| EqtTrace.Error("ExecuteArgumentProcessor: failed to execute argument process: {0}", ex); | ||
| this.Output.Error(false, ex.Message); | ||
| result = ArgumentProcessorResult.Fail; | ||
|
|
||
| // Send inner exception only when its message is different to avoid duplicate. | ||
| if (ex is TestPlatformException && |
There was a problem hiding this comment.
Instead of this we can do ex.ToString() in line 335 which will print inner exception. But in that case duplicacy will be there if inner exception and outer exception message are same. This happens for many scenarios in TestPlatformException
| @@ -190,28 +187,14 @@ public bool DiscoverTests(DiscoveryRequestPayload discoveryPayload, ITestDiscove | |||
| } | |||
| } | |||
| } | |||
| catch (Exception ex) | |||
| finally | |||
There was a problem hiding this comment.
finally code me null check
| { | ||
| throw; | ||
| } | ||
| testRunResultAggregator.MarkTestRunFailed(); |
There was a problem hiding this comment.
As testRunResultAggregator required only for console scenarios. I think we can remove this?
| [TestMethod] | ||
| public void DesignModeClientConnectShouldSendTestMessageAndDiscoverCompleteOnTestPlatformExceptionInDiscovery() | ||
| { | ||
| var payload = new DiscoveryRequestPayload(); |
| @@ -147,6 +147,81 @@ public void ExecuteShouldExitWithErrorOnResponseFileException() | |||
| Assert.AreEqual(1, exitCode, "Response File Exception execution should exit with error."); | |||
| } | |||
|
|
|||
| [TestMethod] | |||
| public void ExecuteShouldNotThrowSettingsExceptionButLogOutput() | |||
| { | |||
| @@ -1,28 +1,22 @@ | |||
| // Copyright (c) Microsoft Corporation. All rights reserved. | |||
| // Licensed under the MIT license. See LICENSE file in the project root for full license information. | |||
|
|
|||
| using System.ComponentModel; | |||
| using System.Xml; | |||
There was a problem hiding this comment.
nit: move these as well below?
| @@ -111,7 +111,6 @@ internal UriDataAttachment Clone(string baseDirectory, bool useAbsoluteUri) | |||
| { | |||
| Debug.Assert(!string.IsNullOrEmpty(baseDirectory), "'baseDirectory' is null or empty"); | |||
| Debug.Assert(baseDirectory == baseDirectory.Trim(), "'baseDirectory' contains whitespace at the ends"); | |||
| Debug.Assert(Path.IsPathRooted(baseDirectory), "'baseDirectory' is not a rooted path"); | |||
There was a problem hiding this comment.
Assertion if to test assumption, here it says that this method expects basedirectory to be rooted, if that is not the case, then may be change to to that it should not be null or empty.
Removal should only happen when we are not at all needing baseDirectory
| UriDataAttachment uriDataAttachment = attachment as UriDataAttachment; | ||
| if (uriDataAttachment != null) | ||
| { | ||
| Debug.Assert(uriDataAttachment.Uri.IsAbsoluteUri, "'collectorDataEntry' contains a URI data attachment with a relative URI"); |
There was a problem hiding this comment.
Same goes here, if can pick attachments, from relative URI, the change it that this path should not be null/empty
Description
Known exceptions like TestPlatformException, SettingsException, InvalidOperationException curerntly don't flow to Tralsation layer. Exceptions are eaten up in TestRequestManager.
Changes are to allow exceptions to flow to Translation layer.
Related issue
#1430