-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
[Merge-on-Red] - Implement XML log fixer for Helix #80751
Conversation
…le it and have Helix use it as needed.
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
.Where(c => char.IsLetter(c)) | ||
.ToArray()); | ||
|
||
if (nextClosing.Equals(tags.Peek())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we flush all writes to the xUnit log? Otherwise a crash may end up with an incomplete log finishing with a partial close tag mismatching the corresponding open tag - I guess we may be able to work around it but I suspect it would require a more precise XML scanner - I leave it up to you and Jeremy to decide whether it's worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that would be an issue. For example, let's suppose we are running Methodical_d1.sh. If I click Ctrl-C midrun, it literally stops logging right then and there, so I don't think we would end up with this mismatching example.
using System.Collections.Generic; | ||
using System.Text; | ||
namespace XUnitWrapperLibrary; | ||
|
||
public class TestSummary | ||
{ | ||
readonly record struct TestResult(string Name, string ContainingTypeName, string MethodName, TimeSpan Duration, Exception? Exception, string? SkipReason, string? Output); | ||
// readonly record struct TestResult(string Name, string ContainingTypeName, string MethodName, TimeSpan Duration, Exception? Exception, string? SkipReason, string? Output); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably obsolete comment worth deleting. Just for my education, what is the benefit of using record
here, is that about something like automatically generated IComparer / Equals / GetHashCode
implementations or is there more to it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure there is a tangible benefit/degrade to using record here. It was already there so I opted to not touch it since I was able to adapt it easily :)
Also, yes that comment ought to be removed. I forgot about it when running some tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me overall assuming is passes in the lab ;-), thanks Ivan!
…rding skipped tests.
…where Mono was being used instead of CoreCLR.
…ft to candidate PR.
The XML fixer work item from #77735. This PR implements two main things: