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

store stdout/stderr output from tests as bytes #19184

Merged
merged 2 commits into from
Jun 3, 2023

Conversation

tdyas
Copy link
Contributor

@tdyas tdyas commented May 28, 2023

Problem

Pants expects test output to be in UTF-8 even though this assumption is not a valid to make given that test output comes from process invocations. As described in #19121, this causes a UnicodeDecodeError under the conditions mentioned there.

Solution

Instead of storing a str, just store the binary output for stdout and stderr from the underlying Process to avoid string encoding issues as much as possible. When creating a message for output, only then decode the stings as UTF-8 and replace invalid characters.

The stdout and stderr attributes are renamed to stdout_bytes and stderr_bytes, respectively, Introduce stdout and stderr properties so that external plugins get a deprecation warning.

Fixes #19121.

@tdyas tdyas changed the title [WIPstore stdout/stderr output from tests as bytes [WIP] store stdout/stderr output from tests as bytes May 28, 2023
@tdyas tdyas added the category:bugfix Bug fixes for released features label May 28, 2023
Copy link
Sponsor Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Thanks!

@tdyas tdyas changed the title [WIP] store stdout/stderr output from tests as bytes store stdout/stderr output from tests as bytes Jun 2, 2023
@tdyas tdyas requested a review from benjyw June 2, 2023 19:10
@tdyas
Copy link
Contributor Author

tdyas commented Jun 2, 2023

Rebased and added a test to ensure invalid UTF-8 still allows .message to render output.

@tdyas tdyas marked this pull request as ready for review June 2, 2023 19:10
@tdyas
Copy link
Contributor Author

tdyas commented Jun 2, 2023

@benjyw: Thoughts on the interface change for TestResult? E.g., stdout -> stdout_bytes.

Copy link
Sponsor Contributor

@benjyw benjyw left a comment

Choose a reason for hiding this comment

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

I think that's an excellent API change. Given that we ourselves got confused by this, being explicit in the name is good.

@tdyas tdyas merged commit 86ad0ef into pantsbuild:main Jun 3, 2023
24 checks passed
@tdyas tdyas deleted the test_store_output_bytes branch June 3, 2023 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:bugfix Bug fixes for released features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Getting UnicodeDecodeError from stdout.decode when xdist is disabled
3 participants