test: improve debug-break-on-uncaught reliability #6793

Trott · 2016-05-16T20:58:19Z

Checklist

tests and code linting passes
the commit message follows commit guidelines

Affected core subsystem(s)

test debugger

Description of change

Move test-debug-break-on-uncaught from debugger directory to
sequential so that it gets exercised by make test and via the
continuous integration server for the project.

Removed unnecessary port number modification that is probable source of
unreliability on CI.

Trott · 2016-05-16T20:58:45Z

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

jasnell · 2016-05-16T21:06:44Z

LGTM if CI is green.

claudiorodriguez · 2016-05-18T23:49:09Z

Test is failing on several platforms with this:

not ok 1116 sequential/test-debug-break-on-uncaught
# 
# assert.js:90
#   throw new assert.AssertionError({
#   ^
# AssertionError: debugger did not pause on exception
#     at assertHasPaused (/home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1204-64/test/sequential/test-debug-break-on-uncaught.js:91:12)
#     at tryOnTimeout (timers.js:224:11)
#     at Timer.listOnTimeout (timers.js:198:5)
# **[ global.js ]**
# **[ timeout.js ]**

Trott · 2016-05-20T23:15:48Z

CI again to see if platforms are failing consistently or not: https://ci.nodejs.org/job/node-test-pull-request/2718/

Trott · 2016-05-21T03:33:26Z

The use of setTimeout() seemed suspicious so I refactored the test to check what was going on a bit more carefully, ran another CI, and sure enough...

https://ci.nodejs.org/job/node-test-commit-linux/3463/nodes=ubuntu1404-64/console

not ok 1121 sequential/test-debug-break-on-uncaught
# 
# assert.js:90
#   throw new assert.AssertionError({
#   ^
# AssertionError: no exceptions thrown, race condition in test?
#     at assertHasPaused (/home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1404-64/test/sequential/test-debug-break-on-uncaught.js:91:5)
#     at tryOnTimeout (timers.js:224:11)
#     at Timer.listOnTimeout (timers.js:198:5)
# **[ global.js ]**
# **[ timeout.js ]**
  ---
  duration_ms: 1.10

Trott · 2016-05-21T06:08:51Z

The latest change makes stuff better but still lots of fail. I'm going to move it back to test/debugger where it won't bother anyone but leave the improvements in case someone else wants to pick it up...

Trott · 2016-05-21T06:09:56Z

Squashed and force pushed. PTAL.

Trott · 2016-05-21T06:14:58Z

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

bnoordhuis · 2016-05-21T09:25:46Z

test/debugger/test-debug-break-on-uncaught.js

-  setTimeout(setupClient.bind(null, runTest), 200);
+
+  child.stderr.on('data', (data) => {
+    if (data.toString().includes('Debugger listening on port')) {


Assumes that lines correspond with data events. I'd concatenate the output and scan that.

@bnoordhuis True. Fixed.

bnoordhuis · 2016-05-22T06:51:13Z

LGTM with a suggestion.

claudiorodriguez · 2016-05-22T15:36:14Z

LGTM

Running the test through CI reveals unreliability due to a race condition. These changes mitigate the race condition, but do not eliminate it.

Trott · 2016-05-24T06:34:38Z

implemented @bnoordhuis suggestion...onward to CI: https://ci.nodejs.org/job/node-test-pull-request/2755/

bnoordhuis · 2016-05-24T08:48:27Z

Still LGTM. Another java.nio.file.DirectoryNotEmptyException on vcbt2015.

/cc @nodejs/build - known issue? Happened several times yesterday, too.

Running the test through CI reveals unreliability due to a race condition. These changes mitigate the race condition, but do not eliminate it. PR-URL: nodejs#6793 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Claudio Rodriguez <[email protected]>

Trott · 2016-05-24T20:45:06Z

Landed in ff00a48

Running the test through CI reveals unreliability due to a race condition. These changes mitigate the race condition, but do not eliminate it. PR-URL: nodejs#6793 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Claudio Rodriguez <[email protected]>

Running the test through CI reveals unreliability due to a race condition. These changes mitigate the race condition, but do not eliminate it. PR-URL: #6793 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Claudio Rodriguez <[email protected]>

MylesBorins · 2016-07-11T21:34:32Z

@Trott lts?

Trott · 2016-07-11T22:37:42Z

@thealphanerd Yes, if it lands cleanly.

Running the test through CI reveals unreliability due to a race condition. These changes mitigate the race condition, but do not eliminate it. PR-URL: #6793 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Claudio Rodriguez <[email protected]>

Trott added debugger test Issues and PRs related to the tests. labels May 16, 2016

Trott force-pushed the catch branch from 2dcedfa to 0e8603c Compare May 20, 2016 23:14

Trott force-pushed the catch branch from 3183976 to 74f77d7 Compare May 21, 2016 06:09

Trott force-pushed the catch branch from 74f77d7 to 90aca00 Compare May 21, 2016 06:12

Trott changed the title ~~test: move debug-break-on-uncaught to sequential~~ test: improve debug-break-on-uncaught reliability May 21, 2016

Trott force-pushed the catch branch from 90aca00 to 2eea63f Compare May 21, 2016 06:12

bnoordhuis reviewed May 21, 2016
View reviewed changes

Trott added 3 commits May 22, 2016 20:50

test: improve debug-break-on-uncaught reliability

e003338

Running the test through CI reveals unreliability due to a race condition. These changes mitigate the race condition, but do not eliminate it.

squash

dcf13b2

squash: encoding

b1eda6b

Trott force-pushed the catch branch from f2f026f to b1eda6b Compare May 23, 2016 03:54

Trott closed this May 24, 2016

MylesBorins mentioned this pull request Jul 11, 2016

Audit commits not found on v4.x-staging #7661

Closed

MylesBorins added the lts-watch-v4.x label Jul 11, 2016

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jul 11, 2016

MylesBorins mentioned this pull request Jul 12, 2016

v4.5.0 proposal #7688

Merged

Trott deleted the catch branch January 13, 2022 22:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: improve debug-break-on-uncaught reliability #6793

test: improve debug-break-on-uncaught reliability #6793

Trott commented May 16, 2016

Trott commented May 16, 2016

jasnell commented May 16, 2016

claudiorodriguez commented May 18, 2016

Trott commented May 20, 2016

Trott commented May 21, 2016

Trott commented May 21, 2016 •

edited

Loading

Trott commented May 21, 2016

Trott commented May 21, 2016

bnoordhuis May 21, 2016

Trott May 21, 2016

bnoordhuis commented May 22, 2016 •

edited

Loading

claudiorodriguez commented May 22, 2016

Trott commented May 24, 2016

bnoordhuis commented May 24, 2016

Trott commented May 24, 2016

MylesBorins commented Jul 11, 2016

Trott commented Jul 11, 2016

test: improve debug-break-on-uncaught reliability #6793

test: improve debug-break-on-uncaught reliability #6793

Conversation

Trott commented May 16, 2016

Checklist

Affected core subsystem(s)

Description of change

Trott commented May 16, 2016

jasnell commented May 16, 2016

claudiorodriguez commented May 18, 2016

Trott commented May 20, 2016

Trott commented May 21, 2016

Trott commented May 21, 2016 • edited Loading

Trott commented May 21, 2016

Trott commented May 21, 2016

bnoordhuis May 21, 2016

Choose a reason for hiding this comment

Trott May 21, 2016

Choose a reason for hiding this comment

bnoordhuis commented May 22, 2016 • edited Loading

claudiorodriguez commented May 22, 2016

Trott commented May 24, 2016

bnoordhuis commented May 24, 2016

Trott commented May 24, 2016

MylesBorins commented Jul 11, 2016

Trott commented Jul 11, 2016

Trott commented May 21, 2016 •

edited

Loading

bnoordhuis commented May 22, 2016 •

edited

Loading