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: favor === over == in fs-utimes #9290

Closed
wants to merge 2 commits into from
Closed

test: favor === over == in fs-utimes #9290

wants to merge 2 commits into from

Conversation

jun-oka
Copy link
Contributor

@jun-oka jun-oka commented Oct 26, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test fs-utimes

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 26, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Oct 26, 2016

This is a good start, but there are other improvements that can be made while you're updating this test:

  • Replace var with const where appropriate.
  • Use common.mustCall() and common.fail() to enforce execution flow.
  • It might be possible to get rid of tests_run, tests_ok, and the process.on('exit', ...), but this would require more digging.

@mscdex mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Oct 26, 2016
@jun-oka
Copy link
Contributor Author

jun-oka commented Oct 26, 2016

@cjihrig Sorry, I overlooked var and const one before I commit. Thank you for common.mustCall() and common.fail() info! I will handle it today.

@Trott
Copy link
Member

Trott commented Oct 26, 2016

I'd also add that if you do end up keeping the assert.equal(), it would be great if it were possible to use assert.strictEqual() instead.

@jun-oka
Copy link
Contributor Author

jun-oka commented Oct 26, 2016

@Trott Understood. I found it on Line 143. Thank you.

@jun-oka jun-oka force-pushed the test-fs-utimes-eqeqeq branch from 9c93f6c to 8133444 Compare December 24, 2016 18:08
@jun-oka
Copy link
Contributor Author

jun-oka commented Dec 24, 2016

@cjihrig @Trott I have corrected those issue. Thanks a lot!

@Trott
Copy link
Member

Trott commented Dec 24, 2016

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

@Trott
Copy link
Member

Trott commented Jan 6, 2017

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

@Trott Trott force-pushed the test-fs-utimes-eqeqeq branch from 8133444 to 90b88b9 Compare January 7, 2017 00:02
@Trott
Copy link
Member

Trott commented Jan 7, 2017

Landed in 377ea28.
Thanks for the contribution! 🎉

@Trott Trott closed this Jan 7, 2017
Trott pushed a commit to Trott/io.js that referenced this pull request Jan 7, 2017
@jun-oka @Trott
test: refactor test-fs-utimes
377ea28 
* favor `===` over `==`
* `var` -> `const`/`let`
* use `common.mustCall()`

PR-URL: nodejs#9290
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
@jun-oka @italoacasas
test: refactor test-fs-utimes
cc1af2b 
* favor `===` over `==`
* `var` -> `const`/`let`
* use `common.mustCall()`

PR-URL: nodejs#9290
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 19, 2017
@jun-oka @italoacasas
test: refactor test-fs-utimes
4990feb 
* favor `===` over `==`
* `var` -> `const`/`let`
* use `common.mustCall()`

PR-URL: nodejs#9290
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 25, 2017
@jun-oka @italoacasas
test: refactor test-fs-utimes
146d231 
* favor `===` over `==`
* `var` -> `const`/`let`
* use `common.mustCall()`

PR-URL: nodejs#9290
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@italoacasas italoacasas mentioned this pull request Jan 25, 2017
Closed
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
@jun-oka @italoacasas
test: refactor test-fs-utimes
38bdfb0 
* favor `===` over `==`
* `var` -> `const`/`let`
* use `common.mustCall()`

PR-URL: nodejs#9290
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Mar 8, 2017
@jun-oka @MylesBorins
test: refactor test-fs-utimes
a0f1b23 
* favor `===` over `==`
* `var` -> `const`/`let`
* use `common.mustCall()`

PR-URL: #9290
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 8, 2017
@jun-oka @MylesBorins
test: refactor test-fs-utimes
50523dd 
* favor `===` over `==`
* `var` -> `const`/`let`
* use `common.mustCall()`

PR-URL: #9290
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@jun-oka @MylesBorins
test: refactor test-fs-utimes
12746af 
* favor `===` over `==`
* `var` -> `const`/`let`
* use `common.mustCall()`

PR-URL: #9290
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@jun-oka @MylesBorins
test: refactor test-fs-utimes
3f3c78d 
* favor `===` over `==`
* `var` -> `const`/`let`
* use `common.mustCall()`

PR-URL: #9290
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@jun-oka @cjihrig @Trott @jasnell @targos @mscdex @MylesBorins @nodejs-github-bot