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.

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

swap arguments in strictEqual() and comment the third argument (test-timers) #21660

Closed
wants to merge 2 commits into from

Conversation

sohailrajdev97
Copy link
Contributor

This addresses a good first issue assigned to me by @Trott . The arguments to all the assert.strictEqual() calls have been swapped and the third argument has been removed and put up as a comment.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jul 4, 2018
@@ -31,7 +31,7 @@ let interval_count = 0;
clearTimeout(null);
clearInterval(null);

assert.strictEqual(true, setTimeout instanceof Function);
assert.strictEqual(setTimeout instanceof Function, true);
Copy link
Member

@ChALkeR ChALkeR Jul 4, 2018

Choose a reason for hiding this comment

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

Why not assert.ok? And in some other places.
/cc @Trott — do we have a reason for a strict check here?

Copy link
Member

Choose a reason for hiding this comment

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

assert.ok() would be fine here of course, but I'm not terribly bothered by the strict check either.

There are 5 other files in our test suite that use strictEqual() for an instanceof check. If there is consensus that changing it to assert.ok() would be better, I'd rather change them all at once in a separate PR and add a lint rule.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that should not block this PR, just asking. 😃

assert.strictEqual(interval_count, 3);
assert.strictEqual(count4, 11);

// clearTimeout cleared too many timeouts
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Now that I'm looking at it, perhaps the comment might better as:

// Check that the correct number of timers ran.

@Trott
Copy link
Member

Trott commented Jul 5, 2018

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 5, 2018
@Trott
Copy link
Member

Trott commented Jul 5, 2018

LinuxONE CI failure is almost certainly unrelated. Resume Build isn't working right now on CI, but here's a Rebuild of just that job: https://ci.nodejs.org/job/node-test-commit-linuxone/2762/

@trivikr
Copy link
Member

trivikr commented Jul 10, 2018

Landed in b70db33

Congratulations @sohailrajdev97 on your first commit in Node.js core! 🎉🎉🎉

@trivikr trivikr closed this Jul 10, 2018
trivikr pushed a commit that referenced this pull request Jul 10, 2018
This commit also comments the third argument

PR-URL: #21660
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
targos pushed a commit that referenced this pull request Jul 10, 2018
This commit also comments the third argument

PR-URL: #21660
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@targos targos mentioned this pull request Jul 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants