FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: remove magic numbers in test-gc-http-client-onerror by Trott · Pull Request #24943 · nodejs/node · GitHub

/ node Public

test: remove magic numbers in test-gc-http-client-onerror - #24943

Closed
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:fix-23089
ConversationCommits (1)ChecksFiles changed
Closed

test: remove magic numbers in test-gc-http-client-onerror#24943
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:fix-23089

Conversation

Trott commented Dec 10, 2018

Copy link
Copy Markdown
Member

Remove magic numbers 500 and 10 from the test. Instead, detect when GC
has started and stop sending requests at that point.

On my laptop, this results in 68 or 72 requests per run instead of 500.

Fixes: #23089

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

nodejs-github-bot commented Dec 10, 2018

Copy link
Copy Markdown
Collaborator

@Trott build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1886/pipeline

nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 10, 2018

Trott commented Dec 10, 2018

Copy link
Copy Markdown
Member Author

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

Trott commented Dec 10, 2018

Copy link
Copy Markdown
Member Author

Stress test on master (with -J --repeat 100) to hopefully reproduce failures: https://ci.nodejs.org/job/node-stress-single-test/2125/

Stress test on this PR with same parameters:
https://ci.nodejs.org/job/node-stress-single-test/2127/

Trott added the wip Issues and PRs that are still a work in progress. label Dec 10, 2018

bnoordhuis commented Dec 10, 2018

Copy link
Copy Markdown
Member

Relevant failure: https://ci.nodejs.org/job/node-test-commit-linux/23914/nodes=centos6-64-gcc6/console

If I had to guess, it's because of the removal of the early return in getall().

Trott commented Dec 10, 2018

Copy link
Copy Markdown
Member Author

Relevant failure:

Yeah, there's a few of those. Not sure how to re-implement an early return without restoring some kind of magic number but I'll think about it....

BridgeAR commented Dec 12, 2018

Copy link
Copy Markdown
Member

@Trott it should be sufficient to add another variable (e.g., stop) which is set to true when the server ends. In that case getAll should stop being called.

Trott force-pushed the fix-23089 branch 2 times, most recently from 502e7a7 to aeb2138 Compare December 13, 2018 05:14

Trott commented Dec 13, 2018

Copy link
Copy Markdown
Member Author

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

Trott force-pushed the fix-23089 branch 2 times, most recently from 92b8899 to 011027a Compare December 13, 2018 18:38

Trott commented Dec 13, 2018

Copy link
Copy Markdown
Member Author

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

Trott commented Dec 13, 2018

Copy link
Copy Markdown
Member Author

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

test: remove magic numbers in test-gc-http-client-onerror
7974779 
Remove magic numbers (500, 10, 100) from the test. Instead, detect when GC
has started and stop sending requests at that point.

On my laptop, this results in 16 or 20 requests per run instead of 500.

Fixes: nodejs#23089

Trott commented Dec 13, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/19493/ ✔️

Trott commented Dec 14, 2018

Copy link
Copy Markdown
Member Author

Worked out the puzzle. This works now. Could use some reviews!

Trott removed the wip Issues and PRs that are still a work in progress. label Dec 14, 2018

Trott commented Dec 15, 2018

Copy link
Copy Markdown
Member Author

Needs a review or two. /ping @nodejs/testing

Trott commented Dec 16, 2018

Copy link
Copy Markdown
Member Author

Bump.

Trott added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 16, 2018

Trott commented Dec 17, 2018

Copy link
Copy Markdown
Member Author

Landed in 47ecf20

Trott closed this Dec 17, 2018
Trott added a commit to Trott/io.js that referenced this pull request Dec 17, 2018
test: remove magic numbers in test-gc-http-client-onerror
47ecf20 
Remove magic numbers (500, 10, 100) from the test. Instead, detect when
GC has started and stop sending requests at that point.

On my laptop, this results in 16 or 20 requests per run instead of 500.

Fixes: nodejs#23089

PR-URL: nodejs#24943
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 25, 2018
test: remove magic numbers in test-gc-http-client-onerror
b7bb114 
Remove magic numbers (500, 10, 100) from the test. Instead, detect when
GC has started and stop sending requests at that point.

On my laptop, this results in 16 or 20 requests per run instead of 500.

Fixes: #23089

PR-URL: #24943
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 25, 2018
test: remove magic numbers in test-gc-http-client-onerror
3e14212 
Remove magic numbers (500, 10, 100) from the test. Instead, detect when
GC has started and stop sending requests at that point.

On my laptop, this results in 16 or 20 requests per run instead of 500.

Fixes: #23089

PR-URL: #24943
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins mentioned this pull request Dec 25, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Dec 25, 2018
test: remove magic numbers in test-gc-http-client-onerror
9ad6bc2 
Remove magic numbers (500, 10, 100) from the test. Instead, detect when
GC has started and stop sending requests at that point.

On my laptop, this results in 16 or 20 requests per run instead of 500.

Fixes: #23089

PR-URL: #24943
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins mentioned this pull request Dec 25, 2018
Merged
This was referenced Dec 26, 2018
Closed
Closed
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
test: remove magic numbers in test-gc-http-client-onerror
423c533 
Remove magic numbers (500, 10, 100) from the test. Instead, detect when
GC has started and stop sending requests at that point.

On my laptop, this results in 16 or 20 requests per run instead of 500.

Fixes: nodejs#23089

PR-URL: nodejs#24943
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Trott deleted the fix-23089 branch January 13, 2022 22:50
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

cjihrig cjihrig approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Investigate flaky test-gc-http-client-onerror

5 participants

Back | FazBrowse Home | New Git URL