test: port domains regression test from v0.10 #3356

misterdjules · 2015-10-14T01:50:29Z

f2a45ca contained a test for a
regression that had been introduced by the original change that
77a10ed ported. While
77a10ed did not contain that
regression, the test that f2a45ca
contained should still be in the code base to prevent any regression
from happening in the future.

Original message for the commit that contained the test:

domains: fix stack clearing after error handled

caeb677 introduced a regression where
the domains stack would not be cleared after an error had been handled
by the top-level domain.

This change clears the domains stack regardless of the position of the
active domain in the stack.

PR: #9364
PR-URL: nodejs/node-v0.x-archive#9364
Reviewed-By: Trevor Norris trev.norris@gmail.com
Reviewed-By: Julien Gilli julien.gilli@joyent.com

misterdjules · 2015-10-14T01:50:59Z

/cc @nodejs/collaborators

jbergstroem · 2015-10-14T02:34:19Z

CI: https://ci.nodejs.org/job/node-test-commit/832/

lint issue:

test/parallel/test-domain-top-level-error-handler-clears-stack.js
  1:0  error  Mandatory module "common" must be loaded  required-modules

thefourtheye · 2015-10-14T03:26:14Z

test/parallel/test-domain-top-level-error-handler-clears-stack.js

@@ -0,0 +1,32 @@
+'use strict';
+


include common

Thank you, updated.

misterdjules · 2015-10-14T05:54:03Z

@jbergstroem @thefourtheye Thanks for catching the lint issue, and my apologies for not running make test in the first place.

jbergstroem · 2015-10-14T06:40:14Z

Heading into cosmetics here, but could we perhaps use the shortform git commit sha's instead? Github automatically formats to this which makes the commit message look a bit weird unless read through -HTML.

jbergstroem · 2015-10-14T06:40:35Z

https://github.com/misterdjules/node-1/commit/6feb95386a24ee5a4b678995943da70fd9e4723f.patch vs misterdjules@6feb953

bnoordhuis · 2015-10-14T08:58:14Z

test/parallel/test-domain-top-level-error-handler-clears-stack.js

+      // handled the original error, thus throwing here would trigger another
+      // call to process._fatalException, and so on recursively and
+      // indefinitely.
+      process.exit(1);


Maybe use process.abort() instead?

What is the advantage of using process.abort over process.exit in this case? One disadvantage I can think of is that it could create core files on some systems depending on their configuration.

Yes, that's a disadvantage. The advantage is that it's a lot more noisy than process.exit(1) is.

It will make this test take quite a bit longer on OS X at least too

Agreed. What about adding a noisy console.error output?

Updated this PR and added a console.error output before exiting, let me know what you think.

bnoordhuis · 2015-10-14T08:59:25Z

LGTM with two suggestions.

misterdjules · 2015-10-14T16:47:57Z

@jbergstroem @bnoordhuis Thanks for the review, will update shortly.

@jbergstroem Regarding using full commits shas: I always use full shas when I want to make sure that they can be found a long time after the commit message was written. It's difficult to determine how long git commit shas should be to never be ambiguous throughout the lifetime of the project. So instead of relying on guessing, I always include the full sha and I'm confident that anyone will be able to use them and unambiguously retrieve their corresponding changes.

misterdjules · 2015-10-14T16:54:04Z

@jbergstroem @bnoordhuis Updated, please take a look.

bnoordhuis · 2015-10-14T17:00:38Z

test/parallel/test-domain-top-level-error-handler-clears-stack.js

+'use strict';
+
+const domain = require('domain');
+const common = require('../common');


common should be the first import.

Right, updated, thank you.

f2a45ca contained a test for a regression that had been introduced by the original change that 77a10ed ported. While 77a10ed did not contain that regression, the test that f2a45ca contained should still be in the code base to prevent any regression from happening in the future. Original message for the commit that contained the test: domains: fix stack clearing after error handled caeb677 introduced a regression where the domains stack would not be cleared after an error had been handled by the top-level domain. This change clears the domains stack regardless of the position of the active domain in the stack. PR: nodejs#9364 PR-URL: nodejs/node-v0.x-archive#9364 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Julien Gilli <julien.gilli@joyent.com>

misterdjules · 2015-10-15T16:35:52Z

@jbergstroem @bnoordhuis ping. Let me know if my latest comments and updates address your concerns.

misterdjules · 2015-10-16T17:20:53Z

CI: https://ci.nodejs.org/job/node-test-commit/860/.

misterdjules · 2015-10-16T18:19:13Z

All tests failures seem to be known flaky tests:

Landing soon.

f2a45ca contained a test for a regression that had been introduced by the original change that 77a10ed ported. While 77a10ed did not contain that regression, the test that f2a45ca contained should still be in the code base to prevent any regression from happening in the future. Original message for the commit that contained the test: domains: fix stack clearing after error handled caeb677 introduced a regression where the domains stack would not be cleared after an error had been handled by the top-level domain. This change clears the domains stack regardless of the position of the active domain in the stack. PR: #9364 PR-URL: nodejs/node-v0.x-archive#9364 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Julien Gilli <julien.gilli@joyent.com> PR: #3356 PR-URL: #3356 Reviewed-By: Ben Noordhuis <ben@strongloop.com>

misterdjules · 2015-10-16T18:23:08Z

Thank you, landed in 642928b.

MylesBorins · 2015-10-16T18:24:03Z

@jasnell will tests like this make their ways into LTS?

misterdjules · 2015-10-16T18:47:11Z

@thealphanerd That's a good question. My opinion is that yes, they should because they can only help make LTS releases more solid. Hence tagging this PR with land-on-v4.x.

f2a45ca contained a test for a regression that had been introduced by the original change that 77a10ed ported. While 77a10ed did not contain that regression, the test that f2a45ca contained should still be in the code base to prevent any regression from happening in the future. Original message for the commit that contained the test: domains: fix stack clearing after error handled caeb677 introduced a regression where the domains stack would not be cleared after an error had been handled by the top-level domain. This change clears the domains stack regardless of the position of the active domain in the stack. PR: #9364 PR-URL: nodejs/node-v0.x-archive#9364 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Julien Gilli <julien.gilli@joyent.com> PR: #3356 PR-URL: #3356 Reviewed-By: Ben Noordhuis <ben@strongloop.com>

jasnell · 2015-10-28T15:24:58Z

Landed in v4.x-staging in b8869bd

f2a45ca contained a test for a regression that had been introduced by the original change that 77a10ed ported. While 77a10ed did not contain that regression, the test that f2a45ca contained should still be in the code base to prevent any regression from happening in the future. Original message for the commit that contained the test: domains: fix stack clearing after error handled caeb677 introduced a regression where the domains stack would not be cleared after an error had been handled by the top-level domain. This change clears the domains stack regardless of the position of the active domain in the stack. PR: #9364 PR-URL: nodejs/node-v0.x-archive#9364 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Julien Gilli <julien.gilli@joyent.com> PR: #3356 PR-URL: #3356 Reviewed-By: Ben Noordhuis <ben@strongloop.com>

misterdjules added domain Issues and PRs related to the domain subsystem. test Issues and PRs related to the tests. labels Oct 14, 2015

thefourtheye reviewed Oct 14, 2015
View reviewed changes

misterdjules force-pushed the port-domains-clears-stack-test branch from 37e358e to 8a0518e Compare October 14, 2015 05:45

misterdjules force-pushed the port-domains-clears-stack-test branch from 8a0518e to 6feb953 Compare October 14, 2015 06:00

bnoordhuis reviewed Oct 14, 2015
View reviewed changes

misterdjules force-pushed the port-domains-clears-stack-test branch from 6feb953 to b2d7301 Compare October 14, 2015 16:53

bnoordhuis reviewed Oct 14, 2015
View reviewed changes

misterdjules force-pushed the port-domains-clears-stack-test branch from b2d7301 to 8b58dfa Compare October 14, 2015 17:09

misterdjules closed this Oct 16, 2015

misterdjules added the land-on-v4.x label Oct 16, 2015

rvagg mentioned this pull request Oct 21, 2015

Release proposal: v5.0.0 #3466

Merged

rvagg mentioned this pull request Oct 28, 2015

Preparing v4.2.2 LTS Maintenance nodejs/Release#50

Closed

jasnell added the lts-watch-v4.x label Oct 28, 2015

jasnell removed the lts-watch-v4.x label Oct 28, 2015

jasnell removed the land-on-v4.x label Nov 3, 2015

misterdjules deleted the port-domains-clears-stack-test branch July 24, 2017 17:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: port domains regression test from v0.10 #3356

test: port domains regression test from v0.10 #3356

misterdjules commented Oct 14, 2015

misterdjules commented Oct 14, 2015

jbergstroem commented Oct 14, 2015

thefourtheye Oct 14, 2015

misterdjules Oct 14, 2015

misterdjules commented Oct 14, 2015

jbergstroem commented Oct 14, 2015

jbergstroem commented Oct 14, 2015

bnoordhuis Oct 14, 2015

misterdjules Oct 14, 2015

bnoordhuis Oct 14, 2015

evanlucas Oct 14, 2015

misterdjules Oct 14, 2015

misterdjules Oct 14, 2015

bnoordhuis commented Oct 14, 2015

misterdjules commented Oct 14, 2015

misterdjules commented Oct 14, 2015

bnoordhuis Oct 14, 2015

misterdjules Oct 14, 2015

misterdjules commented Oct 15, 2015

misterdjules commented Oct 16, 2015

misterdjules commented Oct 16, 2015

misterdjules commented Oct 16, 2015

MylesBorins commented Oct 16, 2015

misterdjules commented Oct 16, 2015

jasnell commented Oct 28, 2015

test: port domains regression test from v0.10 #3356

test: port domains regression test from v0.10 #3356

Conversation

misterdjules commented Oct 14, 2015

misterdjules commented Oct 14, 2015

jbergstroem commented Oct 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misterdjules commented Oct 14, 2015

jbergstroem commented Oct 14, 2015

jbergstroem commented Oct 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented Oct 14, 2015

misterdjules commented Oct 14, 2015

misterdjules commented Oct 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misterdjules commented Oct 15, 2015

misterdjules commented Oct 16, 2015

misterdjules commented Oct 16, 2015

misterdjules commented Oct 16, 2015

MylesBorins commented Oct 16, 2015

misterdjules commented Oct 16, 2015

jasnell commented Oct 28, 2015