FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: improve debug-break-on-uncaught reliability by Trott · Pull Request #6793 · nodejs/node · GitHub

/ node Public

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

Closed
Trott wants to merge 3 commits into
nodejs:masterfrom
Trott:catch
ConversationCommits (3)ChecksFiles changed
Closed

test: improve debug-break-on-uncaught reliability#6793
Trott wants to merge 3 commits into
nodejs:masterfrom
Trott:catch

Conversation

Trott commented May 16, 2016

Copy link
Copy Markdown
Member
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 added debugger test Issues and PRs related to the tests. labels May 16, 2016

Trott commented May 16, 2016

Copy link
Copy Markdown
Member Author

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

jasnell commented May 16, 2016

Copy link
Copy Markdown
Member

LGTM if CI is green.

claudiorodriguez commented May 18, 2016

Copy link
Copy Markdown
Contributor

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 commented May 20, 2016

Copy link
Copy Markdown
Member Author

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

Trott commented May 21, 2016

Copy link
Copy Markdown
Member Author

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 commented May 21, 2016 •
edited
Loading

Copy link
Copy Markdown
Member Author

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 commented May 21, 2016

Copy link
Copy Markdown
Member Author

Squashed and force pushed. PTAL.

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

Trott commented May 21, 2016

Copy link
Copy Markdown
Member Author

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

bnoordhuis reviewed May 21, 2016
View reviewed changes
Comment thread test/debugger/test-debug-break-on-uncaught.js Outdated

bnoordhuis May 21, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Trott May 21, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@bnoordhuis True. Fixed.

bnoordhuis commented May 22, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

LGTM with a suggestion.

claudiorodriguez commented May 22, 2016

Copy link
Copy Markdown
Contributor

LGTM

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 commented May 24, 2016

Copy link
Copy Markdown
Member Author

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

bnoordhuis commented May 24, 2016

Copy link
Copy Markdown
Member

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

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

Trott added a commit to Trott/io.js that referenced this pull request May 24, 2016
test: improve debug-break-on-uncaught reliability
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 <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>

Trott commented May 24, 2016

Copy link
Copy Markdown
Member Author

Landed in ff00a48

Trott closed this May 24, 2016
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 30, 2016
test: improve debug-break-on-uncaught reliability
825723c 
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 <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
rvagg pushed a commit that referenced this pull request Jun 2, 2016
test: improve debug-break-on-uncaught reliability
38a3323 
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 <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
MylesBorins mentioned this pull request Jul 11, 2016
Closed

MylesBorins commented Jul 11, 2016

Copy link
Copy Markdown
Contributor

@Trott lts?

Trott commented Jul 11, 2016

Copy link
Copy Markdown
Member Author

@thealphanerd Yes, if it lands cleanly.

MylesBorins pushed a commit that referenced this pull request Jul 11, 2016
test: improve debug-break-on-uncaught reliability
5f679ab 
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 <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
test: improve debug-break-on-uncaught reliability
b6632a7 
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 <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
test: improve debug-break-on-uncaught reliability
8dd7eec 
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 <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
MylesBorins mentioned this pull request Jul 12, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
test: improve debug-break-on-uncaught reliability
188960d 
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 <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
test: improve debug-break-on-uncaught reliability
2a59e4e 
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 <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
Trott deleted the catch branch January 13, 2022 22:43
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL