-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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/node): http2.createServer #22708
Conversation
tests/unit_node/http2_test.ts
Outdated
@@ -165,3 +166,23 @@ Deno.test("[node/http2 client GET https://www.example.com]", async () => { | |||
assertEquals(status, 200); | |||
assert(chunk.length > 0); | |||
}); | |||
|
|||
Deno.test("[node/http2.createServer()]", { sanitizeOps: false }, async () => { |
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.
The tests are timing out when we have two http2.createServer() in the same file. I'm not sure how the tests are processed by Deno.test underneath but I'll explore what's the cause.
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.
The previous test was timing out because we no longer emit "stream" on a session in the current implement. I replaced the test with the new test instead.
Signed-off-by: Satya Rohith <me@satyarohith.com>
Looks good to me -- just a windows sanitizer failure left. |
This reverts commit 9c79c7e.
I unfortunately don't have a windows machine to investigate this further so I disabled the test on windows for time being. But looking at the error the TCP listener is closed when we call server.close() in the test but not sure why it isn't triggered in windows. I also wonder if it's exposing any bugs in the sanitizer. |
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.
Closes #22618