Skip to content

Conversation

lrlna
Copy link
Contributor

@lrlna lrlna commented Dec 1, 2016

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

tests

Description of change

This PR tests cancelling timers with null, or no arguments do not throw an error.

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 1, 2016
@julianduque julianduque added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Dec 1, 2016
@MylesBorins
Copy link
Contributor

Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

LGTM if CI is green

// This test makes sure clearing timers with
// 'null' or no input does not throw error

assert.doesNotThrow(() => { clearInterval(null);});
Copy link
Contributor

Choose a reason for hiding this comment

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

These arrow functions would be a bit cleaner without the braces, e.g. assert.doesNotThrow(() => clearInterval(null));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gotcha!

@Fishrock123
Copy link
Contributor

Seems good otherwise!


assert.doesNotThrow(() => clearTimeout());

assert.doesNotThrow(() => clearInterval(null));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason you're testing intervals twice? Should these be immediates?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're rightttt!

@addaleax addaleax added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Dec 2, 2016
Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

@Fishrock123
Copy link
Contributor

@lrlna If you could squash the commits into one that would be great. :D

Otherwise we'll just do it before landing.

@silverwind
Copy link
Contributor

Thanks! Landed in e21e129.

@silverwind silverwind closed this Dec 3, 2016
silverwind pushed a commit that referenced this pull request Dec 3, 2016
Add a test for cancelling timers with null or no arguments.

PR-URL: #10068
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
addaleax pushed a commit that referenced this pull request Dec 5, 2016
Add a test for cancelling timers with null or no arguments.

PR-URL: #10068
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
@Fishrock123 Fishrock123 mentioned this pull request Dec 5, 2016
2 tasks
addaleax pushed a commit to addaleax/node that referenced this pull request Dec 8, 2016
Add a test for cancelling timers with null or no arguments.

PR-URL: nodejs#10068
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
jmdarling pushed a commit to jmdarling/node that referenced this pull request Dec 8, 2016
Add a test for cancelling timers with null or no arguments.

PR-URL: nodejs#10068
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 20, 2016
Add a test for cancelling timers with null or no arguments.

PR-URL: #10068
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
Add a test for cancelling timers with null or no arguments.

PR-URL: #10068
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
Add a test for cancelling timers with null or no arguments.

PR-URL: #10068
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
This was referenced Dec 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. test Issues and PRs related to the tests. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants