Skip to content

Unescape the XmlEscape method from XUnit - #7323

Merged
ChadNedzlek merged 1 commit into
dotnet:mainfrom
ChadNedzlek:unescape-xunit-messages
Apr 30, 2021
Merged

Unescape the XmlEscape method from XUnit#7323
ChadNedzlek merged 1 commit into
dotnet:mainfrom
ChadNedzlek:unescape-xunit-messages

Conversation

@ChadNedzlek

Copy link
Copy Markdown
Contributor

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.

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.
@ChadNedzlek
ChadNedzlek force-pushed the unescape-xunit-messages branch from fe3ed0f to 02c698a Compare April 30, 2021 00:02
@alexperovich

Copy link
Copy Markdown
Member

note: i didn't parse and execute the regex in my head, i'm assuming it works.

@ChadNedzlek

Copy link
Copy Markdown
Contributor Author

It's a relatively boring one... either \x followed by 2-4 digits, or \[something other than x]. Technically \x followed by 3 digits isn't right (the weird escaping is always 2 or 4), but I don't want to make it even more complicated.

It would be SO useful if regex had \h for "hex digit"

@BruceForstall

Copy link
Copy Markdown
Contributor

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"?

@ChadNedzlek

Copy link
Copy Markdown
Contributor Author

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.

@ChadNedzlek

Copy link
Copy Markdown
Contributor Author

@BruceForstall is the question specifically about coreclr? Do they do something strange that we'd need to take into account?

@ViktorHofer

Copy link
Copy Markdown
Member

Just to make sure, this code will not run for trx test results?

@BruceForstall

Copy link
Copy Markdown
Contributor

@BruceForstall is the question specifically about coreclr? Do they do something strange that we'd need to take into account?

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.

@ChadNedzlek

ChadNedzlek commented Apr 30, 2021

Copy link
Copy Markdown
Contributor Author

Sure:
Before the fix
After the fix

(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)

@ChadNedzlek

Copy link
Copy Markdown
Contributor Author

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.

@ChadNedzlek

Copy link
Copy Markdown
Contributor Author

on the plus side, since we are calling the JSON directly for the Analysis check, we will render it correctly with leading spaces.

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.

4 participants