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

test: move net bytes-per-chunk test to parallel by addaleax · Pull Request #21322 · nodejs/node · GitHub

/ node Public

test: move net bytes-per-chunk test to parallel - #21322

Closed
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:move-net-bytes-per-incoming-chunk
Closed

test: move net bytes-per-chunk test to parallel#21322
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:move-net-bytes-per-incoming-chunk

Conversation

Copy link
Copy Markdown
Member

This was added in 3217e8e as
a regression test for a security patch. We moved it to
sequential to lower the risk of creating a flaky test,
because an earlier version of it was failing one some platforms.

There is no known reason why te test should be flaky in this form,
though, and moving it to parallel would be good because it does take
around 3 seconds that would otherwise fully count towards the
test run time.

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

This was added in 3217e8e as
a regression test for a security patch. We moved it to
`sequential` to lower the risk of creating a flaky test,
because an earlier version of it was failing one some platforms.

There is no known reason why te test should be flaky in this form,
though, and moving it to parallel would be good because it does take
around 3 seconds that would otherwise fully count towards the
test run time.

Copy link
Copy Markdown
Collaborator

nodejs-github-bot added the test Issues and PRs related to the tests. label Jun 13, 2018
addaleax requested a review from Trott June 13, 2018 22:44

Copy link
Copy Markdown
Member Author

trivikr added the net Issues and PRs related to the net subsystem. label Jun 14, 2018

Trott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This test is unusually resource-intensive and times out for me locally with tools/test.py -j 40 --repeat 80 test/parallel/test-net-bytes-per-incoming-chunk-overhead.js . (I usually try to get -j up to 96 and --repeat to 192 before I move something to parallel but I have no basis for that other than my own experience. Empirical? Anecdotal? Arbitrary? All of the above?)

That said, my local machine is not CI and running 40 copies of the test is not what CI will do. So I'm fine with moving this to parallel. It can always be fast-track moved back to sequential if we start seeing it time out.

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jun 17, 2018

Copy link
Copy Markdown
Member Author

Landed in d362b07

addaleax closed this Jun 20, 2018
addaleax deleted the move-net-bytes-per-incoming-chunk branch June 20, 2018 20:51
addaleax added a commit that referenced this pull request Jun 20, 2018
This was added in 3217e8e as
a regression test for a security patch. We moved it to
`sequential` to lower the risk of creating a flaky test,
because an earlier version of it was failing one some platforms.

There is no known reason why te test should be flaky in this form,
though, and moving it to parallel would be good because it does take
around 3 seconds that would otherwise fully count towards the
test run time.

PR-URL: #21322
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Trott commented Jun 22, 2018

Copy link
Copy Markdown
Member

@addaleax Unfortunately, this test timed out on FreeBSD today.

https://ci.nodejs.org/job/node-test-commit-freebsd/18625/nodes=freebsd10-64/console

15:52:56 not ok 1288 parallel/test-net-bytes-per-incoming-chunk-overhead
15:52:56   ---
15:52:56   duration_ms: 120.210
15:52:56   severity: fail
15:52:56   exitcode: -15
15:52:56   stack: |-
15:52:56     timeout
15:52:56   ...

Trott commented Jun 22, 2018

Copy link
Copy Markdown
Member

Even when it passes, it is dangerously close to the 120 second limit on FreeBSD.

https://ci.nodejs.org/job/node-test-commit-freebsd/18623/nodes=freebsd10-64/console
114.377 seconds

https://ci.nodejs.org/job/node-test-commit-freebsd/18619/nodes=freebsd10-64/console
118.110 seconds

For comparison, when it was in sequential instead of parallel:
https://ci.nodejs.org/job/node-test-commit-freebsd/18617/nodes=freebsd10-64/console
52.190 seconds

Trott commented Jun 22, 2018

Copy link
Copy Markdown
Member

Hmmm...seems like something worth looking at on FreeBSD that it takes 52 seconds even in sequential. Other platforms don't show nearly that much time for the test.

Trott added a commit to Trott/io.js that referenced this pull request Jun 22, 2018
The test is timing out on FreeBSD 10 in CI. It takes less than half as
long to run when it is in sequential on that platform instead of
parallel.

Refs: nodejs#21322 (comment)
targos pushed a commit that referenced this pull request Jun 22, 2018
This was added in 3217e8e as
a regression test for a security patch. We moved it to
`sequential` to lower the risk of creating a flaky test,
because an earlier version of it was failing one some platforms.

There is no known reason why te test should be flaky in this form,
though, and moving it to parallel would be good because it does take
around 3 seconds that would otherwise fully count towards the
test run time.

PR-URL: #21322
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Jun 24, 2018
The test is timing out on FreeBSD 10 in CI. It takes less than half as
long to run when it is in sequential on that platform instead of
parallel.

Refs: #21322 (comment)

PR-URL: #21457
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jun 24, 2018
The test is timing out on FreeBSD 10 in CI. It takes less than half as
long to run when it is in sequential on that platform instead of
parallel.

Refs: #21322 (comment)

PR-URL: #21457
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos mentioned this pull request Jul 3, 2018
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants


Back | FazBrowse Home | New Git URL