Skip to content
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

Jump to bottom

test: increase coverage of timers #11290

Closed
wants to merge 1 commit into from
Closed

test: increase coverage of timers #11290

wants to merge 1 commit into from

Conversation

DavidCai1111
Copy link
Member

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 10, 2017
@mscdex mscdex added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Feb 10, 2017
@mscdex
Copy link
Contributor

mscdex commented Feb 10, 2017

I think we should probably restrict the ES6 usage, otherwise we can't backport the changes easily (especially to v4.x -- which has spread and other ES6 features behind a flag yet).

Fishrock123
Fishrock123 previously requested changes Feb 10, 2017
View reviewed changes
test/parallel/test-timers-args.js Outdated
if (nargs < 128) interval(nargs + 1);
}
}

timeout(0);
interval(0);
function immediate(nargs) {
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this should be in a separate file that is about Immediates rather than Timeouts.

Sure, they are from the same module, but they share pretty much 0 implementation at the current time.

@DavidCai1111
Copy link
Member Author

DavidCai1111 commented Feb 11, 2017 •
edited
Loading

@Fishrock123 Thanks for the review, I moved that test to test-timers-immediate.js, PTAL.
@mscdex I agree and I changed spread back to apply. May be we should mention this in https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#esnext-features , now in the guide it says "when writing tests, it is encouraged to use ES.Next features that have already landed in the ECMAScript specification". If more people agree with it, I think I can also submit another PR to do this.

mscdex
mscdex reviewed Feb 11, 2017
View reviewed changes
test/parallel/test-timers-clearImmediate.js Outdated
const N = 3;
let count = 0;
function next() {
const immediate = setImmediate(function() {
Copy link
Contributor

@mscdex mscdex Feb 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Perhaps we can drop count entirely and the process 'exit' handler and just wrap the callback in a common.mustCall()?

mscdex
mscdex reviewed Feb 11, 2017
View reviewed changes
test/parallel/test-timers-clearImmediate.js Outdated
const N = 3;
let count = 0;
function next() {
const immediate = setImmediate(function() {
Copy link
Contributor

@mscdex mscdex Feb 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Now that we have a common.mustNotCall(), I think we can use that here and drop count and the process 'exit' handler.

mscdex
mscdex reviewed Feb 11, 2017
View reviewed changes
test/parallel/test-timers-immediate.js Outdated
assert.deepStrictEqual(immediateC, [1, 2, 3], 'immediateC args should match');
assert.deepStrictEqual(immediateD, [1, 2, 3, 4, 5], '5 args should match');
});
process.on('exit', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we can remove this event handler and just move the assertion to inside callback?

@DavidCai1111
Copy link
Member Author

@mscdex Thanks for the review, PTAL.

@mscdex
Copy link
Contributor

mscdex commented Feb 11, 2017

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

@mscdex
Copy link
Contributor

mscdex commented Feb 11, 2017

CI is green, LGTM.

@jasnell
Copy link
Member

jasnell commented Feb 17, 2017 •
edited
Loading

@Fishrock123 PTAL

@joyeecheung
Copy link
Member

@DavidCai1993

I agree and I changed spread back to apply. May be we should mention this in https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#esnext-features , now in the guide it says "when writing tests, it is encouraged to use ES.Next features that have already landed in the ECMAScript specification". If more people agree with it, I think I can also submit another PR to do this.

Oops, it was me who wrote this line...forgot about the backportability part(:facepalm) +1 on fixing this.

@DavidCai1111 DavidCai1111 mentioned this pull request Feb 18, 2017
Closed
3 tasks
joyeecheung pushed a commit that referenced this pull request Feb 21, 2017
@DavidCai1111 @joyeecheung
doc: restrict the ES.Next features usage in tests
88035bc 
PR-URL: #11452
Refs: #11290
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Feb 22, 2017
@DavidCai1111 @addaleax
doc: restrict the ES.Next features usage in tests
c5ff76d 
PR-URL: #11452
Refs: #11290
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
DavidCai1111 added a commit to DavidCai1111/node that referenced this pull request Mar 7, 2017
@DavidCai1111
doc: restrict the ES.Next features usage in tests
9cce019 
PR-URL: nodejs#11452
Refs: nodejs#11290
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@DavidCai1111 @MylesBorins
doc: restrict the ES.Next features usage in tests
c2e818d 
PR-URL: #11452
Refs: #11290
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@DavidCai1111 @MylesBorins
doc: restrict the ES.Next features usage in tests
227cc1e 
PR-URL: #11452
Refs: #11290
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR
Copy link
Member

This needs a rebase @DavidCai1993

@Fishrock123 I dismiss your request as that was addressed by @DavidCai1993. Please have another look as soon as he rebased.

@BridgeAR BridgeAR dismissed Fishrock123’s stale review August 26, 2017 10:13

The requested change was addressed

@BridgeAR BridgeAR added the stalled Issues and PRs that are stalled. label Sep 8, 2017
@BridgeAR
Copy link
Member

Closing due to a long inactivity. @DavidCai1993 please feel free to reopen if you want to follow up on this.

@BridgeAR BridgeAR closed this Sep 12, 2017
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this pull request May 5, 2024
@DavidCai1111
doc: restrict the ES.Next features usage in tests
67f563b 
PR-URL: nodejs/node#11452
Refs: nodejs/node#11290
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mscdex mscdex mscdex left review comments

@jasnell jasnell jasnell approved these changes

@Fishrock123 Fishrock123 Fishrock123 left review comments

Assignees
No one assigned
Labels
stalled Issues and PRs that are stalled. test Issues and PRs related to the tests. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@DavidCai1111 @mscdex @jasnell @joyeecheung @BridgeAR @Fishrock123 @nodejs-github-bot