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: fix flaky inspector-cli tests when breakpoints are restored #38431

Merged
merged 1 commit into from
May 3, 2021
Merged

test: fix flaky inspector-cli tests when breakpoints are restored #38431

merged 1 commit into from
May 3, 2021

Conversation

Trott
Copy link
Member

@Trott Trott commented Apr 27, 2021

Sometimes, there isn't a newine in the AIX output that already has a
comment indicating it needs investigation.

@github-actions github-actions bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Apr 27, 2021
@Trott
Copy link
Member Author

Trott commented Apr 27, 2021

Refs: https://ci.nodejs.org/job/node-test-commit-aix/36437/nodes=aix72-ppc64/console

00:30:58 not ok 3190 inspector-cli/test-inspector-cli-pid
00:31:08   ---
00:31:08   duration_ms: 10.872
00:31:08   severity: fail
00:31:09   exitcode: 1
00:31:09   stack: |-
00:31:09     node:internal/process/promises:246
00:31:09               triggerUncaughtException(err, true /* fromPromise */);
00:31:09               ^
00:31:09     
00:31:09     AssertionError [ERR_ASSERTION]: ifError got unwanted exception: Timeout (10000) while waiting for />\s+(?:\n1 breakpoints restored\.)?$/; found: Warning: script 'alive.js' was not loaded yet.
00:31:09     debug> 
00:31:09     break in test/fixtures/inspector-cli/alive.js:3
00:31:09       1 let x = 0;
00:31:09       2 function heartbeat() {
00:31:09     > 3   ++x;
00:31:09       4 }
00:31:09       5 setInterval(heartbeat, 50);
00:31:09     debug> 1 breakpoints restored.
00:31:09     
00:31:09         at Timeout.<anonymous> (/home/iojs/build/workspace/node-test-commit-aix/nodes/aix72-ppc64/test/common/inspector-cli.js:84:18)
00:31:09         at listOnTimeout (node:internal/timers:557:17)
00:31:09         at processTimers (node:internal/timers:500:7) {
00:31:09       generatedMessage: false,
00:31:09       code: 'ERR_ASSERTION',
00:31:09       actual: Error: Timeout (10000) while waiting for />\s+(?:\n1 breakpoints restored\.)?$/; found: Warning: script 'alive.js' was not loaded yet.
00:31:09       debug> 
00:31:09       break in test/fixtures/inspector-cli/alive.js:3
00:31:09         1 let x = 0;
00:31:09         2 function heartbeat() {
00:31:09       > 3   ++x;
00:31:09         4 }
00:31:09         5 setInterval(heartbeat, 50);
00:31:09       debug> 1 breakpoints restored.
00:31:09       
00:31:09           at Timeout.<anonymous> (/home/iojs/build/workspace/node-test-commit-aix/nodes/aix72-ppc64/test/common/inspector-cli.js:84:18)
00:31:09           at listOnTimeout (node:internal/timers:557:17)
00:31:09           at processTimers (node:internal/timers:500:7),
00:31:09       expected: null,
00:31:09       operator: 'ifError'
00:31:09     }
00:31:09   ...

@Trott Trott added aix Issues and PRs related to the AIX platform. 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 labels Apr 27, 2021
@nodejs-github-bot
Copy link
Collaborator

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

RaisinTen
RaisinTen reviewed Apr 27, 2021
View reviewed changes
test/inspector-cli/test-inspector-cli-pid.js Outdated
@@ -49,7 +49,7 @@ function launchTarget(...args) {
// .then(() => cli.waitForPrompt())
//
// What we're diong for now:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// What we're diong for now:
// What we're doing for now:

nit: typo from an earlier change

@richardlau
Copy link
Member

Is there perhaps some deeper underlying issue with the inspector-cli tests?
Today's daily failed in inspector_cli_test_inspector_cli_address: https://ci.nodejs.org/job/node-test-commit-aix/36460/nodes=aix72-ppc64/testReport/junit/(root)/test/inspector_cli_test_inspector_cli_address/

@Trott
Copy link
Member Author

Trott commented Apr 27, 2021

Is there perhaps some deeper underlying issue with the inspector-cli tests?
Today's daily failed in inspector_cli_test_inspector_cli_address: https://ci.nodejs.org/job/node-test-commit-aix/36460/nodes=aix72-ppc64/testReport/junit/(root)/test/inspector_cli_test_inspector_cli_address/

Yeah, I think it's all one issue that:

  • Should be addressed in waitPrompt() in test/common/inspector-cli.js rather than done piecemeal throughout the files. I'll look at opening a PR to do that.
  • Doesn't really affect end users and might not even really be considered a bug. I'm guessing it has to do with buffering output somewhere.

I've been wondering if it makes sense to differentiate stdout and stderr in the debugger. The prompt should go to stdout, I imagine, but the warning messages should go to stderr perhaps. If we then keep them separate in the tests, it should be easy to make them robust.

@Trott
Copy link
Member Author

Trott commented Apr 29, 2021

OK, I've generalized the workaround. PTAL

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@Trott Trott changed the title test: fix flaky inspector-cli/test-inspector-cli-pid on AIX test: fix flaky inspector-cli tests when breakpoints are restored May 3, 2021
@Trott
test: fix flaky inspector-cli tests when breakpionts are restored
18e4f40 
PR-URL: nodejs#38431
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott Trott merged commit 18e4f40 into nodejs:master May 3, 2021
@Trott
Copy link
Member Author

Trott commented May 3, 2021

Landed in 18e4f40

@Trott Trott deleted the pid-fix branch May 3, 2021 16:00
targos pushed a commit that referenced this pull request May 17, 2021
@Trott @targos
test: fix flaky inspector-cli tests when breakpionts are restored
e64ebac 
PR-URL: #38431
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request May 18, 2021
Merged
@Trott Trott mentioned this pull request Jun 5, 2021
Closed
targos pushed a commit to Trott/io.js that referenced this pull request Jun 6, 2021
@Trott @targos
test: fix flaky inspector-cli tests when breakpionts are restored
0ea6546 
PR-URL: nodejs#38431
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit to Trott/io.js that referenced this pull request Jun 6, 2021
@Trott @targos
test: fix flaky inspector-cli tests when breakpionts are restored
7c8bc20 
PR-URL: nodejs#38431
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@bl-ue bl-ue mentioned this pull request Jun 12, 2021
Closed
@tniessen tniessen mentioned this pull request Jul 7, 2021
Merged
richardlau pushed a commit that referenced this pull request Jul 19, 2021
@Trott @richardlau
test: fix flaky inspector-cli tests when breakpionts are restored
7f2017d 
PR-URL: #38431
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@richardlau
Copy link
Member

This didn't seem to be included in #38858 but does cherry-pick cleanly across to v14.x-staging after that landed.

richardlau pushed a commit that referenced this pull request Jul 20, 2021
@Trott @richardlau
test: fix flaky inspector-cli tests when breakpionts are restored
99fd8bf 
PR-URL: #38431
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@richardlau richardlau mentioned this pull request Jul 20, 2021
Merged
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
@Trott @foxxyz
test: fix flaky inspector-cli tests when breakpionts are restored
dbcbb13 
PR-URL: nodejs#38431
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RaisinTen RaisinTen RaisinTen left review comments

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@richardlau richardlau richardlau approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
aix Issues and PRs related to the AIX platform. 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 needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Trott @nodejs-github-bot @richardlau @jasnell @gireeshpunathil @RaisinTen @targos