-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
refactor(testing): redirect console output via reporter #11911
refactor(testing): redirect console output via reporter #11911
Conversation
This feeds console output to the reporter and handles silencing there instead of in the JavaScript code.
@@ -313,27 +313,27 @@ unitTest(function consoleTestStringifyCircular() { | |||
"JSON {}", | |||
); | |||
assertEquals( | |||
stringify(console), | |||
stringify(new Console(() => {})), |
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.
Test case is faulty on main, run with deno test --quiet and it fails because it is not bound
.
Probably introduced with wrapConsole.
Doesn't matter in this case what the fixture is, stringify is what is being tested.
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.
Yes I added it with wrapConsole
API. Good catch and thanks for fixing this.
globalThis.console = new Console((line) => { | ||
dispatchTestEvent({ | ||
output: { | ||
console: line, | ||
}, | ||
}); | ||
}); |
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.
Interesting
@@ -313,27 +313,27 @@ unitTest(function consoleTestStringifyCircular() { | |||
"JSON {}", | |||
); | |||
assertEquals( | |||
stringify(console), | |||
stringify(new Console(() => {})), |
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.
Yes I added it with wrapConsole
API. Good catch and thanks for fixing this.
@@ -248,7 +272,6 @@ async fn test_specifier( | |||
test_source.push_str(&format!( | |||
"await Deno[Deno.internal].runTests({});\n", | |||
json!({ | |||
"disableLog": program_state.flags.log_level == Some(Level::Error), |
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.
Nice! The less data needed to forward to JS the better
@@ -600,7 +624,9 @@ async fn test_specifiers( | |||
.buffer_unordered(concurrent_jobs.get()) | |||
.collect::<Vec<Result<Result<(), AnyError>, tokio::task::JoinError>>>(); | |||
|
|||
let mut reporter = create_reporter(concurrent_jobs.get() > 1); | |||
let mut reporter = | |||
create_reporter(concurrent_jobs.get() > 1, log_level != Some(Level::Error)); |
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.
log_level != Some(Level::Error)
is true when --quiet
flag is passed?
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.
It's inverted here, the reporter's flag is enable rather than disable.
Quiet sets log level to error (https://github.com/denoland/deno/blob/main/cli/flags.rs#L400)
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.
LGTM, thanks
This feeds console output to the reporter and handles silencing/echoing there instead of in the JavaScript code making all the output to stdout in a test come through from the reporter.
Stdout and stderr should also be piped through here but that would be a breaking change for later, possibly 2.0.
Towards #10099