test: add logging for test-debug-port-cluster #6769
Conversation
|
Refs: #6754 |
|
The refactoring in this PR appears to resolve the test flakiness issue. Stress test with this PR: https://ci.nodejs.org/job/node-stress-single-test/722/nodes=freebsd102-64/console Stress test with current master: https://ci.nodejs.org/job/node-stress-single-test/721/nodes=freebsd102-64/console |
| for (let port = PORT_MIN; port <= PORT_MAX; port += 1) { | ||
| assert(stderr.includes(`Debugger listening on port ${port}`)); | ||
| } | ||
| } |
There was a problem hiding this comment.
There should really be some at-exit check that verifies whether this code is reached.
There was a problem hiding this comment.
@bnoordhuis Ah, yes, good point. I've extracted the for loop to its own function and wrapped it in common.mustCall() so there will be an at-exit check that the function ran. Stress testing now to see if that re-introduces the problem or not. (If it does, I guess my "solution" was to not check when the problem arose. :-( )
The test is currently flaky and CI provides no real information because the test times out rather than failing on an assertion. Add logging to gather more information about the failure. Refs: nodejs#6754
|
LGTM. This PR's stress test is green. |
|
Stress tests appear kind of meaningless because for each stress test, the test either always succeeds or always fails. But the logging appears to be helpfule: |
Remove port increment by `1337` which appears to sometimes result in a port that is already in use. There is no reason not to use `common.PORT`.
|
Removed the |
|
FreeBSD failure appears unrelated. Opened a separate issue for it. |
|
Running CI three more times just to make sure the flakiness is really truly gone. (Stress tests don't seem to be meaningful for this one, although I guess they could be useful to avoid exercising all the other tests unnecessarily. ¯_(ツ)_/¯) |
The test is currently flaky and CI provides no real information because the test times out rather than failing on an assertion. Add logging to gather more information about the failure. Refs: nodejs#6754 PR-URL: nodejs#6769 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Remove port increment by `1337` which appears to sometimes result in a port that is already in use. There is no reason not to use `common.PORT`. PR-URL: nodejs#6769 Fixes: nodejs#6754 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
|
marking as |
Checklist
Affected core subsystem(s)
test debugger
Description of change
The test is currently flaky and CI provides no real information because
the test times out rather than failing on an assertion. Add logging to
gather more information about the failure.