test: fix setTimeout, remove unused err by amazingandyyy · Pull Request #10193 · nodejs/node · GitHub

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.

why?

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

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().

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

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?

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

Commits need to be squashed

LGTM

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

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

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

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

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

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

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

LGTM

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

@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

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

@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 <amazingandyyy@gmail.com>

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 <sam@strongloop.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

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

We need your full name in the commit.

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

(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 <sam@strongloop.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

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

test: fix setTimeout, remove unused err by amazingandyyy · Pull Request #10193 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: fix setTimeout, remove unused err - #10193

test: fix setTimeout, remove unused err#10193amazingandyyy wants to merge 1 commit intonodejs:masternodejs/node:masterfrom amazingandyyy:masteramazingandyyy/node:masterCopy head branch name to clipboard

Conversation

amazingandyyy commented Dec 9, 2016

Uh oh!

sam-github Dec 9, 2016

Choose a reason for hiding this comment

Uh oh!

sam-github Dec 9, 2016

Choose a reason for hiding this comment

Uh oh!

amazingandyyy Dec 9, 2016

Choose a reason for hiding this comment

Uh oh!

Trott Dec 9, 2016

Choose a reason for hiding this comment

Uh oh!

sam-github Dec 9, 2016

Choose a reason for hiding this comment

Uh oh!

amazingandyyy Dec 9, 2016

Choose a reason for hiding this comment

Uh oh!

Trott Dec 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sam-github left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Dec 9, 2016

Uh oh!

Trott Dec 10, 2016

Choose a reason for hiding this comment

Uh oh!

sam-github commented Dec 13, 2016

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

sam-github commented Dec 13, 2016

Uh oh!

addaleax commented Dec 13, 2016

Uh oh!

amazingandyyy commented Dec 13, 2016

Uh oh!

italoacasas commented Dec 13, 2016

Uh oh!

addaleax commented Dec 13, 2016

Uh oh!

amazingandyyy commented Dec 13, 2016

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Dec 14, 2016

Uh oh!

mhdawson commented Dec 14, 2016

Uh oh!

mhdawson commented Dec 14, 2016

Uh oh!

mhdawson commented Dec 14, 2016

Uh oh!

mhdawson commented Dec 14, 2016

Uh oh!

amazingandyyy commented Dec 14, 2016

Uh oh!

Trott commented Dec 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Dec 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

test: fix setTimeout, remove unused err#10193
amazingandyyy wants to merge 1 commit into
nodejs:masterfrom
amazingandyyy:master

Trott Dec 9, 2016 •
edited
Loading

Trott commented Dec 14, 2016 •
edited
Loading

Trott commented Dec 14, 2016 •
edited
Loading