Skip to content
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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

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

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

@Trott
Copy link
Member Author

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-github sam-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
Member Author

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

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

@Trott
Copy link
Member Author

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
Member Author

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
Member Author

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

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
Member Author

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
Member Author

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
Member Author

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

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
@addaleax addaleax mentioned this pull request Sep 17, 2019
Closed
3 tasks
@addaleax
Copy link
Member

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

@Trott
Copy link
Member Author

Trott commented Sep 17, 2019

Closing in favor of #29588

@Trott Trott 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 <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
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 <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
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 <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@Trott Trott deleted the mv-to-sequential branch January 13, 2022 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmarchini mmarchini mmarchini approved these changes

@lpinca lpinca lpinca approved these changes

@benjamingr benjamingr benjamingr approved these changes

@eugeneo eugeneo eugeneo approved these changes

@sam-github sam-github sam-github approved these changes

Assignees
No one assigned
Labels
fast-track PRs that do not need to wait for 48 hours to land. flaky-test Issues and PRs related to the tests with unstable failures on the CI. inspector Issues and PRs related to the V8 inspector protocol test Issues 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