-
Notifications
You must be signed in to change notification settings - Fork 29.1k
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.
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
test_runner: use validateStringArray for timers.enable()
#49534
test_runner: use validateStringArray for timers.enable()
#49534
Conversation
Review requested:
|
@@ -508,7 +508,7 @@ class MockTimers { | |||
); | |||
} | |||
|
|||
validateArray(timers, 'timers'); | |||
validateStringArray(timers, 'timers'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is semver-major, correct? If so, can you please take it as its own PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aduh95 the MockTimers
is currently experimental, I don't think we have to mark this as
semver-major
Lines 1560 to 1568 in c86e700
## Class: `MockTimers` | |
<!-- YAML | |
added: | |
- v20.4.0 | |
--> | |
> Stability: 1 - Experimental | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I'm not sure this PR is semver-major
or not. How about checking jenkins-ci's result according to below collaborator guide?
node/doc/contributing/collaborator-guide.md
Lines 901 to 908 in 7bf29b5
* `semver-{minor,major}` | |
* be conservative – that is, if a change has the remote _chance_ of breaking | |
something, go for semver-major | |
* when adding a semver label, add a comment explaining why you're adding it | |
* minor vs. patch: roughly: "does it add a new method / does it add a new | |
section to the docs" | |
* major vs. everything else: run last versions tests against this version, if | |
they pass, **probably** minor or patch |
cc @ErickWendel PTAL when you are available. |
This needs a rebase. |
ba15590
to
40e1c33
Compare
@deokjinkim please do not add
request-ci
|
@lpinca I rebased this PR. Could you review again? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test that's failing on main
and passing on this branch please?
Some timer values such as `setImmediate` and `clearImmediate` are missed. And `milliseconds` which is argument of `timers.tick()` is optional and default is 1. Refs: nodejs#49534 (comment)
Thank you for review. I added a test you mentioned :) |
Some timer values such as `setImmediate` and `clearImmediate` are missed. And `milliseconds` which is argument of `timers.tick()` is optional and default is 1. Refs: #49534 (comment) PR-URL: #52969 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Some timer values such as `setImmediate` and `clearImmediate` are missed. And `milliseconds` which is argument of `timers.tick()` is optional and default is 1. Refs: #49534 (comment) PR-URL: #52969 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Some timer values such as `setImmediate` and `clearImmediate` are missed. And `milliseconds` which is argument of `timers.tick()` is optional and default is 1. Refs: nodejs#49534 (comment) PR-URL: nodejs#52969 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Some timer values such as `setImmediate` and `clearImmediate` are missed. And `milliseconds` which is argument of `timers.tick()` is optional and default is 1. Refs: nodejs#49534 (comment) PR-URL: nodejs#52969 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Some timer values such as `setImmediate` and `clearImmediate` are missed. And `milliseconds` which is argument of `timers.tick()` is optional and default is 1. Refs: #49534 (comment) PR-URL: #52969 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Some timer values such as `setImmediate` and `clearImmediate` are missed. And `milliseconds` which is argument of `timers.tick()` is optional and default is 1. Refs: #49534 (comment) PR-URL: #52969 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Landed in 4f94397 |
`apis` which is argument of `timers.enable()` is string array. So use `validatStringArray` instead of `validateArray`. And `options` is optional, so update JSDoc. PR-URL: #49534 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Zijian Liu <lxxyxzj@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
`apis` which is argument of `timers.enable()` is string array. So use `validatStringArray` instead of `validateArray`. And `options` is optional, so update JSDoc. PR-URL: #49534 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Zijian Liu <lxxyxzj@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
apis
which is argument oftimers.enable()
is string array. So usevalidatStringArray
instead ofvalidateArray
. Andoptions
is optional, so update JSDoc.In document, some default value of
timers
are missed such assetImmediate
andclearImmediate
. Next,milliseconds
which is argument of `timers.tick() is optional and default is 1.