test: fix setTimeout, remove unused err #10193

amazingandyyy · 2016-12-09T08:11:27Z

file: test/parallel/test-domain-uncaught-exception.js

There are three setTimeout() in the file and they do not specify a
duration (the second argument), so I change them to setImmediate()
instead.

There are four callbacks that take an argument called err but that
argument is never used, I just removed them.

sam-github · 2016-12-09T15:00:01Z

test/parallel/test-domain-timers.js

@@ -14,7 +15,7 @@ timeoutd.on('error', common.mustCall(function(e) {
 }));

 timeoutd.run(function() {
-  setTimeout(function() {
+  setTimeout(function onTimeout() {


sam-github · 2016-12-09T15:02:23Z

test/parallel/test-domain-uncaught-exception.js

@@ -23,7 +23,7 @@ function test1() {
   */
  const d = domain.create();
  d.run(function() {
-    setTimeout(function onTimeout() {
+    setImmediate(function onSetImmediate() {


The test used to test that exceptions thrown from setTimeout were in the domain, now they test that test thrown from setImmediate are still in the domain! I don't think you are looking at what the test is doing.

I understand your point. Since it does not specify a duration (the second argument), I think it's more appropriate to change this call to setImmediate() instead of setTimeout().

For this test, instead of changing the setTimeout() to setImmediate(), it is better to keep setTimeout() but add a duration of 1. @sam-github is right. This is testing a timer, not an immediate. Changing it to an immediate changes the test. In other words, the setTimeout() is not an incidental tool used within the test. It is part of what is actually being tested.

(In @amazingandyyy's defense, I'm the one that suggested changing setTimeout() with no duration to setImmediate() in the first place. But yeah, that's not right in this test.)

sam-github · 2016-12-09T15:03:22Z

test/parallel/test-domain-uncaught-exception.js

    process.send('errorHandledByDomain');
  });

  d5.run(function() {
    d6.run(function() {
-      setTimeout(function onTimeout() {
+      setImmediate(function onSetImmediate() {


ditto. the point of the test isn't to run these functions, its to test interactions with timeouts and domains

same here, it does not specify a duration (the second argument), setImmediate is more appropriate.

@sam-github is (as usual) right on this one too. We're testing that domains and setTimeout() interact the way they are supposed to, so it needs to be setTimeout(). Add a duration of 1 rather than changing it to setImmediate().

sam-github

This looks to me like a combination of random code churn, and unintended changes to the function of the tests.

Trott · 2016-12-09T23:58:20Z

I don't think the changes to test-domain-timers.js belong here. Can we please limit the scope of this PR to just test-domain-uncaught-exception.js?

Trott · 2016-12-10T00:01:28Z

test/parallel/test-domain-uncaught-exception.js

    process.send('errorHandledByDomain');
  });

  d9.run(function() {
    d10.run(function() {
-      setTimeout(function onTimeout() {
+      setImmediate(function onSetImmediate() {


Same here: Use setTimeout() with a duration of 1 instead of setImmediate().

sam-github · 2016-12-13T18:03:35Z

Commits need to be squashed

addaleax

LGTM

Squashing commits can also be done by whoever lands the PR, btw.

sam-github · 2016-12-13T19:30:54Z

@addaleax, it can, but last time I did that @mhdawson suggested I ask the PR author. Rebasing is part of the work flow, so knowing how to do that helps get people up to speed with node work habits. On the other hand, doing it for them makes it easier. I don't care.

@amazingandyyy do you want to squash, or do you want me to?

addaleax · 2016-12-13T21:28:16Z

@sam-github Right, but squashing and rebasing can be tricky depending on somebody’s git experience level, so I like to mention that it’s optional.

amazingandyyy · 2016-12-13T22:06:28Z

@sam-github @addaleax I just do "git rebase -i HEAD~6" and then replace "pick" by "squash" then save the file.

I am not sure if this works? I still see 6 commits on this PR. I appreciate this practicing chance.

btw, Please feel free to squash it(if my rebasing doesn't work) when landing, thanks!

italoacasas · 2016-12-13T22:09:10Z

@amarzavery the last step is to comment all the msg except one (the one that you are going to use as a commit msg)

addaleax · 2016-12-13T22:09:29Z

@amazingandyyy That sounds about right, except you’ll need to keep the first commit as pick

file: test/parallel/test-domain-uncaught-exception.js 1. There are three setTimeout() in the file and they do not specify a duration (the second argument), so I change them to setImmediate() instead. 2. There are four callbacks that take an argument called `err` but that argument is never used, so I removed them.

amazingandyyy · 2016-12-13T22:32:32Z

Thank you! @italoacasas and @addaleax

I leave one as pick and clean commit message, then got

amazingandyyy ~/projects/node $ git rebase -i HEAD~6
[detached HEAD 8d0badb] minor update to test/parallel/test-domain-uncaught-exception.js
 Date: Fri Dec 9 00:03:13 2016 -0800
 1 file changed, 7 insertions(+), 7 deletions(-)
Successfully rebased and updated refs/heads/master.

but I still see 6 commits on this PR, do I miss any step or what's next step to complete(make the squashing work?) Thanks

mhdawson

LGTM

mhdawson · 2016-12-14T17:57:22Z

CI run: https://ci.nodejs.org/job/node-test-pull-request/5404/

mhdawson · 2016-12-14T18:25:31Z

@amazingandyyy just noticed that the commit message does not fit our contributor guidelines. I'll fix up as part of landing but can you review the guidelines for the next time.

The commit message should start with a prefix. In this case 'test:' and the length needs to be less than or equal to 50 chars. See https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#step-3-commit

mhdawson · 2016-12-14T18:28:45Z

CI failure on arm was #10073, so not related to this change so CI is good. Will land.

mhdawson · 2016-12-14T18:40:46Z

@amazingandyyy one more thing for the next time as well. We need your full name in the commit. I've amended while landed but fore the next time can you make sure its set to:

Andy Chen <[email protected]>

mhdawson · 2016-12-14T18:44:14Z

landed as e27c2a7

file: test/parallel/test-domain-uncaught-exception.js 1. There are three setTimeout() in the file and they do not specify a duration (the second argument), so I change them to setImmediate() instead. 2. There are four callbacks that take an argument called `err` but that argument is never used, so I removed them. PR-URL: #10193 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

amazingandyyy · 2016-12-14T19:09:31Z

@mharsch thank you! I will follow contributor guidelines and add my full name next time! Thank you for posting reference!

Trott · 2016-12-14T22:44:35Z

We need your full name in the commit.

We do not need anyone's name for commits. It is up to them.

Trott · 2016-12-14T22:49:32Z

(Always good to ask first-time contributors, though, of course! People forget to set the name all the time. So, it's a nice gesture to make sure they are making a conscious choice in the matter. I'm just reacting to the "need your full name" phrasing. If someone wants to be "Pat" or "avatar-46", that's their choice.)

file: test/parallel/test-domain-uncaught-exception.js 1. There are three setTimeout() in the file and they do not specify a duration (the second argument), so I change them to setImmediate() instead. 2. There are four callbacks that take an argument called `err` but that argument is never used, so I removed them. PR-URL: #10193 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

nodejs-github-bot added test Issues and PRs related to the tests. lts-watch-v6.x labels Dec 9, 2016

amazingandyyy changed the title ~~minor update to test/parallel/test-domain-uncaught-exception.js~~ test: fix setTimeout() and remove uunusable "err" in test-domain-uncaught-exception Dec 9, 2016

amazingandyyy changed the title ~~test: fix setTimeout() and remove uunusable "err" in test-domain-uncaught-exception~~ test: fix setTimeout() and remove unused "err" in test-domain-uncaught-exception Dec 9, 2016

nodejs-github-bot added the lts-watch-v6.x label Dec 9, 2016

Fishrock123 removed the lts-watch-v4.x label Dec 9, 2016

sam-github reviewed Dec 9, 2016

View reviewed changes

sam-github suggested changes Dec 9, 2016

View reviewed changes

mscdex added domain Issues and PRs related to the domain subsystem. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels Dec 9, 2016

nodejs-github-bot added the dont-land-on-v7.x label Dec 9, 2016

amazingandyyy changed the title ~~test: fix setTimeout() and remove unused "err" in test-domain-uncaught-exception~~ test: fix setTimeout, remove unused err, add const Dec 9, 2016

Trott reviewed Dec 10, 2016

View reviewed changes

nodejs-github-bot added the dont-land-on-v7.x label Dec 10, 2016

amazingandyyy changed the title ~~test: fix setTimeout, remove unused err, add const~~ test: fix setTimeout, remove unused err Dec 10, 2016

Fishrock123 removed the dont-land-on-v7.x label Dec 12, 2016

sam-github approved these changes Dec 13, 2016

View reviewed changes

addaleax approved these changes Dec 13, 2016

View reviewed changes

Fishrock123 added lts-watch-v4.x and removed dont-land-on-v7.x labels Dec 13, 2016

lpinca approved these changes Dec 14, 2016

View reviewed changes

mhdawson approved these changes Dec 14, 2016

View reviewed changes

mhdawson closed this Dec 14, 2016

italoacasas mentioned this pull request Dec 15, 2016

v7.3.0 proposal #10277

Merged

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jan 23, 2017

This was referenced Jan 24, 2017

v4.8.0 proposal #10973

Merged

v6.10.0 proposal #10974

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: fix setTimeout, remove unused err #10193

test: fix setTimeout, remove unused err #10193

amazingandyyy commented Dec 9, 2016

sam-github Dec 9, 2016

sam-github Dec 9, 2016

amazingandyyy Dec 9, 2016

Trott Dec 9, 2016

sam-github Dec 9, 2016

amazingandyyy Dec 9, 2016

Trott Dec 9, 2016 •

edited

Loading

sam-github left a comment

Trott commented Dec 9, 2016

Trott Dec 10, 2016

sam-github commented Dec 13, 2016

addaleax left a comment

sam-github commented Dec 13, 2016

addaleax commented Dec 13, 2016

amazingandyyy commented Dec 13, 2016

italoacasas commented Dec 13, 2016

addaleax commented Dec 13, 2016

amazingandyyy commented Dec 13, 2016

mhdawson left a comment

mhdawson commented Dec 14, 2016

mhdawson commented Dec 14, 2016

mhdawson commented Dec 14, 2016

mhdawson commented Dec 14, 2016

mhdawson commented Dec 14, 2016

amazingandyyy commented Dec 14, 2016

Trott commented Dec 14, 2016 •

edited

Loading

Trott commented Dec 14, 2016 •

edited

Loading

test: fix setTimeout, remove unused err #10193

test: fix setTimeout, remove unused err #10193

Conversation

amazingandyyy commented Dec 9, 2016

sam-github Dec 9, 2016

Choose a reason for hiding this comment

sam-github Dec 9, 2016

Choose a reason for hiding this comment

amazingandyyy Dec 9, 2016

Choose a reason for hiding this comment

Trott Dec 9, 2016

Choose a reason for hiding this comment

sam-github Dec 9, 2016

Choose a reason for hiding this comment

amazingandyyy Dec 9, 2016

Choose a reason for hiding this comment

Trott Dec 9, 2016 • edited Loading

Choose a reason for hiding this comment

sam-github left a comment

Choose a reason for hiding this comment

Trott commented Dec 9, 2016

Trott Dec 10, 2016

Choose a reason for hiding this comment

sam-github commented Dec 13, 2016

addaleax left a comment

Choose a reason for hiding this comment

sam-github commented Dec 13, 2016

addaleax commented Dec 13, 2016

amazingandyyy commented Dec 13, 2016

italoacasas commented Dec 13, 2016

addaleax commented Dec 13, 2016

amazingandyyy commented Dec 13, 2016

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Dec 14, 2016

mhdawson commented Dec 14, 2016

mhdawson commented Dec 14, 2016

mhdawson commented Dec 14, 2016

mhdawson commented Dec 14, 2016

amazingandyyy commented Dec 14, 2016

Trott commented Dec 14, 2016 • edited Loading

Trott commented Dec 14, 2016 • edited Loading

Trott Dec 9, 2016 •

edited

Loading

Trott commented Dec 14, 2016 •

edited

Loading

Trott commented Dec 14, 2016 •

edited

Loading