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: fix domain-top-level-error-handler-throw #4364

Closed
wants to merge 1 commit into from

Conversation

santigimeno
Copy link
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 r-52 added domain Issues and PRs related to the domain subsystem. test Issues and PRs related to the tests. labels Dec 20, 2015
@misterdjules
Copy link

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.

@misterdjules
Copy link

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.
@santigimeno
Copy link
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!

@misterdjules
Copy link

@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>
@misterdjules
Copy link

Landed in cf50305.

@Trott
Copy link
Member

Trott commented Dec 24, 2015

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)

@MylesBorins
Copy link
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.
@MylesBorins
Copy link
Contributor

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

@MylesBorins MylesBorins reopened this Dec 24, 2015
@MylesBorins
Copy link
Contributor

This does not appear to be breaking things

@Trott
Copy link
Member

Trott commented Dec 24, 2015

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 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>
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