test: adding mustCall in test-timers-clearImmediate.js by Zahidul-Islam · Pull Request #12598 · nodejs/node

Zahidul-Islam · 2017-04-22T19:50:52Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

mscdex · 2017-04-22T19:51:56Z

test/parallel/test-timers-clearImmediate.js

    clearImmediate(immediate);
    ++count;
-  });
+  }, 3));


~~I think we should just use N here instead.~~ I think we just need common.mustCall() without a second argument when the loop below is kept.

mscdex · 2017-04-22T19:52:32Z

test/parallel/test-timers-clearImmediate.js

-  });
+  }, 3));
 }
-for (let i = 0; i < N; ++i)


I think this loop is still needed?

cjihrig · 2017-04-22T19:53:34Z

test/parallel/test-timers-clearImmediate.js

+  }, 3));
 }
-for (let i = 0; i < N; ++i)
-  next();


Does this test even do anything without this loop?

Is the below code work? It doesn't complain when I run test.

'use strict'; const common = require('../common'); const assert = require('assert'); const N = 3; let count = 0; function next() { const immediate = setImmediate(common.mustCall(() => { clearImmediate(immediate); ++count; })); } for(let i = 0; i < N; i++) { next(); }

If it doesn't complain, that means the test is passing (or possibly not even doing anything). I think you should:

Put the for loop back.

Remove the count variable.

Drop the second argument to common.mustCall() as @mscdex said.

Zahidul-Islam · 2017-04-22T20:11:32Z

@cjihrig I have updated my commit.

mscdex · 2017-04-22T20:13:15Z

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

cjihrig

LGTM once it passes CI. You look to have some linting errors though. Try running make lint locally.

Zahidul-Islam · 2017-04-22T20:39:03Z

@cjihrig I have fixed lint error.

mscdex · 2017-04-22T21:14:58Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/7604/

Trott · 2017-04-23T04:03:19Z

Linux CI issues are build/infrastructure issues and nothing to do with the changes here. CI looks good.

Fishrock123 · 2017-04-24T15:42:28Z

test/parallel/test-timers-clearImmediate.js

-  });
+  const immediate = setImmediate(
+                        common.mustCall(() => clearImmediate(immediate))
+                    );


Is this indentation off?

jasnell

LGTM with linting errors fixed

PR-URL: nodejs#12598 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Trott · 2017-04-26T04:11:48Z

Landed in c405497.
Thanks for the contribution! 🎉

PR-URL: #12598 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs/node#12598 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

test: adding mustCall in test-timers-clearImmediate.js

60ac964

nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 22, 2017

mscdex added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Apr 22, 2017

mscdex reviewed Apr 22, 2017

View reviewed changes

test/parallel/test-timers-clearImmediate.js

});

}, 3));

}

for (let i = 0; i < N; ++i)

Copy link

Contributor

mscdex Apr 22, 2017

Choose a reason for hiding this comment

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

I think this loop is still needed?

cjihrig requested changes Apr 22, 2017

View reviewed changes

ChatbotSchool added 2 commits April 22, 2017 13:03

test: update test-times-clearImmediate.js

8f439e2

test: update test-times-clearImmediate.js

69222b5

cjihrig approved these changes Apr 22, 2017

View reviewed changes

addaleax added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Apr 22, 2017

test: fixing lint error in test-times-clearImmediate.js

6eb7161

addaleax approved these changes Apr 23, 2017

View reviewed changes

lpinca approved these changes Apr 23, 2017

View reviewed changes

Fishrock123 reviewed Apr 24, 2017

View reviewed changes

jasnell approved these changes Apr 24, 2017

View reviewed changes

Trott closed this Apr 26, 2017

evanlucas mentioned this pull request May 1, 2017

v7.10.0 proposal #12775

Merged

gibfahn added the land-on-v6.x label May 16, 2017

MylesBorins mentioned this pull request May 23, 2017

v6.11.0 proposal #13059

Merged

Uh oh!

Conversation

Zahidul-Islam commented Apr 22, 2017

Checklist

Affected core subsystem(s)

Uh oh!

mscdex Apr 22, 2017 • 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!

mscdex Apr 22, 2017

Choose a reason for hiding this comment

Uh oh!

cjihrig Apr 22, 2017

Choose a reason for hiding this comment

Uh oh!

Zahidul-Islam Apr 22, 2017

Choose a reason for hiding this comment

Uh oh!

cjihrig Apr 22, 2017

Choose a reason for hiding this comment

Uh oh!

Zahidul-Islam commented Apr 22, 2017

Uh oh!

mscdex commented Apr 22, 2017

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

Zahidul-Islam commented Apr 22, 2017

Uh oh!

mscdex commented Apr 22, 2017

Uh oh!

Trott commented Apr 23, 2017

Uh oh!

Fishrock123 Apr 24, 2017 • 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!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Apr 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

mscdex Apr 22, 2017 •

edited

Loading

Fishrock123 Apr 24, 2017 •

edited

Loading