GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test: make test-http2-client-jsstream-destroy.js reliable by Trott · Pull Request #36129 · nodejs/node

Conversation

@Trott

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss and szmarczak reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to Node.js core tests and test infrastructure. label Nov 15, 2020
@Trott Trott mentioned this pull request Nov 15, 2020
test-http2-client-jsstream-destroy.js write after a stream was destroyed #36078
Closed
@Trott

Trott commented Nov 15, 2020

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aduh95
aduh95 approved these changes Nov 15, 2020
View reviewed changes
benjamingr
benjamingr approved these changes Nov 16, 2020
View reviewed changes
@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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mmomtchev
mmomtchev reviewed Nov 16, 2020
View reviewed changes
Comment thread test/parallel/test-http2-client-jsstream-destroy.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@Trott

Trott commented Nov 16, 2020

Copy link
Copy Markdown
Member Author

@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.
mmomtchev reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Nov 16, 2020
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. 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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@Trott
Trott force-pushed the fix-36078 branch from 0e73e6c to fff25a0 Compare November 17, 2020 16:28
@Trott
Trott merged commit fff25a0 into nodejs:master Nov 17, 2020
@Trott

Trott commented Nov 17, 2020

Copy link
Copy Markdown
Member Author

Landed in fff25a0
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@codebytere codebytere mentioned this pull request Nov 22, 2020
v15.3.0 proposal #36232
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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
v14.15.2 proposal #36476
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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjamingr benjamingr benjamingr approved these changes
@aduh95 aduh95 aduh95 approved these changes
+1 more reviewer
@mmomtchev mmomtchev mmomtchev left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

test Issues and PRs related to Node.js core tests and test infrastructure.

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

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@Trott @mmomtchev @nodejs-github-bot @benjamingr @aduh95
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.