test: add test cases for setUnrefTimeout. by kakts · Pull Request #20945 · nodejs/node

kakts · 2018-05-24T17:45:44Z

Summary

Added some test cases for setUnrefTimeout in lib/internal/timers.js

This PR improves the test coverage of it.
https://coverage.nodejs.org/coverage-9a02de7084e3f3c9/root/internal/timers.js.html#L92

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

Trott

LGTM if CI is good

apapirovski

Thanks for working on this @kakts. I think a few small changes are needed before we get it landed.

I don't think this really belongs in this file. The refresh test for each of these isn't needed. I would create a new file test-timers-setunreftimeout.js or something similar.

test/parallel/test-timers-refresh.js

kakts · 2018-05-29T08:32:45Z

@apapirovski
I'll move these test cases to test-timers-setunreftimeout.js.

kakts · 2018-05-31T17:34:18Z

I moved test cases to test/parallel/test-timers-setunreftimeout.js.
And also refactored them.

test/parallel/test-timers-setunreftimeout.js

kakts · 2018-06-01T15:26:20Z

@apapirovski Thank you for the reviewing. I fixed them.

Fishrock123

The tests do not pass. Please run ./configure && make -j4 test on your local machine!

test/parallel/test-timers-setunreftimeout.js

kakts · 2018-06-03T09:41:53Z

I fixed them again. @Fishrock123
Thank you for reviewing.

test/parallel/test-timers-setunreftimeout.js

I think my review was addressed

lundibundi · 2018-08-25T18:11:55Z

ping @kakts.

kakts · 2018-08-30T15:13:26Z

@lundibundi Thank you for the review.
I'll check them.

lundibundi

LGTM.

test/parallel/test-timers-setunreftimeout.js

lundibundi · 2018-09-08T22:26:42Z

ping @apapirovski.

test/parallel/test-timers-setunreftimeout.js

kakts · 2018-09-18T14:50:09Z

@lundibundi Thank you for your review.
I fixed them.

old

apapirovski

This is still unnecessarily complicated and brittle (and didn't incorporate changes based on my earlier feedback). I've left some comments on what needs to be changed.

apapirovski · 2018-10-15T04:02:32Z

test/parallel/test-timers-setunreftimeout.js

+      for (const arg of args) {
+        results.push(arg);
+      }
+    }), 1, ...inputArgs);


clearTimeout(testTimer); here (after line 39, inside the function body).

Thank you! But if I fix this as you requested, it occures an error.
I fixed as below
ce868dd

apapirovski · 2018-10-15T04:03:02Z

test/parallel/test-timers-setunreftimeout.js

+    }), 1, ...inputArgs);
+
+    const testTimer = setTimeout(common.mustCall(() => {
+      for (let k = 0; k < maxArgsNum; k++) {


Instead of populating results, just do the check in the first timeout. Then this timeout shouldn't have a body. Literally just const testTimer = setTimeout(common.mustCall(), 1);

Thank you!
I've fixed it.

apapirovski · 2018-10-15T04:04:06Z

test/parallel/test-timers-setunreftimeout.js

+                  `actual ${args.length}`
+      );
+      for (const arg of args) {
+        results.push(arg);


Just check the results here instead of populating an array.

Thank you. I'll fix not populating an array.

Trott · 2018-11-18T05:01:35Z

@kakts Are you able to address the remaining review comments?

@apapirovski If @kakts is not available to finish this, are your requests sufficiently narrow that you can add them via a fixup commit or something?

kakts · 2018-11-18T10:35:52Z

@Trott @apapirovski
Very sorry for late my fixing.
I've fixed ce868dd

kakts · 2018-11-18T12:03:19Z

commit title lint emmitted an error.
https://github.com/nodejs/node/blob/master/.travis.yml#L7-L14

So I will made a new PR.

test: add test cases for setUnrefTimeout.
+ npx -q core-validate-commit --no-validate-metadata https://api.github.com/repos/nodejs/node/commits/50d63c94774b6cacae78d1da03d6ca7219ff3fe9
  ✖  50d63c94774b6cacae78d1da03d6ca7219ff3fe9
     ✔  0:0      skipping fixes-url                        fixes-url
     ✔  0:0      blank line after title                    line-after-title
     ✔  0:0      line-lengths are valid                    line-length
     ✔  0:0      valid subsystems                          subsystem
     ✖  0:41     Do not use punctuation at end of title.   title-format
     ✔  0:0      Title is <= 50 columns.                   title-length

refack · 2018-11-18T12:06:14Z

@kakts no need. It's easily fixable while landing the commit.

Trott · 2018-12-04T04:22:24Z

/ping @apapirovski Have your comments been adequately addressed? And if so, can you remove your request for changes?

Trott · 2018-12-04T04:22:53Z

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

Trott · 2018-12-05T02:40:41Z

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/19216/ ✔️

shisama · 2019-03-09T16:27:16Z

@apapirovski Could you review if your comments have been addressed?

kakts · 2019-06-13T09:43:34Z

Hi! How about this PR?
I'd fixed for some review comments already.

Could you check them again?

Fishrock123

This looks fine to me. I've started a new CI run.

nodejs-github-bot · 2019-10-08T21:58:58Z

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

Fishrock123 · 2019-10-08T22:05:40Z

test/parallel/test-timers-setunreftimeout.js

+    }
+
+    const timer = setUnrefTimeout(common.mustCall((...args) => {
+      // check the number of arguments passed to this callback.


Linter wants this to be capitalized (make -j4 lint)

Suggested change

// check the number of arguments passed to this callback.

// Check the number of arguments passed to this callback.

Fishrock123 · 2019-10-08T22:05:57Z

test/parallel/test-timers-setunreftimeout.js

+  const maxArgsNum = 4;
+  for (let i = 0; i < maxArgsNum; i++) {
+    const inputArgs = [];
+    // set the input argument params


Linter wants this to be capitalized (make -j4 lint)

Suggested change

// set the input argument params

// Set the input argument params

Fishrock123

Hmm. The test fails. Here is the CI output:

AssertionError [ERR_ASSERTION]: Expected "actual" to be reference-equal to "expected":
+ actual - expected

  Comparison {
    code: 'ERR_INVALID_CALLBACK',
+   message: 'Callback must be a function. Received undefined',
-   message: 'Callback must be a function',
    type: [Function: TypeError]
  }
    at new AssertionError (internal/assert/assertion_error.js:418:11)
    at Object.innerFn (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/common/index.js:617:15)
    at expectedException (assert.js:645:26)
    at expectsError (assert.js:770:3)
    at Function.throws (assert.js:819:3)
    at Object.expectsError (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/common/index.js:629:12)
    at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-timers-setunreftimeout.js:10:10)
    at Module._compile (internal/modules/cjs/loader.js:956:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:973:10)
    at Module.load (internal/modules/cjs/loader.js:811:32) {
  generatedMessage: false,
  code: 'ERR_ASSERTION',
  actual: [NodeError],
  expected: [Object],
  operator: 'common.expectsError'
}

kakts · 2019-10-11T07:07:10Z

@Fishrock123 Thank you for reviewing! I'll fix them again.

nodejs-github-bot · 2019-12-18T12:28:22Z

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

gireeshpunathil · 2019-12-18T12:30:25Z

@apapirovski @Fishrock123 PTAL

jasnell · 2020-06-25T01:25:25Z

This has not been updated in a while and has not made progress. Closing but can reopen if someone wants to pick it up again.

test: add test cases for setUnrefTimeout.

50d63c9

nodejs-github-bot added the test Issues and PRs related to the tests. label May 24, 2018

BridgeAR requested review from Trott and apapirovski May 28, 2018 13:31

Trott approved these changes May 28, 2018

View reviewed changes

trivikr approved these changes May 28, 2018

View reviewed changes

apapirovski previously requested changes May 29, 2018

View reviewed changes

test/parallel/test-timers-refresh.js Outdated Show resolved Hide resolved

test/parallel/test-timers-refresh.js Outdated Show resolved Hide resolved

test: refactored some test cases for setUnrefTimeout.

0822c0f

test: deleted notStrictEqual.

0d639ea

apapirovski reviewed Jun 1, 2018

View reviewed changes

hiroppy added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Jun 1, 2018

test: refactored some test cases for setUnrefTimeout.

6436008

Fishrock123 previously requested changes Jun 2, 2018

View reviewed changes

test/parallel/test-timers-setunreftimeout.js Outdated Show resolved Hide resolved

test: refactor test-timers-setunreftimeout.js

64e15a7

lundibundi reviewed Jul 28, 2018

View reviewed changes

kakts added 2 commits September 4, 2018 22:04

fix: refactor test/parallel/test-timers-setunreftimeout.js .

a5570d9

fix: lint error in test/parallel/test-timers-setunreftimeout.js

e756201

lundibundi approved these changes Sep 8, 2018

View reviewed changes

test/parallel/test-timers-setunreftimeout.js Outdated Show resolved Hide resolved

test/parallel/test-timers-setunreftimeout.js Outdated Show resolved Hide resolved

jasnell reviewed Sep 10, 2018

View reviewed changes

test/parallel/test-timers-setunreftimeout.js Outdated Show resolved Hide resolved

test: refactor test/parallel/test-timers-setunreftimeout.js

90daaa9

jasnell approved these changes Sep 19, 2018

View reviewed changes

apapirovski suggested changes Oct 15, 2018

View reviewed changes

test: refactor test case for timers.setUnrefTimeout

ce868dd

joyeecheung mentioned this pull request Dec 5, 2018

CI failures: 20181201-20181205 nodejs/reliability#19

Closed

shisama approved these changes Mar 9, 2019

View reviewed changes

Trott force-pushed the master branch from 1ecc406 to 49cf67e Compare September 17, 2019 16:52

Fishrock123 approved these changes Oct 8, 2019

View reviewed changes

Fishrock123 reviewed Oct 8, 2019

View reviewed changes

Fishrock123 suggested changes Oct 8, 2019

View reviewed changes

kakts added 2 commits October 22, 2019 22:11

test: capitalize the words in setunreftimeout.

a92b4d8

test: fix ERR_INVALID_CALLBACK message in setunreftimeout.

27ebac4

BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:20

jasnell added the stalled Issues and PRs that are stalled. label Jun 19, 2020

jasnell closed this Jun 25, 2020

	// check the number of arguments passed to this callback.
	// Check the number of arguments passed to this callback.

	// set the input argument params
	// Set the input argument params

Uh oh!

Conversation

kakts commented May 24, 2018

Summary

Checklist

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

apapirovski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kakts commented May 29, 2018

Uh oh!

kakts commented May 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kakts commented Jun 1, 2018

Uh oh!

Fishrock123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kakts commented Jun 3, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lundibundi commented Aug 25, 2018

Uh oh!

kakts commented Aug 30, 2018

Uh oh!

lundibundi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lundibundi commented Sep 8, 2018

Uh oh!

Uh oh!

kakts commented Sep 18, 2018

Uh oh!

apapirovski left a comment

Choose a reason for hiding this comment

Uh oh!

apapirovski Oct 15, 2018 • 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!

kakts Nov 18, 2018

Choose a reason for hiding this comment

Uh oh!

apapirovski Oct 15, 2018

Choose a reason for hiding this comment

Uh oh!

kakts Nov 18, 2018

Choose a reason for hiding this comment

Uh oh!

apapirovski Oct 15, 2018

Choose a reason for hiding this comment

Uh oh!

kakts Nov 18, 2018

Choose a reason for hiding this comment

Uh oh!

Trott commented Nov 18, 2018

Uh oh!

kakts commented Nov 18, 2018

Uh oh!

kakts commented May 31, 2018 •

edited

Loading

apapirovski Oct 15, 2018 •

edited

Loading

Trott commented Dec 5, 2018 •

edited

Loading

kakts commented Jun 13, 2019 •

edited

Loading