Skip to content

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

Merged
Trott merged 1 commit into
nodejs:masterfrom
Trott:fix-36078
Nov 17, 2020
Merged

test: make test-http2-client-jsstream-destroy.js reliable#36129
Trott merged 1 commit into
nodejs:masterfrom
Trott:fix-36078

Conversation

@Trott

@TrottTrott commented Nov 15, 2020

Copy link
Copy Markdown
Member

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-botnodejs-github-bot added the test Issues and PRs related to the tests. label Nov 15, 2020
@TrottTrott mentioned this pull request Nov 15, 2020
Closed
@Trott

Trott commented Nov 15, 2020

Copy link
Copy Markdown
MemberAuthor

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

@mmomtchev

mmomtchev commented Nov 16, 2020

Copy link
Copy Markdown
Contributor

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

mmomtchev
mmomtchev reviewed Nov 16, 2020
View reviewed changes
Comment threadtest/parallel/test-http2-client-jsstream-destroy.js Outdated
@Trott

Trott commented Nov 16, 2020

Copy link
Copy Markdown
MemberAuthor

@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.

@TrottTrott added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 16, 2020
@github-actionsgithub-actionsBot 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

nodejs-github-bot commented Nov 16, 2020

Copy link
Copy Markdown
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:masterNov 17, 2020
@Trott

Trott commented Nov 17, 2020

Copy link
Copy Markdown
MemberAuthor

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>
@codebyterecodebytere 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>
@BethGriggsBethGriggs 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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjamingrbenjamingrbenjamingr approved these changes
@aduh95aduh95aduh95 approved these changes
+1 more reviewer
@mmomtchevmmomtchevmmomtchev left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

testIssues 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