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.

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

test: move common.PORT debug tests to sequential #13592

Closed
wants to merge 1 commit into from

Conversation

gibfahn
Copy link
Member

@gibfahn gibfahn commented Jun 10, 2017

They tend to hang if they happen to run in parallel with another test that uses common.PORT.

I missed these when looking at #13580, looks like two of these tests were competing with each other in the last run I did.

I've done a diff against the list in #12376, and these are the parallel debugger tests that contain common.PORT and are no longer in master:

  • test-debug-brk-no-arg.js
  • test-debug-brk.js (test: move test-debug-brk to sequential #13580)
  • test-debug-no-context.js
  • test-debug-port-cluster.js
  • test-debug-port-from-cmdline.js
  • test-debug-port-numbers.js
  • test-debug-signal-cluster.js
  • test-debugger-util-regression.js
grep results:
➜  parallel git:(v6.x-staging) ❯ rg common.PORT `fnd test-debug`                                        ~/wrk/com/node/test/parallel
test-debug-no-context.js
7:const args = ['--debug', `--debug-port=${common.PORT}`, '--interactive'];

test-debug-brk-no-arg.js
6:const child = spawn(process.execPath, ['--debug-brk=' + common.PORT, '-i']);

test-debug-port-cluster.js
6:const PORT_MIN = common.PORT + 1;  // The fixture uses common.PORT.

test-debug-signal-cluster.js
8:const port = common.PORT;

test-debug-brk.js
18:    const procArgs = [`--debug-brk=${common.PORT}`].concat(extraArgs);
25:        const agentArgs = ['debug', `localhost:${common.PORT}`];

test-debug-port-from-cmdline.js
6:const debugPort = common.PORT;

test-debug-port-numbers.js
16:  const port = common.PORT + i;

test-debugger-util-regression.js
16:  `--port=${common.PORT}`,
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@gibfahn gibfahn added the test Issues and PRs related to the tests. label Jun 10, 2017
@gibfahn gibfahn requested a review from Trott June 10, 2017 00:47
@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. v6.x labels Jun 10, 2017
@gibfahn
Copy link
Member Author

gibfahn commented Jun 10, 2017

Interested to hear any other method of solving this. I don't think we need to worry too much about these, they'll go away when 6.x goes away. Just trying to avoid any more flakes in CI.

Failures that prompted this:

not ok 920 parallel/test-debug-signal-cluster
  ---
  duration_ms: 60.14
  severity: fail
  stack: |-
    timeout
  ...

not ok 900 parallel/test-debug-brk
  ---
  duration_ms: 60.109
  severity: fail
  stack: |-
    timeout
  ...

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM

They tend to hang if they happen to run in parallel with another test
that uses common.PORT.

PR-URL: nodejs#13592
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
gibfahn added a commit to gibfahn/node that referenced this pull request Jun 17, 2017
They tend to hang if they happen to run in parallel with another test
that uses common.PORT.

PR-URL: nodejs#13592
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@gibfahn
Copy link
Member Author

gibfahn commented Jun 17, 2017

Landed in 7a22964

@gibfahn gibfahn closed this Jun 17, 2017
@gibfahn gibfahn deleted the seq-debug-sig branch June 17, 2017 21:46
gibfahn added a commit that referenced this pull request Jun 20, 2017
They tend to hang if they happen to run in parallel with another test
that uses common.PORT.

PR-URL: #13592
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
They tend to hang if they happen to run in parallel with another test
that uses common.PORT.

PR-URL: #13592
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
BethGriggs added a commit to BethGriggs/node that referenced this pull request Mar 1, 2018
Remove 'flaky' in parallel.status for test-debug-signal-cluster as the
test was moved to sequential.

Refs: nodejs#13592
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
Remove 'flaky' in parallel.status for test-debug-signal-cluster as the
test was moved to sequential.

Refs: #13592
PR-URL: #19069
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 28, 2018
Remove 'flaky' in parallel.status for test-debug-signal-cluster as the
test was moved to sequential.

Refs: #13592
PR-URL: #19069
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2018
Remove 'flaky' in parallel.status for test-debug-signal-cluster as the
test was moved to sequential.

Refs: #13592
PR-URL: #19069
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants