Skip to content

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

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom
Closed
Trott wants to merge 1 commit into from
Closed

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

Trott wants to merge 1 commit into from

Conversation

@Trott
Copy link
Member

@TrottTrott commented Sep 16, 2019

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

@Trott
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
@TrottTrott 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-botnodejs-github-bot added the test Issues and PRs related to the tests. label Sep 16, 2019
@TrottTrott mentioned this pull request Sep 16, 2019
Merged
4 tasks
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 16, 2019

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

@Trott
Copy link
MemberAuthor

Trott commented Sep 16, 2019

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
sam-github approved these changes Sep 16, 2019
View reviewed changes
Copy link
Contributor

@sam-githubsam-github left a comment

Choose a reason for hiding this comment

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

RSLGTM

@Trott
Copy link
MemberAuthor

Trott commented Sep 16, 2019

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
Copy link
Collaborator

nodejs-github-bot commented Sep 16, 2019

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

@Trott
Copy link
MemberAuthor

Trott commented Sep 16, 2019

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
Copy link
MemberAuthor

Trott commented Sep 16, 2019

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
Copy link
MemberAuthor

Trott commented Sep 16, 2019

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
Copy link
Member

addaleax commented Sep 16, 2019

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
Copy link
MemberAuthor

Trott commented Sep 16, 2019

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
Copy link
MemberAuthor

Trott commented Sep 16, 2019

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
Copy link
MemberAuthor

Trott commented Sep 16, 2019

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

@eugeneo
Copy link
Contributor

eugeneo commented Sep 17, 2019

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
Copy link
Member

addaleax commented Sep 17, 2019

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).

addaleax added a commit to addaleax/node that referenced this pull request Sep 17, 2019
@addaleax
test: fix flaky test-inspector-connect-main-thread
b1314e0
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
@addaleaxaddaleax mentioned this pull request Sep 17, 2019
Closed
3 tasks
@addaleax
Copy link
Member

addaleax commented Sep 17, 2019

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

@Trott
Copy link
MemberAuthor

Trott commented Sep 17, 2019

Closing in favor of #29588

@TrottTrott closed this Sep 17, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Sep 17, 2019
@addaleax@Trott
test: fix flaky test-inspector-connect-main-thread
3adec43
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 <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit that referenced this pull request Sep 20, 2019
@addaleax@targos
test: fix flaky test-inspector-connect-main-thread
3014fb5
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 <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
BridgeAR pushed a commit that referenced this pull request Sep 25, 2019
@addaleax@BridgeAR
test: fix flaky test-inspector-connect-main-thread
78ee065
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 <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
@TrottTrott deleted the mv-to-sequential branch January 13, 2022 22:52
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjamingrbenjamingrbenjamingr approved these changes

@lpincalpincalpinca approved these changes

+3 more reviewers

@sam-githubsam-githubsam-github approved these changes

@eugeneoeugeneoeugeneo approved these changes

@mmarchinimmarchinimmarchini approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

fast-trackPRs that do not need to wait for 48 hours to land.flaky-testIssues and PRs related to the tests with unstable failures on the CI.inspectorIssues and PRs related to the V8 inspector protocoltestIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@Trott@nodejs-github-bot@addaleax@eugeneo@sam-github@benjamingr@lpinca@mmarchini