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

At test discovery, write to logfile in the same format as to stdout #123365

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

aspotashev
Copy link

@aspotashev aspotashev commented Apr 2, 2024

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:

cargo test -- --logfile=/tmp/1.log --list

Old format:

test whatever

New format:

whatever: test

1 test, 0 benchmarks

Rationale for the behavior change:

  • The code becomes easier to maintain: we reuse the same code to generate output into stdout and into log file
  • Fix user confusion, as they currently see different formats in stdout and in log file.

In addition to the behavior change, the following refactorings are applies:

  • Added OutputMultiplexer. It simplifies writing to multiple outputs (e.g. stdout + logfile). In a future PR, I'm going to change run_tests_console() to also use OutputMultiplexer.
    • To demonstrate the simplification, Make libtest logfile respect --format #96290 has repeated calls like out.write_result + log_output.write_result (one for stdout, one for log file output.) This repetition won't be needed when we use OutputMultiplexer.
  • Eliminate tight coupling of formatters to the enum OutputLocation<T>. This is good for code health, and also unblocks the introduction of OutputMultiplexer.

Future work:

@rustbot
Copy link
Collaborator

rustbot commented Apr 2, 2024

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 (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 2, 2024
@aspotashev aspotashev changed the title Converge discovery logfile At test discovery, write to logfile in the same format as to stdout Apr 2, 2024
@rust-log-analyzer

This comment has been minimized.

@aspotashev
Copy link
Author

cc @felipeamp

@aspotashev aspotashev force-pushed the converge-discovery-logfile branch from b057fcc to d01f463 Compare April 2, 2024 14:59
@aspotashev aspotashev marked this pull request as ready for review April 2, 2024 15:07
@rustbot
Copy link
Collaborator

rustbot commented Apr 2, 2024

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@aspotashev
Copy link
Author

r? rust-lang/testing-devex

@rustbot rustbot assigned Muscraft and unassigned cuviper Apr 2, 2024
@epage
Copy link
Contributor

epage commented Apr 3, 2024

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.
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
```
@aspotashev aspotashev force-pushed the converge-discovery-logfile branch from d01f463 to 1c78c9c Compare April 12, 2024 13:19
@aspotashev
Copy link
Author

Could you expand on why this was only partially done and why coupling refactors in?

I intentionally left the run_tests_console out of this PR to reduce its scope of impact. If you prefer a larger PR that covers both list_tests_console and run_tests_console, let me know and I will include the additional changes here. Here's a draft of run_tests_console changes: aspotashev#1.

Regarding the other changes in this PR:

  • The refactorings are necessary to reuse the text generation logic and therefore avoid diversion of 2 code paths (stdout vs logfile). We could do it as a later step, but it would be more work/code changes that way.
  • The new unit tests are added to help reviewers and repo archaeologists understand the user impact.

Seems like #96290 was mostly ready, just waiting on merge conflicts.

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

@bors
Copy link
Contributor

bors commented Jun 11, 2024

☔ The latest upstream changes (presumably #126240) made this pull request unmergeable. Please resolve the merge conflicts.

@aspotashev
Copy link
Author

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.

@jieyouxu jieyouxu added T-testing-devex Relevant to the testing devex team (testing DX), which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-testing-devex Relevant to the testing devex team (testing DX), which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--logfile for libtest doesn't respect --format.
8 participants