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: add test-domain-exit-dispose-again back #4256

Conversation

misterdjules
Copy link

1c85849 "fixed"
test-domain-exit-dispose-again by changing its logic to test that
process.domain was cleared properly in case an error was thrown from a
timer's callback.

However, it became clear when reviewing a recent change that refactors
lib/timers.js that it was not quite the intention of the original test.
Thus, this change adds the original implementation of
test-domain-exit-dispose-again back, with comments that make its
implementation easier to understand.

It also preserve the changes made by
1c85849, but it moves them to a new
test file named test-timers-reset-process-domain-on-throw.js.

/cc @Fishrock123 This is the test I mentioned in #4007 (comment).

1c85849 "fixed"
test-domain-exit-dispose-again by changing its logic to test that
process.domain was cleared properly in case an error was thrown from a
timer's callback.

However, it became clear when reviewing a recent change that refactors
lib/timers.js that it was not quite the intention of the original test.
Thus, this change adds the original implementation of
test-domain-exit-dispose-again back, with comments that make its
implementation easier to understand.

It also preserve the changes made by
1c85849, but it moves them to a new
test file named test-timers-reset-process-domain-on-throw.js.
@misterdjules misterdjules added domain Issues and PRs related to the domain subsystem. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. test Issues and PRs related to the tests. lts-watch-v4.x labels Dec 12, 2015
@misterdjules
Copy link
Author

CI tests running.

@misterdjules misterdjules mentioned this pull request Dec 12, 2015
@Fishrock123
Copy link
Contributor

Tests seem fine, I'll test it on the PR with and without the related code when I get the chance.

@Fishrock123
Copy link
Contributor

Note: my timers branch does not have 1c85849, and the old version doesn't catch the relevant change in timers.

if (process.domain !== null) {
console.log('process.domain should be null, but instead is:',
process.domain);
process.exit(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually we avoid using process.exit() in tests because it is rather immediate, is there a way this could be switched to Throw?

Copy link
Author

Choose a reason for hiding this comment

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

The problem is explained by the comment above:

// Do not use assert here, as it throws errors and if a domain with an error
// handler is active, then asserting wouldn't make the test fail.

maybe this comment should be moved right above the call to process.exit(1)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, probably. And of course, domains, oopsie.

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good, I'll move it closer to the call to process.exit.

@misterdjules
Copy link
Author

@Fishrock123 Just to make sure we're on the same page: do you mean that the version of test/parallel/test-domain-exit-dispose-again.js prior to 1c85849 doesn't fail if process.domain is not reset after a timer's callback threw? That's what I'd expect, and I would expect both 1c85849 and the changes in this PR to make that case fail.

@misterdjules
Copy link
Author

/ping @Fishrock123 Did you have some time to look at my answers to your questions/comments?

@misterdjules
Copy link
Author

@Fishrock123 Pushed a new commit that I believe addresses your review comments. If that looks good to you, I'll squash the two commits in one. Thanks!

@Fishrock123
Copy link
Contributor

@Fishrock123 Just to make sure we're on the same page: do you mean that the version of test/parallel/test-domain-exit-dispose-again.js prior to 1c85849 doesn't fail if process.domain is not reset after a timer's callback threw? That's what I'd expect, and I would expect both 1c85849 and the changes in this PR to make that case fail.

I don't understand the details, but ok!

Rubber-stamp LGTM as I don't feel like I can adequately assess the tests' details, but since this is just re-introducing an old test it should be fine.

CI again, for good measure: https://ci.nodejs.org/job/node-test-pull-request/1144/

misterdjules pushed a commit that referenced this pull request Jan 5, 2016
1c85849 "fixed"
test-domain-exit-dispose-again by changing its logic to test that
process.domain was cleared properly in case an error was thrown from a
timer's callback.

However, it became clear when reviewing a recent change that refactors
lib/timers.js that it was not quite the intention of the original test.
Thus, this change adds the original implementation of
test-domain-exit-dispose-again back, with comments that make its
implementation easier to understand.

It also preserve the changes made by
1c85849, but it moves them to a new
test file named test-timers-reset-process-domain-on-throw.js.

PR: #4256
PR-URL: #4256
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@misterdjules
Copy link
Author

Thank you @Fishrock123. Landed in 1453b87.

Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Jan 6, 2016
1c85849 "fixed"
test-domain-exit-dispose-again by changing its logic to test that
process.domain was cleared properly in case an error was thrown from a
timer's callback.

However, it became clear when reviewing a recent change that refactors
lib/timers.js that it was not quite the intention of the original test.
Thus, this change adds the original implementation of
test-domain-exit-dispose-again back, with comments that make its
implementation easier to understand.

It also preserve the changes made by
1c85849, but it moves them to a new
test file named test-timers-reset-process-domain-on-throw.js.

PR: nodejs#4256
PR-URL: nodejs#4256
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 13, 2016
1c85849 "fixed"
test-domain-exit-dispose-again by changing its logic to test that
process.domain was cleared properly in case an error was thrown from a
timer's callback.

However, it became clear when reviewing a recent change that refactors
lib/timers.js that it was not quite the intention of the original test.
Thus, this change adds the original implementation of
test-domain-exit-dispose-again back, with comments that make its
implementation easier to understand.

It also preserve the changes made by
1c85849, but it moves them to a new
test file named test-timers-reset-process-domain-on-throw.js.

PR: #4256
PR-URL: #4256
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
1c85849 "fixed"
test-domain-exit-dispose-again by changing its logic to test that
process.domain was cleared properly in case an error was thrown from a
timer's callback.

However, it became clear when reviewing a recent change that refactors
lib/timers.js that it was not quite the intention of the original test.
Thus, this change adds the original implementation of
test-domain-exit-dispose-again back, with comments that make its
implementation easier to understand.

It also preserve the changes made by
1c85849, but it moves them to a new
test file named test-timers-reset-process-domain-on-throw.js.

PR: #4256
PR-URL: #4256
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 19, 2016
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
1c85849 "fixed"
test-domain-exit-dispose-again by changing its logic to test that
process.domain was cleared properly in case an error was thrown from a
timer's callback.

However, it became clear when reviewing a recent change that refactors
lib/timers.js that it was not quite the intention of the original test.
Thus, this change adds the original implementation of
test-domain-exit-dispose-again back, with comments that make its
implementation easier to understand.

It also preserve the changes made by
1c85849, but it moves them to a new
test file named test-timers-reset-process-domain-on-throw.js.

PR: nodejs#4256
PR-URL: nodejs#4256
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@misterdjules misterdjules deleted the readd-test-domain-exit-dispose-again branch July 24, 2017 17:35
ryzokuken added a commit to ryzokuken/node that referenced this pull request Mar 8, 2018
Rename the test appropriately alongside mentioning the subsystem
Also, make a few basic changes to make sure the test conforms
to the standard test structure

Refs: nodejs#19105
Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 11, 2018
Rename the tests appropriately alongside mentioning the subsystem.
Also, make a few basic changes to make sure the test conforms
to the standard test structure.

This renames:
- test-regress-nodejsGH-1531
- test-regress-nodejsGH-2245
- test-regress-nodejsGH-3238
- test-regress-nodejsGH-3542
- test-regress-nodejsGH-3739
- test-regress-nodejsGH-4256

PR-URL: nodejs#19212
Refs: nodejs#19105
Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Mar 17, 2018
Rename the tests appropriately alongside mentioning the subsystem.
Also, make a few basic changes to make sure the test conforms
to the standard test structure.

This renames:
- test-regress-GH-1531
- test-regress-GH-2245
- test-regress-GH-3238
- test-regress-GH-3542
- test-regress-GH-3739
- test-regress-GH-4256

PR-URL: #19212
Refs: #19105
Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
Rename the tests appropriately alongside mentioning the subsystem.
Also, make a few basic changes to make sure the test conforms
to the standard test structure.

This renames:
- test-regress-GH-1531
- test-regress-GH-2245
- test-regress-GH-3238
- test-regress-GH-3542
- test-regress-GH-3739
- test-regress-GH-4256

PR-URL: #19212
Refs: #19105
Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
Rename the tests appropriately alongside mentioning the subsystem.
Also, make a few basic changes to make sure the test conforms
to the standard test structure.

This renames:
- test-regress-nodejsGH-1531
- test-regress-nodejsGH-2245
- test-regress-nodejsGH-3238
- test-regress-nodejsGH-3542
- test-regress-nodejsGH-3739
- test-regress-nodejsGH-4256

PR-URL: nodejs#19212
Refs: nodejs#19105
Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Oct 2, 2018
Rename the tests appropriately alongside mentioning the subsystem.
Also, make a few basic changes to make sure the test conforms
to the standard test structure.

This renames:
- test-regress-GH-1531
- test-regress-GH-2245
- test-regress-GH-3238
- test-regress-GH-3542
- test-regress-GH-3739
- test-regress-GH-4256

PR-URL: #19212
Refs: #19105
Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
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. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants