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: remove magic numbers in test-gc-http-client-onerror #24943

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Dec 10, 2018

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
Copy link
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 nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 10, 2018
@Trott
Copy link
Member Author

Trott commented Dec 10, 2018

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

@Trott
Copy link
Member Author

Trott commented Dec 10, 2018

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 Trott added the wip Issues and PRs that are still a work in progress. label Dec 10, 2018
@bnoordhuis
Copy link
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
Copy link
Member Author

Trott commented Dec 10, 2018

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
Copy link
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 Trott force-pushed the fix-23089 branch 2 times, most recently from 502e7a7 to aeb2138 Compare December 13, 2018 05:14
@Trott
Copy link
Member Author

Trott commented Dec 13, 2018

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

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

Trott commented Dec 13, 2018

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

@Trott
Copy link
Member Author

Trott commented Dec 13, 2018

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

@Trott
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
Copy link
Member Author

Trott commented Dec 13, 2018 •
edited
Loading

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

@Trott
Copy link
Member Author

Trott commented Dec 14, 2018

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

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

Trott commented Dec 15, 2018

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

@Trott
Copy link
Member Author

Trott commented Dec 16, 2018

Bump.

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 16, 2018
@Trott
Copy link
Member Author

Trott commented Dec 17, 2018

Landed in 47ecf20

@Trott Trott closed this Dec 17, 2018
Trott added a commit to Trott/io.js that referenced this pull request Dec 17, 2018
@Trott
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
@Trott @MylesBorins
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
@Trott @MylesBorins
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 MylesBorins mentioned this pull request Dec 25, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Dec 25, 2018
@Trott @MylesBorins
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 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
@Trott @refack
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 Trott deleted the fix-23089 branch January 13, 2022 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, 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
@Trott @nodejs-github-bot @bnoordhuis @BridgeAR @cjihrig