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
Improve V2 ./pants test
output to make it easier to parse results of multiple targets
#7676
Merged
Eric-Arellano
merged 6 commits into
pantsbuild:master
from
Eric-Arellano:v2-pytest-better-output
May 9, 2019
Merged
Improve V2 ./pants test
output to make it easier to parse results of multiple targets
#7676
Eric-Arellano
merged 6 commits into
pantsbuild:master
from
Eric-Arellano:v2-pytest-better-output
May 9, 2019
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Only output if success * Colorize stdout in red when its due to a failure * Add more whitespace between targets
Marked as a draft because I suspect you all will have a lot of feedback on this one. All ready for review otherwise. |
These ergonomic changes all make sense to me 👍 |
blorente
reviewed
May 8, 2019
Eric-Arellano
changed the title
Improve V2
Improve V2 May 8, 2019
./pants test
output for better logging when using multiple targets./pants test
output to make it easier to parse results of multiple targets
stuhood
approved these changes
May 8, 2019
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This should definitely incorporate the --no-colors
flag before landing.
Eric-Arellano
added a commit
that referenced
this pull request
May 9, 2019
### Problem To improve the UX of V2 rules, there are certain times that we would like to use colors in the output, just as we do in V1. However, we also need some way to respect the global flag `--no-colors` / `--colors`. ### Solution Add safe color methods to the `Console` and `MockConsole` classes. These take a parameter for whether colors may be used, which in the former case get initialized by the global options. Console rule users would then use colors like this: `console.write_stdout(console.green("Pretty colors!"))`. #### Alternative API considered: stripping output Instead of defining color methods on the `Console` class, we could have left the callers to use the `colors` library normally, then changed `Console.write_stderr()` et al. to call `colors.strip_colors()` on the payload. This reduces the risk of people in the current approach accidentally directly using `colors` rather than the console methods. It also allows us to keep the definition of `Console` a bit smaller. However, the idea of first adding color codes then intentionally stripping them away is awkward and it was decided that this was not worth it. ### Result Colors can now be safely used in V2, which unblocks #7676. The v2 test rule was also updated to print in red "Tests failed" upon any failures. <img width="830" alt="Screen Shot 2019-05-09 at 8 53 41 AM" src="https://user-images.githubusercontent.com/14852634/57467745-f45cc900-7237-11e9-8bcb-6a7cf3c5ebc0.png">
Eric-Arellano
added a commit
that referenced
this pull request
May 10, 2019
### Problem Implements #7388. Swallowing stderr makes debugging very difficult. Important clarification: failing tests will write to stdout, meaning that we do already output any message captured by Pytest. Instead, this PR is meant to log issues that occur before Pytest executes, such as issues from Pants. ### Solution Follow the convention established by #7676 to output `{address} stderr:`, followed by the stderr.
Eric-Arellano
added a commit
to Eric-Arellano/pants
that referenced
this pull request
May 10, 2019
commit 0a38b338ba5c3d9dd0bc080fa477e8b74e9850e6 Merge: c0bfc9e 9ca32a9 Author: Eric Arellano <ericarellano@me.com> Date: Fri May 10 11:20:51 2019 -0700 Merge branch 'dwagnerhall/strip-prefix' of https://github.com/twitter/pants into twitter-dwagnerhall/strip-prefix commit c0bfc9e Author: Eric Arellano <ericarellano@me.com> Date: Fri May 10 10:49:18 2019 -0700 Output stderr in V2 test rule (pantsbuild#7694) ### Problem Implements pantsbuild#7388. Swallowing stderr makes debugging very difficult. Important clarification: failing tests will write to stdout, meaning that we do already output any message captured by Pytest. Instead, this PR is meant to log issues that occur before Pytest executes, such as issues from Pants. ### Solution Follow the convention established by pantsbuild#7676 to output `{address} stderr:`, followed by the stderr. commit 55e5721 Author: Daniel Wagner-Hall <dwagnerhall@twitter.com> Date: Fri May 10 17:32:03 2019 +0100 Remove now-unused Path type (pantsbuild#7701) The engine lost Paths from its Snapshots at some point, and we didn't clean up. commit 9ca32a9 Author: Daniel Wagner-Hall <dwagnerhall@twitter.com> Date: Fri May 10 10:59:41 2019 +0100 Allow Directories to be un-nested commit 5eed2e7 Author: Eric Arellano <ericarellano@me.com> Date: Thu May 9 17:38:03 2019 -0700 Mark build-support Python files as Pants targets to lint build-support (pantsbuild#7633) ### Converting to Pants targets Now that we have two Python scripts, and may have more scripts in the future, there is value in linting our script code. By making these two scripts as targets, we can now use `./pants fmt`, `./pants lint`, and `./pants mypy` on the build-support folder for little cost. The pre-commit check will check `./pants fmt` and `./pants lint` automatically on the `build-support` Python files now. It will not do the header check until we explicitly add the folder. ### Add `common.py` We also add `common.py` to reduce duplication and simplify writing scripts. See the `NB` in that file about why we only use the stdlib for that file.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
When running
./pants test
via V2 with multiple arguments, the output is difficult to scan. Everything appears as the same target, and it is not clear what is being printed:Solution
stdout
andstderr
. It's important that we can disambiguate which address those correspond to.Not done: ignoring stdout of success tests
Successful tests arguably don't need their stdout printed. Pytest for example silences stdout and stderr for successful tests.
However, we would only want to make this change if we gave users the options to choose the logging behavior, such as Bazel's https://docs.bazel.build/versions/master/command-line-reference.html#flag--test_output.
We don't want to introduce V2-only flags unless we absolutely have to, so we punt on this change for now.
Result