test: move test-inspector-connect-main-thread to sequential by Trott · Pull Request #29582 · nodejs/node

Trott · 2019-09-16T18:52:14Z

test-inspector-connect-main-thread is dependent on timing and will be
more reliable in sequential.

Refs: https://github.com/nodejs/node/pull/28870/files#issuecomment-531906327

Assuming CI passes, I'd like to fast-track this to get CI back to yellow/green.

Checklist

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

test-inspector-connect-main-thread is dependent on timing and will be more reliable in `sequential`. Refs: https://github.com/nodejs/node/pull/28870/files#issuecomment-531906327

nodejs-github-bot · 2019-09-16T18:52:53Z

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

Trott · 2019-09-16T18:53:46Z

Started a node-daily-master just now for comparison with the CI results for this PR: https://ci.nodejs.org/job/node-daily-master/1679/

sam-github

RSLGTM

Trott · 2019-09-16T20:20:22Z

The tests for this PR had other failures but not test-inspector-connect-main-thread. The tests for master branch had multiple failures for test-inspector-connect-main-thread.

I'd like to fast-track this to get CI in better shape. @nodejs/collaborators Please 👍 here if you approve fast-tracking.

nodejs-github-bot · 2019-09-16T20:21:02Z

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

Trott · 2019-09-16T22:06:23Z

Ah, crud, it failed in sequential on a re-run. This isn't the fix.... It may be part of a fix, though. Will see if stress tests show anything....

Trott · 2019-09-16T22:11:28Z

Stress test running freebsd11-x64 against master with -J --repeat 192 10 times in parallel directory: https://ci.nodejs.org/job/node-stress-single-test/5/

Stress test running freebsd11-x64 against this PR with -J --repeat 192 10 times in sequential directory: https://ci.nodejs.org/job/node-stress-single-test/6/

(Hope I didn't mess up any of the configs and I don't have to re-do these to get them to compile.)

Trott · 2019-09-16T22:13:25Z

Stress test running freebsd11-x64 against master with -J --repeat 192 10 times in parallel directory: https://ci.nodejs.org/job/node-stress-single-test/5/

Stress test running freebsd11-x64 against this PR with -J --repeat 192 10 times in sequential directory: https://ci.nodejs.org/job/node-stress-single-test/6/

(Hope I didn't mess up any of the configs and I don't have to re-do these to get them to compile.)

Welp...compilation failed for both of them, but doesn't look like anything I did, I don't think. Trying again with freebsd 10....

Master: https://ci.nodejs.org/job/node-stress-single-test/7/

This PR: https://ci.nodejs.org/job/node-stress-single-test/8/

addaleax · 2019-09-16T22:18:18Z

If you do move this to sequential, can you open an issue about it, because moving to sequential may improve reliability but not solve the underlying issue?

Trott · 2019-09-16T22:21:16Z

If you do move this to sequential, can you open an issue about it, because moving to sequential may improve reliability but not solve the underlying issue?

Yes, I will do that if this lands.

Trott · 2019-09-16T22:23:18Z

If you do move this to sequential, can you open an issue about it, because moving to sequential may improve reliability but not solve the underlying issue?

Yes, I will do that if this lands.

Aside: It would be great to have an audit of all the tests in sequential to add explanatory comments to the ones that really need to be in sequential while opening issues for the ones that ideally should be refactored and moved. There are currently 121 tests in sequential so that's an ambitious undertaking but not an impossible one.

Trott · 2019-09-16T23:50:54Z

Welp, the stress test compilation failed after 47 minutes. sigh

eugeneo · 2019-09-17T05:42:56Z

I do not have access to a workstation, but can you revert the commit 3d841fe? I will take another look at the test when I get an opportunity.

addaleax · 2019-09-17T10:49:44Z

Fwiw, I’ve found that replacing the console.log()s in post() in the test file with process._rawDebug() solve this issue locally for me. I’m not sure why yet, but given that the test also checks console.log()’s interaction with the inspector, maybe that’s not too surprising.

It still fails about 3/1000 times with the crash in #28870 (comment) afterwards, but that seems like a genuine bug in Node.js either way (whether introduced by #28870 or not).

Using `console.log()` likely interferes with the functionality of the test, which also checks the interaction between inspector and `console.log()` as part of the test. Using `process._rawDebug()` solves that issue. Refs: nodejs#28870 Refs: nodejs#29582

addaleax · 2019-09-17T11:56:21Z

#29588 and, to lesser degree, #29587, should solve this problem as an alternative to moving the test.

Trott · 2019-09-17T18:10:32Z

Closing in favor of #29588

Using `console.log()` likely interferes with the functionality of the test, which also checks the interaction between inspector and `console.log()` as part of the test. Using `process._rawDebug()` solves that issue. Refs: nodejs#28870 Refs: nodejs#29582 PR-URL: nodejs#29588 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Using `console.log()` likely interferes with the functionality of the test, which also checks the interaction between inspector and `console.log()` as part of the test. Using `process._rawDebug()` solves that issue. Refs: #28870 Refs: #29582 PR-URL: #29588 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

test: move test-inspector-connect-main-thread to sequential

d218077

test-inspector-connect-main-thread is dependent on timing and will be more reliable in `sequential`. Refs: https://github.com/nodejs/node/pull/28870/files#issuecomment-531906327

Trott added inspector Issues and PRs related to the V8 inspector protocol flaky-test Issues and PRs related to the tests with unstable failures on the CI. fast-track PRs that do not need to wait for 48 hours to land. labels Sep 16, 2019

nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 16, 2019

Trott mentioned this pull request Sep 16, 2019

inspector: new API - Session.connectToMainThread #28870

Merged

4 tasks

eugeneo approved these changes Sep 16, 2019

View reviewed changes

sam-github approved these changes Sep 16, 2019

View reviewed changes

lpinca approved these changes Sep 16, 2019

View reviewed changes

mmarchini approved these changes Sep 16, 2019

View reviewed changes

addaleax mentioned this pull request Sep 17, 2019

test: fix flaky test-inspector-connect-main-thread #29588

Closed

3 tasks

benjamingr approved these changes Sep 17, 2019

View reviewed changes

Trott force-pushed the master branch from 1ecc406 to 49cf67e Compare September 17, 2019 16:50

Trott closed this Sep 17, 2019

Trott deleted the mv-to-sequential branch January 13, 2022 22:52

Uh oh!

Conversation

Trott commented Sep 16, 2019

Checklist

Uh oh!

nodejs-github-bot commented Sep 16, 2019

Uh oh!

Trott commented Sep 16, 2019

Uh oh!

sam-github left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Sep 16, 2019

Uh oh!

nodejs-github-bot commented Sep 16, 2019

Uh oh!

Trott commented Sep 16, 2019

Uh oh!

Trott commented Sep 16, 2019

Uh oh!

Trott commented Sep 16, 2019

Uh oh!

addaleax commented Sep 16, 2019

Uh oh!

Trott commented Sep 16, 2019

Uh oh!

Trott commented Sep 16, 2019

Uh oh!

Trott commented Sep 16, 2019

Uh oh!

eugeneo commented Sep 17, 2019

Uh oh!

addaleax commented Sep 17, 2019

Uh oh!

addaleax commented Sep 17, 2019

Uh oh!

Trott commented Sep 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants