test: port domains regression test from v0.10 by misterdjules · Pull Request #3356 · nodejs/node · GitHub

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

/cc @nodejs/collaborators

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

include common

Thank you, updated.

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

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.

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

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.

LGTM with two suggestions.

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

@jbergstroem @bnoordhuis Updated, please take a look.

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>

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

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

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>

Thank you, landed in 642928b.

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

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

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

test: port domains regression test from v0.10 by misterdjules · Pull Request #3356 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

test: port domains regression test from v0.10#3356misterdjules wants to merge 1 commit intonodejs:masternodejs/node:masterfrom misterdjules:port-domains-clears-stack-testmisterdjules/node-1:port-domains-clears-stack-testCopy head branch name to clipboard

Conversation

misterdjules commented Oct 14, 2015

Uh oh!

misterdjules commented Oct 14, 2015

Uh oh!

jbergstroem commented Oct 14, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

misterdjules commented Oct 14, 2015

Uh oh!

jbergstroem commented Oct 14, 2015

Uh oh!

jbergstroem commented Oct 14, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Oct 14, 2015

Uh oh!

misterdjules commented Oct 14, 2015

Uh oh!

misterdjules commented Oct 14, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

misterdjules commented Oct 15, 2015

Uh oh!

misterdjules commented Oct 16, 2015

Uh oh!

misterdjules commented Oct 16, 2015

Uh oh!

misterdjules commented Oct 16, 2015

Uh oh!

MylesBorins commented Oct 16, 2015

Uh oh!

misterdjules commented Oct 16, 2015

Uh oh!

jasnell commented Oct 28, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

test: port domains regression test from v0.10#3356
misterdjules wants to merge 1 commit into
nodejs:masterfrom
misterdjules:port-domains-clears-stack-test