-
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
fix(ext/websocket): Ensure that errors are available after async response returns #19642
Conversation
Maybe we can revert 3fe44a5 with this change? |
I think so. I'll do that as part of this PR. |
f002398
to
75393d4
Compare
@@ -154,7 +154,6 @@ export async function runSingleTest( | |||
harnessStatus = JSON.parse(line.slice(5)); | |||
} else { | |||
stderr += line + "\n"; | |||
console.error(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.
This was confusing test results by printing out-of-order
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.
Good catch 👍
tools/wpt.ts
Outdated
@@ -611,6 +611,7 @@ function reportVariation(result: TestResult, expectation: boolean | string[]) { | |||
for (const result of expectedFailedButPassed) { | |||
console.log(` ${JSON.stringify(result.name)}`); | |||
} | |||
console.log("\nstderr:\n" + result.stderr); |
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.
This makes stderr output consistent.
@@ -567,8 +567,9 @@ function analyzeTestResult( | |||
|
|||
function reportVariation(result: TestResult, expectation: boolean | string[]) { | |||
if (result.status !== 0 || result.harnessStatus === null) { | |||
console.log(`test stderr:`); | |||
writeAllSync(Deno.stdout, new TextEncoder().encode(result.stderr)); | |||
if (result.stderr) { |
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.
Print stderr
consistently between modes.
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
Fixes the WPT tests that test w/invalid codes. Also explicitly ignoring some h2 tests to hopefully prevent flakes.
The previous changes to WebSocketStream introduced a bug where the close errors were not made available if the
pull
method was re-entrant.