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

Add tests for platform specific error codes for fs.rmdir on file #22095

Closed
wants to merge 6 commits into from

Conversation

gautamarora
Copy link
Contributor

@gautamarora gautamarora commented Aug 2, 2018

As per the discussion at #8797 and subsequent fix to documentation here, fs.rmdir has different error codes on windows vs posix systems.

This PR adds a test case to account for this difference in error codes.

This PR also updates the existing test to wrap each "case" with an anonymous scope.

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Aug 2, 2018
@gautamarora
Copy link
Contributor Author

i've not tested this on windows (dont have a win machine), so could use some help with that :)

@jasnell
Copy link
Member

jasnell commented Aug 3, 2018

CI (including Windows) : https://ci.nodejs.org/job/node-test-pull-request/16179/

@maclover7
Copy link
Contributor

@gautamarora Looks like some extra commits got pulled in here -- can you rebase on master and see if that fixes the issue?

@maclover7 maclover7 added the wip Issues and PRs that are still a work in progress. label Aug 11, 2018
@lundibundi
Copy link
Member

@gautamarora ping, could you rebase this on master, please?

assert.throws(function() {
fs.rmdirSync(f);
}, /ENOTDIR/);
}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this?:

assert.throws(() => { fs.rmdirSync(f); }
              common.isWindows ? /ENOENT/ : /ENOTDIR/);

@BridgeAR
Copy link
Member

Closing due to a long inactivity. @gautamarora thanks a lot for your contribution nevertheless. It is much appreciated. If you would like to continue working on this, please just open a new PR or leave a comment here.

@BridgeAR BridgeAR closed this Mar 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants