-
Notifications
You must be signed in to change notification settings - Fork 378
Unescape the XmlEscape method from XUnit #7323
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
Conversation
XUnit applies some custom escaping to this particular field. This code is the closest we can come to correctly reversing it so that it can render normally. It will produce incorrect results if a control character is followed by valid hex digits, but there is no way to avoid that, and it should be relatively rare for raw control characters to appear in error messages.
fe3ed0f to
02c698a
Compare
|
note: i didn't parse and execute the regex in my head, i'm assuming it works. |
|
It's a relatively boring one... either It would be SO useful if regex had \h for "hex digit" |
|
I'm curious: do you have a way to test that this, plus some coreclr test that fails, now produces the correct AzDO output UI screen, now without the extra escaped "\n"? |
|
The build itself will do it. The arcade build includes failures. Right now Azure DevOps is exploding a bit. But you can see a previous run with this code (and an intentionally failing test full of unicode stuff) at https://dev.azure.com/dnceng/public/_build/results?buildId=1115034&view=ms.vss-test-web.build-test-results-tab&runId=33968880&resultId=100129&paneView=debug Dev Ops doesn't respect the alignment spaces, but it's a sight better than full of backslashes. |
|
@BruceForstall is the question specifically about coreclr? Do they do something strange that we'd need to take into account? |
|
Just to make sure, this code will not run for trx test results? |
Well, I don't understand the whole pipeline and context here, so don't understand what would be "strange". You linked a case with the "fixed" version. Do you have an example (link) of the same failure without your change? I'm just wondering what the UI change looks like before and after. I don't understand how this change will fix the "\n" issue. |
|
Sure: (Note: The asserted strings are somewhat changed, I wanted to test extra Unicode characters, so I don't have a clear 1 to 1 comparison, because it's not checked in yet) |
|
It's annoying that AzDO is trimming the leading spaces here (they are present in the JSON response, so it's a render time issue for them), but it's significantly nicer than all the escaped stuff. |
|
on the plus side, since we are calling the JSON directly for the Analysis check, we will render it correctly with leading spaces. |
XUnit applies some custom escaping to this particular field.
This code is the closest we can come to correctly reversing it so
that it can render normally.
It will produce incorrect results if a control character is followed
by valid hex digits, but there is no way to avoid that, and it should
be relatively rare for raw control characters to appear in error messages.