-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
At test discovery, write to logfile in the same format as to stdout #123365
base: master
Are you sure you want to change the base?
At test discovery, write to logfile in the same format as to stdout #123365
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cuviper (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
This comment has been minimized.
This comment has been minimized.
cc @felipeamp |
b057fcc
to
d01f463
Compare
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
r? rust-lang/testing-devex |
Without having looked at the changes yet, it sounds like #96290 does this for everything but this only does it for discovery (and couples a bunch of refactors in). Could you expand on why this was only partially done and why coupling refactors in? Seems like #96290 was mostly ready, just waiting on merge conflicts. |
PrettyFormatter doesn't need to check the OutputLocation enum value anymore. This improves the separation of concerns.
…before_printing_them`.
Rationale: * In a next commit, we would store a trait reference in `PrettyFormatter<T>::out` instead of using a concrete type `OutputLocation<T>`. * If the trait had generic methods, we would get compilation error "cannot be made into an object" in that next commit.
Make logfile output more similar to normal test output.
Example of affected command: ``` cargo test -- --logfile=/tmp/1.log --list ```
d01f463
to
1c78c9c
Compare
I intentionally left the Regarding the other changes in this PR:
The merge conflicts on #96290 are mostly due to overlap with 3720753, and they aren't trivial. Resolving the conflicts would likely entail adding more repetitive logic, that's why I'm proposing a slightly different approach (this PR.) |
☔ The latest upstream changes (presumably #126240) made this pull request unmergeable. Please resolve the merge conflicts. |
IIUC, this PR was discussed within testing-devex team a few weeks ago. To provide additional background, I've just posted rust-lang/testing-devex-team#9 with a high-level description of our use case that I was trying to unblock with my PR. Please take a look. |
At test discovery, write to logfile in the same format as to stdout.
The top-most commit of current PR is the only commit that contains a behavior change: aspotashev@d01f463
Example of affected command:
Old format:
New format:
Rationale for the behavior change:
In addition to the behavior change, the following refactorings are applies:
OutputMultiplexer
. It simplifies writing to multiple outputs (e.g. stdout + logfile). In a future PR, I'm going to changerun_tests_console()
to also useOutputMultiplexer
.out.write_result
+log_output.write_result
(one for stdout, one for log file output.) This repetition won't be needed when we useOutputMultiplexer
.OutputLocation<T>
. This is good for code health, and also unblocks the introduction ofOutputMultiplexer
.Future work:
run_tests_console()
to also useOutputMultiplexer
.--logfile
for libtest doesn't respect--format
. #57147.)