-
Notifications
You must be signed in to change notification settings - Fork 13.7k
compiletest: Reduce the number of println!
calls that don't have access to TestCx
#145982
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
base: master
Are you sure you want to change the base?
Conversation
Running `./x --verbose` will still print out the command-line arguments, and setting `RUST_LOG=compiletest` will now log the full config instead of a subset.
When working on a new output-capture system, this will make it easier to obtain a capturing stream from the test context.
The code in this module is always called in the context of running an individual tests, and sometimes prints output that needs to be captured. Moving this module into `runtest` will make it easier to find and audit all of the print statements that need to be updated when overhauling output-capture.
This appears to have been leftover debugging code. If the capture information turns out to have still been useful, we can find a way to emit it in a way that doesn't interfere with overhauling compiletests's output capture system.
|
Some changes occurred in src/tools/compiletest cc @jieyouxu |
I was also going to overhaul panic-capture, but that ended up making the PR much larger, so I've split off this initial chunk to make review a bit easier. |
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 looks good overall, just a question re. logging the config
@@ -538,6 +492,8 @@ pub fn opt_str2(maybestr: Option<String>) -> String { | |||
|
|||
/// Called by `main` after the config has been parsed. | |||
pub fn run_tests(config: Arc<Config>) { | |||
debug!(?config, "run_tests"); |
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.
Discussion: hm, I think it was originally intended so the config gets logged when run in CI?
EDIT: wait no, that can't be right, I don't remember us passing --verbose
in CI 🤔
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.
If we suspect it's still useful, I can add in a println!("{config:#?}")
to just dump the whole config.
(It's mostly redundant with the compiletest command-line arguments that ./x --verbose
will print out anyway, but I suppose that seeing the post-parse config could have some value.)
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.
Hm right. I think we might want to dump the post-parse config under CI env, but not necessary for this PR.
You can r=me after PR CI is 🟢 |
@bors r=jieyouxu rollup |
compiletest: Reduce the number of `println!` calls that don't have access to `TestCx` In order to stop using `#![feature(internal_output_capture)]` in compiletest, we need to be able to capture the console output of individual tests run by the executor. The approach I have planned is to have all test runners print “console” output into a trait object that is passed around as part of `TestCx`, since almost all test-runner code has easy access to that context. So `println!("foo")` will become `writeln!(self.stdout, "foo")`, and so on. In order to make that viable, we need to avoid unnecessary printing in places that don't have easy access to `TestCx`. To do so, we can either get rid of unnecessary print statements, or rearrange the code to make the context available. This PR uses both approaches. r? jieyouxu
Rollup of 4 pull requests Successful merges: - #145675 (Rehome 30 `tests/ui/issues/` tests to other subdirectories under `tests/ui/` [#1 of Batch #2]) - #145676 (Rehome 30 `tests/ui/issues/` tests to other subdirectories under `tests/ui/` [#2 of Batch #2]) - #145982 (compiletest: Reduce the number of `println!` calls that don't have access to `TestCx`) - #145984 (`TokenStream` cleanups) r? `@ghost` `@rustbot` modify labels: rollup
In order to stop using
#![feature(internal_output_capture)]
in compiletest, we need to be able to capture the console output of individual tests run by the executor.The approach I have planned is to have all test runners print “console” output into a trait object that is passed around as part of
TestCx
, since almost all test-runner code has easy access to that context. Soprintln!("foo")
will becomewriteln!(self.stdout, "foo")
, and so on.In order to make that viable, we need to avoid unnecessary printing in places that don't have easy access to
TestCx
. To do so, we can either get rid of unnecessary print statements, or rearrange the code to make the context available. This PR uses both approaches.r? jieyouxu