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

test: fix domain-top-level-error-handler-throw by santigimeno · Pull Request #4364 · nodejs/node · GitHub

/ node Public

test: fix domain-top-level-error-handler-throw - #4364

Closed
santigimeno wants to merge 1 commit into
nodejs:masterfrom
santigimeno:fix_domain_top_level
Closed

test: fix domain-top-level-error-handler-throw#4364
santigimeno wants to merge 1 commit into
nodejs:masterfrom
santigimeno:fix_domain_top_level

Conversation

Copy link
Copy Markdown
Member

Check the stderr output in the close event as it's not guaranteed to be fully
available when the exit event is fired.
It tries to fix #4206.

r-52 added domain Issues and PRs related to the domain subsystem. test Issues and PRs related to the tests. labels Dec 20, 2015

Copy link
Copy Markdown

I ran that test as:

while true; do ./node test/parallel/test-domain-top-level-error-handler-throw.js; done

and I haven't been able to see it fail. Did you manage to reproduce the flakiness of this test?

A minor nit to the commit message: the first line of the description is longer than 72 characters, but that can be changed when landing it, so no worries.

Other than that, LGTM and we don't necessarily have to reproduce the flakiness to land this change, as it's definitely more correct to wait for 'close' to be emitted to check for stderr output.

Added lts-watch-* labels as this test is present in all LTS branches with the same issue.

Copy link
Copy Markdown

Started CI tests.

Check the stderr output in the `close` event as it's not guaranteed to
be fully available when the `exit` event is fired.

Copy link
Copy Markdown
Member Author

@misterdjules I could reproduce in OS X the issue by running a bunch of the tests in parallel. First I made multiple copies of the test: test/parallel/test-domain-top-level-error-handler-throw-1.js, etc. and then:

while true; do /usr/bin/python tools/test.py --mode=release parallel/test-domain-top-level-error-handler-throw* -J; done

I've already fixed the description. Thanks!

Copy link
Copy Markdown

@santigimeno I couldn't reproduce the problem on my OSX setup even when running multiple instances of test/parallel/test-domain-top-level-error-handler-throw.js in parallel. Nevertheless, the change in this PR is a good fix, and CI tests pass. Landing asap.

Thank you!

misterdjules pushed a commit that referenced this pull request Dec 23, 2015
Check the stderr output in the `close` event as it's not guaranteed to
be fully available when the `exit` event is fired.

PR: #4364
PR-URL: #4364
Reviewed-By: Julien Gilli <jgilli@fastmail.fm>

Copy link
Copy Markdown

Landed in cf50305.

Trott commented Dec 24, 2015

Copy link
Copy Markdown
Member

It appears that the code in this PR is breaking CI. While CI was run for this PR, it looks like it was not run on the final version of the commit. It was run on an initial version. Maybe something broke that would have been detected by a CI run with the commit rebased against master? Not sure what happened. Still trying to figure it out.

The specific test broken is in the addons. Here's an example from https://ci.nodejs.org/job/node-test-commit-osx/1553/nodes=osx1010/console:

not ok 918 hello.js
# module.js:433
#   return process.dlopen(module, path._makeLong(filename));
#                  ^
# 
# Error: Module did not self-register.
#     at Error (native)
#     at Object.Module._extensions..node (module.js:433:18)
#     at Module.load (module.js:354:32)
#     at Function.Module._load (module.js:311:12)
#     at Module.require (module.js:364:17)
#     at require (internal/module.js:12:17)
#     at Object.<anonymous> (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/test/addons/doc-1/hello.js:2:15)
#     at Module._compile (module.js:408:26)
#     at Object.Module._extensions..js (module.js:415:10)
#     at Module.load (module.js:354:32)

Copy link
Copy Markdown
Contributor

I'll have a PR in just a moment to revert this

MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Dec 24, 2015
PR-URL: nodejs#4410

As @Trott points out in nodejs#4364 commit cf50305 is currently
breaking CI. This commit reverts the changes.

Copy link
Copy Markdown
Contributor

I'm reopening this now that the new PR is in

MylesBorins reopened this Dec 24, 2015

Copy link
Copy Markdown
Contributor

This does not appear to be breaking things

Trott commented Dec 24, 2015

Copy link
Copy Markdown
Member

Apologies for the bad diagnosis!

MylesBorins pushed a commit that referenced this pull request Dec 30, 2015
Check the stderr output in the `close` event as it's not guaranteed to
be fully available when the `exit` event is fired.

PR: #4364
PR-URL: #4364
Reviewed-By: Julien Gilli <jgilli@fastmail.fm>
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Jan 6, 2016
Check the stderr output in the `close` event as it's not guaranteed to
be fully available when the `exit` event is fired.

PR: nodejs#4364
PR-URL: nodejs#4364
Reviewed-By: Julien Gilli <jgilli@fastmail.fm>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
Check the stderr output in the `close` event as it's not guaranteed to
be fully available when the `exit` event is fired.

PR: #4364
PR-URL: #4364
Reviewed-By: Julien Gilli <jgilli@fastmail.fm>
MylesBorins mentioned this pull request Jan 19, 2016
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Check the stderr output in the `close` event as it's not guaranteed to
be fully available when the `exit` event is fired.

PR: nodejs#4364
PR-URL: nodejs#4364
Reviewed-By: Julien Gilli <jgilli@fastmail.fm>
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain Issues and PRs related to the domain subsystem. test Issues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Investigate flaky test-domain-top-level-error-handler-throw

5 participants


Back | FazBrowse Home | New Git URL