Skip to content

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

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.

Sign up for GitHub

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

Closed
Zahidul-Islam wants to merge 4 commits into from
Closed

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

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
60ac964
test: adding mustCall in test-timers-clearImmediate.js
ChatbotSchool Apr 22, 2017
8f439e2
test: update test-times-clearImmediate.js
ChatbotSchool Apr 22, 2017
69222b5
test: update test-times-clearImmediate.js
ChatbotSchool Apr 22, 2017
6eb7161
test: fixing lint error in test-times-clearImmediate.js
ChatbotSchool Apr 22, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 3 additions & 9 deletions test/parallel/test-timers-clearImmediate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,12 @@
'use strict';
require('../common');
const common = require('../common');
const assert = require('assert');

const N = 3;
let count = 0;
function next() {
const immediate = setImmediate(function() {
const immediate = setImmediate(common.mustCall(() => {
clearImmediate(immediate);
++count;
});
}, 3));
Copy link
Contributor

@mscdex mscdex Apr 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

}
for (let i = 0; i < N; ++i)
Copy link
Contributor

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

next();
Copy link
Contributor

@cjihrig cjihrig Apr 22, 2017

Choose a reason for hiding this comment

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

Does this test even do anything without this loop?

Copy link
Author

@Zahidul-Islam Zahidul-Islam Apr 22, 2017

Choose a reason for hiding this comment

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

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();
}

Copy link
Contributor

@cjihrig cjihrig Apr 22, 2017

Choose a reason for hiding this comment

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

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

  1. Put the for loop back.
  2. Remove the count variable.
  3. Drop the second argument to common.mustCall() as @mscdex said.


process.on('exit', () => {
assert.strictEqual(count, N, `Expected ${N} immediate callback executions`);
});