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.

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: make test-http2-client-jsstream-destroy.js reliable #36129

Merged
merged 1 commit into from Nov 17, 2020
Merged

test: make test-http2-client-jsstream-destroy.js reliable #36129

merged 1 commit into from Nov 17, 2020

Conversation

Trott
Copy link
Member

@Trott Trott commented Nov 15, 2020

Use events instead of setTimeout() calls.

Fixes: #36078

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 15, 2020
@Trott Trott mentioned this pull request Nov 15, 2020
Closed
@Trott
Copy link
Member Author

Trott commented Nov 15, 2020

@mmomtchev Can you confirm that this still tests the thing you intended it to test?

@mmomtchev
Copy link
Contributor

@Trott It has the downside of blocking indefinitely on the old version, but otherwise yes, iit does test it

@Trott
Copy link
Member Author

Trott commented Nov 16, 2020

@Trott It has the downside of blocking indefinitely on the old version, but otherwise yes, iit does test it

Ah, OK. We have a lot of tests that are like that so I think I'm OK leaving it that way. tools/test.py has a default timeout (that is platform dependent) and also a --timeout CLI option, so as long as the tests are run through test.py, they don't hang indefinitely.

@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 16, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 16, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@Trott
test: make test-http2-client-jsstream-destroy.js reliable
fff25a0 
Use events instead of setTimeout() calls.

Fixes: nodejs#36078

PR-URL: nodejs#36129
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@Trott Trott merged commit fff25a0 into nodejs:master Nov 17, 2020
@Trott
Copy link
Member Author

Trott commented Nov 17, 2020

Landed in fff25a0

@Trott Trott deleted the fix-36078 branch November 17, 2020 16:29
codebytere pushed a commit that referenced this pull request Nov 22, 2020
@Trott @codebytere
test: make test-http2-client-jsstream-destroy.js reliable
f9bbd35 
Use events instead of setTimeout() calls.

Fixes: #36078

PR-URL: #36129
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@codebytere codebytere mentioned this pull request Nov 22, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
@Trott @BethGriggs
test: make test-http2-client-jsstream-destroy.js reliable
af0e562 
Use events instead of setTimeout() calls.

Fixes: #36078

PR-URL: #36129
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
@Trott @BethGriggs
test: make test-http2-client-jsstream-destroy.js reliable
fb32f3e 
Use events instead of setTimeout() calls.

Fixes: #36078

PR-URL: #36129
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
@Trott @BethGriggs
test: make test-http2-client-jsstream-destroy.js reliable
402e29a 
Use events instead of setTimeout() calls.

Fixes: #36078

PR-URL: #36129
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmomtchev mmomtchev mmomtchev left review comments

@benjamingr benjamingr benjamingr approved these changes

@aduh95 aduh95 aduh95 approved these changes

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

Successfully merging this pull request may close these issues.

test-http2-client-jsstream-destroy.js write after a stream was destroyed
5 participants
@Trott @mmomtchev @nodejs-github-bot @benjamingr @aduh95