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.

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

Aggregate Inner Exception Messages and Stacktraces #3784

Conversation

thomhurst
Copy link
Contributor

@thomhurst thomhurst commented Sep 7, 2024

Fix #3783

After:
image

I don't know if this actually works... I was trying to run build.cmd -pack -test -integrationTest but I just get:

C:\git\testfx\artifacts\toolset\restore.proj : error MSB4242: SDK Resolver Failure: "The SDK resolver "Microsoft.DotNet
.MSBuildSdkResolver" failed while attempting to resolve the SDK "Microsoft.DotNet.Arcade.Sdk". Exception: "System.IO.Fi
leNotFoundException: Workload manifest microsoft.net.sdk.aspire: 8.0.1/8.0.100 from workload version 9.0.100-preview.7.
24407.12 was not installed. Running "dotnet workload repair" may resolve this.

And I don't know how to resolve it. I've installed the aspire workload and ran workload repair but still getting it.

@MarcoRossignoli
Copy link
Contributor

@nohwnd can you review?

@nohwnd nohwnd self-assigned this Sep 9, 2024
Copy link
Member

@nohwnd nohwnd left a comment

Choose a reason for hiding this comment

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

We can merge as is (once the regex change question is answered).

But my preferred way of presenting multiple errors would be more along the lines of:

Failed with 3 errors:
[1] PlaygroundWrappedException: exception message
Stack Trace:
at ...
[2] Exception: exception message
Stack Trace:
at ...
[3] Exception: exception message
Stack Trace:
at ...

What do you think?

@@ -51,7 +51,7 @@ internal sealed partial class TerminalTestReporter : IDisposable
private bool _wasCancelled;

#if NET7_0_OR_GREATER
[GeneratedRegex(@$"^ at ((?<code>.+) in (?<file>.+):line (?<line>\d+)|(?<code1>.+))$", RegexOptions.ExplicitCapture, 1000)]
[GeneratedRegex(@$"^\s*at ((?<code>.+) in (?<file>.+):line (?<line>\d+)|(?<code1>.+))$", RegexOptions.ExplicitCapture, 1000)]
Copy link
Member

Choose a reason for hiding this comment

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

There should always be exactly 3 spaces in the stack frame, is this change needed or just done to make the regex more regexy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When testing I think the first line of inner exceptions weren't rendering properly without this change. Might be that I messed up formatting then

_terminalTestReporter.TestCompleted(
_assemblyName,
_targetFramework,
_shortArchitecture,
testNodeStateChanged.TestNode.DisplayName,
TestOutcome.Canceled,
duration,
errorMessage: cancelledState.Exception?.Message ?? cancelledState.Explanation,
cancelledState.Exception?.StackTrace,
errorMessage: string.Join(_environment.NewLine, exceptionMessages),
Copy link
Member

Choose a reason for hiding this comment

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

the receiving api can be changed to take array of strings, (we use it both here and behind a remote call in dotnet test so we don't want exception objects).

That will allow us to render the erros and stack traces under each other, so you don't have to jump back and forth.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. Do you want to take over this pr because I can't actually build it locally so it makes Dev a bit harder

Copy link
Member

Choose a reason for hiding this comment

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

Sure.

@nohwnd nohwnd added this to the MSTest 3.7 / Platform 1.5 milestone Sep 10, 2024
@thomhurst thomhurst closed this Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

InnerException's are lost in the Testing Platform output
3 participants