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

Fix Azure Devops Reporter : new change assumes Python 3.6+ #7438

Merged
merged 3 commits into from
May 25, 2021

Conversation

MattGal
Copy link
Member

@MattGal MattGal commented May 24, 2021

It seems we don't test this codepath in default test runs, and the mentioned changes introduce a python 3.6-ism. The fix is simply to access the RegEx's groups via the older method in all cases.

To double check:

I'm validating this fix via both testing on python 3.5 in a Debian docker container (already done) and via applying my change on top of the commit that is the source of the failure via draft PR: #7437

@MattGal MattGal requested a review from ChadNedzlek May 24, 2021 17:02
@MattGal
Copy link
Member Author

MattGal commented May 24, 2021

Verified fixed

Copy link
Member

@garath garath left a comment

Choose a reason for hiding this comment

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

👍

@ChadNedzlek
Copy link
Member

Hrm... Perhaps we should include a testing queue that has a minimum version of python in the matrix so that people stop making this mistake? :-)

@MattGal
Copy link
Member Author

MattGal commented May 24, 2021

Hrm... Perhaps we should include a testing queue that has a minimum version of python in the matrix so that people stop making this mistake? :-)

We already do, it's Debian 9. This code path wasn't hit by non-remote-executor test paths.

@ChadNedzlek
Copy link
Member

It has uploaders for all the various formats. Isn't the path just... "I ran XUnit"? Or am I misunderstanding the issue?

@MattGal
Copy link
Member Author

MattGal commented May 24, 2021

It has uploaders for all the various formats. Isn't the path just... "I ran XUnit"? Or am I misunderstanding the issue?

by code path, I mean it has to not return on https://github.com/dotnet/arcade/blob/main/src/Microsoft.DotNet.Helix/Sdk/tools/azure-pipelines/reporter/formats/xunit.py#L26 and hit https://github.com/dotnet/arcade/blob/main/src/Microsoft.DotNet.Helix/Sdk/tools/azure-pipelines/reporter/formats/xunit.py#L27 to reproduce the bug.

@ChadNedzlek
Copy link
Member

Oohhhh!. It's "fake" XUnit stuff isn't fake enough. It doesn't have the bad escaping in it. Could we add one?

@ChadNedzlek
Copy link
Member

Wait, it doesn't have a fake XUnit file... Now I'm extra confused. It has JUnit and TRX.

Maybe one of the "real" tests needs to include a newline?

…ntioned changes introduce a python 3.6-ism. The fix is simply to access the RegEx's groups via the older method in all cases.
@MattGal
Copy link
Member Author

MattGal commented May 25, 2021

@ChadNedzlek took some wrangling but I think I got it. I had to make a testResults.xml and include it like the JUnit/ TRX parts since a real failing xunit test in Arcade (as I opine it should) fails the work item and thus the job.

https://dev.azure.com/dnceng/public/_build/results?buildId=1154197&view=logs&j=50e98f4f-3b28-577a-6481-eda1b1f9a1d6 is the run

@MattGal MattGal merged commit f22435d into dotnet:main May 25, 2021
@MattGal
Copy link
Member Author

MattGal commented May 25, 2021

Now successfully prints unicode and the other special characters covered in the reporter:
image

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.

3 participants