Skip to content

test: check parameter type of fs.mkdir() #22616

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
wants to merge 4 commits into from

Conversation

shisama
Copy link
Contributor

@shisama shisama commented Aug 31, 2018

Added tests to check parameter type of fs.mkdir(), fs.mkdirSync()
and fsPromises.mkdir() to increase coverage.

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

Added tests to check parameter type of fs.mkdir(), fs.mkdirSync()
and fsPromises.mkdir() to increase coverage.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Aug 31, 2018
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

A couple nits. LGTM once the CI passes.

const pathname = path.join(tmpdir.path, nextdir());
['', 1, {}, [], null, Symbol('test'), () => {}].forEach((i) => {
common.expectsError(
() => fs.mkdir(pathname, { recursive: i }, common.mustNotCall()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you rename i to recursive, and this can just be { recursive }.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your review. Fixed this.

const dir = path.join(tmpDir, nextdir(), nextdir());
['', 1, {}, [], null, Symbol('test'), () => {}].forEach((i) => {
assert.rejects(
// mkdtemp() expects to get a string prefix.
Copy link
Contributor

Choose a reason for hiding this comment

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

mkdtemp()?

Copy link
Contributor Author

@shisama shisama Aug 31, 2018

Choose a reason for hiding this comment

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

oh, sorry. this is wrong copy and paste. Fixed this.

['', 1, {}, [], null, Symbol('test'), () => {}].forEach((recursive) => {
assert.rejects(
// mkdir() expects to get a boolean value for options.recursive.
async () => mkdir(dir, { recursive: i }),
Copy link
Member

Choose a reason for hiding this comment

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

Small oversight: it should be recursive without : i

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your review. I fixed this.

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

@gdams
Copy link
Member

gdams commented Sep 4, 2018

landed in: fdf829e

@gdams gdams closed this Sep 4, 2018
gdams pushed a commit that referenced this pull request Sep 4, 2018
Added tests to check parameter type of fs.mkdir(), fs.mkdirSync()
and fsPromises.mkdir() to increase coverage.

PR-URL: #22616
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
targos pushed a commit that referenced this pull request Sep 12, 2018
Added tests to check parameter type of fs.mkdir(), fs.mkdirSync()
and fsPromises.mkdir() to increase coverage.

PR-URL: #22616
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
targos pushed a commit that referenced this pull request Sep 19, 2018
Added tests to check parameter type of fs.mkdir(), fs.mkdirSync()
and fsPromises.mkdir() to increase coverage.

PR-URL: #22616
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
@targos
Copy link
Member

targos commented Sep 20, 2018

Depends on #21875. Marking dont-land-on-v10.x for now.

targos pushed a commit that referenced this pull request Sep 25, 2018
Added tests to check parameter type of fs.mkdir(), fs.mkdirSync()
and fsPromises.mkdir() to increase coverage.

PR-URL: #22616
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
# for free to join this conversation on GitHub. Already have an account? # 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