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.

Sign up for GitHub

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

Jump to bottom

test: add http2 compat setTimeout tests #15156

Closed
wants to merge 1 commit into from
Closed

test: add http2 compat setTimeout tests #15156

wants to merge 1 commit into from

Conversation

apapirovski
Copy link
Member

Add tests for Http2ServerRequest and Http2ServerResponse setTimeout. Let me know if I should adjust anything!

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@apapirovski
test: add http2 compat setTimeout tests
803832e 
Add tests for Http2ServerRequest and Http2ServerResponse setTimeout
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 3, 2017
@mscdex mscdex added the http2 Issues or PRs related to the http2 subsystem. label Sep 3, 2017
@apapirovski apapirovski mentioned this pull request Sep 3, 2017
Closed
3 tasks
benjamingr
benjamingr requested changes Sep 3, 2017
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thanks for doing this!

Why are the tests skipped if there is no crypto?

@lpinca
Copy link
Member

lpinca commented Sep 3, 2017

@benjamingr http2 requires crypto.

lpinca
lpinca approved these changes Sep 3, 2017
View reviewed changes
Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, maybe we can use a single file for both request.setTimeout() and response.setTimeout() but I'm also fine with it as is.

@jasnell
Copy link
Member

jasnell commented Sep 5, 2017

@benjamingr The tests are skipped because the http2 module depends on the tls module to load.

@jasnell
Copy link
Member

jasnell commented Sep 7, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/9975/

jasnell pushed a commit that referenced this pull request Sep 7, 2017
@apapirovski @jasnell
test: add http2 compat setTimeout tests
91dc507 
Add tests for Http2ServerRequest and Http2ServerResponse setTimeout

PR-URL: #15156
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Sep 7, 2017

Landed in 91dc507

@jasnell jasnell closed this Sep 7, 2017
@apapirovski apapirovski deleted the patch-http2-test-compat-settimeout branch September 9, 2017 19:57
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
@apapirovski @MylesBorins
test: add http2 compat setTimeout tests
3bf3067 
Add tests for Http2ServerRequest and Http2ServerResponse setTimeout

PR-URL: #15156
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Sep 11, 2017
@apapirovski @MylesBorins
test: add http2 compat setTimeout tests
5622224 
Add tests for Http2ServerRequest and Http2ServerResponse setTimeout

PR-URL: #15156
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
@apapirovski @MylesBorins
test: add http2 compat setTimeout tests
ac71d99 
Add tests for Http2ServerRequest and Http2ServerResponse setTimeout

PR-URL: #15156
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit to addaleax/node that referenced this pull request Sep 13, 2017
@apapirovski @addaleax
test: add http2 compat setTimeout tests
993faa7 
Add tests for Http2ServerRequest and Http2ServerResponse setTimeout

PR-URL: nodejs#15156
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@benjamingr benjamingr benjamingr approved these changes

@lpinca lpinca lpinca approved these changes

Assignees
No one assigned
Labels
http2 Issues or PRs related to the http2 subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@apapirovski @lpinca @jasnell @benjamingr @mscdex @MylesBorins @nodejs-github-bot