Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

test: improve test-gc-http-client-timeout #23184

Closed
wants to merge 1 commit into from

Conversation

lundibundi
Copy link
Member

@lundibundi lundibundi commented Sep 30, 2018

  • decrease number of requests 500 -> 300
  • extract 'cb' to a file-local function

This should make test more reliable and less resource intensive.

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

Also, by moving for-loop of getall calls into the server callback I managed to run this with '-j 32 --repeat 1920' on a 4 core, 8 thread machine. Though this removes 'surefire' timeouts (due to a server being unavailable) this test will still get the timeouts due to the sheer amount of requests being made. Therefore if it's okay I'll make the change.

Refs: #23066
(not fixes as this still fails in parallel on '-j 32 --repeat 1920' locally)

* decrease number of requests 500 -> 300
* extract 'cb' to a file-local function

This should make test more reliable and less resource intensive.
@lundibundi lundibundi self-assigned this Sep 30, 2018
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 30, 2018
@lundibundi lundibundi changed the title test: harden test-gc-http-client-timeout test: improve test-gc-http-client-timeout Sep 30, 2018
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! :)

@lundibundi
Copy link
Member Author

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

lundibundi commented Oct 1, 2018

@addaleax @BridgeAR @thefourtheye what do you think of this suggestion

Also, by moving for-loop of getall calls into the server callback I managed to run this with '-j 32 --repeat 1920' on a 4 core, 8 thread machine. Though this removes 'surefire' timeouts (due to a server being unavailable) this test will still get the timeouts due to the sheer amount of requests being made. Therefore if it's okay I'll make the change.

I mentioned in the first post?

Edit: resume CI: https://ci.nodejs.org/job/node-test-pull-request/17552/

@danbev
Copy link
Contributor

danbev commented Oct 4, 2018

Landed in 69a422b.

@danbev danbev closed this Oct 4, 2018
danbev pushed a commit that referenced this pull request Oct 4, 2018
* decrease number of requests 500 -> 300
* extract 'cb' to a file-local function

This should make test more reliable and less resource intensive.

PR-URL: #23184
Refs: #23066
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@thefourtheye
Copy link
Contributor

@lundibundi That suggestion can be done separately. Get a PR up, let's see what people think.

targos pushed a commit that referenced this pull request Oct 5, 2018
* decrease number of requests 500 -> 300
* extract 'cb' to a file-local function

This should make test more reliable and less resource intensive.

PR-URL: #23184
Refs: #23066
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
* decrease number of requests 500 -> 300
* extract 'cb' to a file-local function

This should make test more reliable and less resource intensive.

PR-URL: #23184
Refs: #23066
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
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
Development

Successfully merging this pull request may close these issues.

7 participants