Skip to content

Conversation

@ChadNedzlek
Copy link
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
Member

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

@ChadNedzlek
Copy link
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
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
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
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
Member

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

@BruceForstall
Copy link
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
Copy link
Contributor Author

ChadNedzlek commented Apr 30, 2021

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