test: refactor pummel tests #25485

Trott · 2019-01-14T05:05:30Z

This refactors 4 pummel tests as a hopefully final step before being able to enable pummel tests in node-daily-master. Currently, all pummel tests pass for me locally if run once at a time. However, if run with tools/test.py pummel, there will be some failures due to side effects (mostly network throttling, it would seem). These changes resolve that by reducing some of the pummel-iness of some tests and also moving some tests off of common.PORT and use port 0 (OS-assigned available port) instead.

Checklist

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

* Reduce concurrent and duration options by half so as to avoid interference with other tests. (Excessive TCP activity in this test resulted in throttling that caused subsequent tests to fail on my local setup.) * Use an OS-provided port rather than `common.PORT`. This possibly reduces side-effects on other tests (that may also be using `common.PORT`). * Add punctuation in comments.

* Reduce ROUNDS and ATTEMPTS_PER_ROUND by half to avoid spurious test failures as a result of side effects from other tests. (For my local setup, test-keep-alive seems to cause this test to fail with ETIMEDOUT and/or EADDRNOTAVAIL. It would seem to be a result of throttling. Reducing the pummel-iness of that test and this one seems to solve the problem.) * Apply capitalization and punctuation to comment.

* Use port 0 instead of `common.PORT`. * Reduce `concurrent` from 100 to 50 and `connections_per_client` from 5 to 3. This is to avoid side effects from other tests. Prior to this change, running this along with test-keep-alive would result in failures on my local setup, apparently due to network throttling. * Remove unnecessary `console.log()` and improve remaining `console.log()` to provide clearer information.

* Use port 0 instead of `common.PORT`. * Use `//` for comments, capitalize comments, and add punctuation.

nodejs-github-bot · 2019-01-14T05:05:30Z

@Trott sadly an error occured when I tried to trigger a build :(

Trott · 2019-01-14T05:05:44Z

Pummel tests are not (yet) run in CI so lite CI should be sufficient.

Trott · 2019-01-14T05:06:43Z

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/2242/

targos · 2019-01-14T06:15:04Z

Custom run of pummel tests: https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/3860/

Trott · 2019-01-14T19:18:35Z

Custom run of pummel tests: https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/3860/

test-hash-seed takes over a minute on my laptop, so I guess it's not surprising that it might timeout in CI. Someone will have to take a look at the fixture file, I suspect.

test-keep-alive may be failing because wrk is not installed?

Trott · 2019-01-15T14:10:03Z

I think it's safe to ignore (for now anyway) the two failures in the special pummel run on CI. One is a timeout in a test that is not changed in this PR. It will need to be looked at, but it's an issue that's already there. And the other issue, with wrk not being installed, is also an existing issue.

The regular CI is unaffected, so I'm inclined to land and then go after those other pummel issues separately. If anyone objects to that approach, let me know, please! Thanks.

Trott · 2019-01-15T17:41:53Z

(wrk issue in test-keep-alive is addressed in #25516.)

Trott · 2019-01-15T18:38:52Z

(test-hash-seed issue is addressed in #25522.)

Trott · 2019-01-16T14:50:02Z

Landed in 2190d47...391f839

* Reduce concurrent and duration options by half so as to avoid interference with other tests. (Excessive TCP activity in this test resulted in throttling that caused subsequent tests to fail on my local setup.) * Use an OS-provided port rather than `common.PORT`. This possibly reduces side-effects on other tests (that may also be using `common.PORT`). * Add punctuation in comments. PR-URL: nodejs#25485 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

* Reduce ROUNDS and ATTEMPTS_PER_ROUND by half to avoid spurious test failures as a result of side effects from other tests. (For my local setup, test-keep-alive seems to cause this test to fail with ETIMEDOUT and/or EADDRNOTAVAIL. It would seem to be a result of throttling. Reducing the pummel-iness of that test and this one seems to solve the problem.) * Apply capitalization and punctuation to comment. PR-URL: nodejs#25485 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

* Use port 0 instead of `common.PORT`. * Reduce `concurrent` from 100 to 50 and `connections_per_client` from 5 to 3. This is to avoid side effects from other tests. Prior to this change, running this along with test-keep-alive would result in failures on my local setup, apparently due to network throttling. * Remove unnecessary `console.log()` and improve remaining `console.log()` to provide clearer information. PR-URL: nodejs#25485 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

* Use port 0 instead of `common.PORT`. * Use `//` for comments, capitalize comments, and add punctuation. PR-URL: nodejs#25485 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

* Reduce concurrent and duration options by half so as to avoid interference with other tests. (Excessive TCP activity in this test resulted in throttling that caused subsequent tests to fail on my local setup.) * Use an OS-provided port rather than `common.PORT`. This possibly reduces side-effects on other tests (that may also be using `common.PORT`). * Add punctuation in comments. PR-URL: #25485 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

* Reduce ROUNDS and ATTEMPTS_PER_ROUND by half to avoid spurious test failures as a result of side effects from other tests. (For my local setup, test-keep-alive seems to cause this test to fail with ETIMEDOUT and/or EADDRNOTAVAIL. It would seem to be a result of throttling. Reducing the pummel-iness of that test and this one seems to solve the problem.) * Apply capitalization and punctuation to comment. PR-URL: #25485 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

* Use port 0 instead of `common.PORT`. * Reduce `concurrent` from 100 to 50 and `connections_per_client` from 5 to 3. This is to avoid side effects from other tests. Prior to this change, running this along with test-keep-alive would result in failures on my local setup, apparently due to network throttling. * Remove unnecessary `console.log()` and improve remaining `console.log()` to provide clearer information. PR-URL: #25485 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

* Use port 0 instead of `common.PORT`. * Use `//` for comments, capitalize comments, and add punctuation. PR-URL: #25485 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>