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

fs: fix fsPromises.lchmod errors on non-Mac #21435

Merged

Conversation

shisama
Copy link
Contributor

@shisama shisama commented Jun 21, 2018

On non-macOS, fsPromises.lchmod and lchown throws AssertionError.
Expected behavior is Error [ERR_METHOD_NOT_IMPLEMENTED].
ERR_METHOD_NOT_IMPLEMENTED() requires argument, but it didn't set.
Fixes ERR_METHOD_NOT_IMPLEMENTED() to ERR_METHOD_NOT_IMPLEMENTED('lchmod')
or ERR_METHOD_NOT_IMPLEMENTED('lchown').

Fixes: #21421

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

@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Jun 21, 2018
@BridgeAR
Copy link
Member

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 21, 2018
@shisama
Copy link
Contributor Author

shisama commented Jun 22, 2018

I think CI failures are unrelated to this.

@nodejs nodejs deleted a comment Jun 22, 2018
@ChALkeR ChALkeR added experimental Issues and PRs related to experimental features. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Jun 24, 2018
@targos
Copy link
Member

targos commented Jun 24, 2018

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

@ChALkeR
Copy link
Member

ChALkeR commented Jun 24, 2018

This is (one of the issues that are) blocking #21470, so I would prefer this to land soon. 😃
Also issue #21421 was what prompted me to create #21470, thanks for that. 😉

@apapirovski
Copy link
Member

@shisama
Copy link
Contributor Author

shisama commented Jun 30, 2018

Thanks to #21498 , we can use lchown() on every platform. I will rebase this pull request and remove test for lchown().

@shisama shisama changed the title fs: fix fsPromises.lchmod and lchown errors on non-Mac fs: fix fsPromises.lchmod errors on non-Mac Jun 30, 2018
@shisama shisama force-pushed the fix-fs-promises-err-method-not-implemented branch from f66c492 to 5aef2d8 Compare June 30, 2018 12:32
@shisama
Copy link
Contributor Author

shisama commented Jun 30, 2018

@ChALkeR @jasnell @cjihrig @BridgeAR @starkwang @trivikr @targos @apapirovski Now, we can use lchown() on non-macOS platforms thanks to #21498. So, I removed test for lchown(). Please review again.

@cjihrig
Copy link
Contributor

cjihrig commented Jun 30, 2018

Still LGTM. Thanks for updating this.

On non-macOS, fsPromises.lchmod throws AssertionError.
Expected behavior is `Error [ERR_METHOD_NOT_IMPLEMENTED]`.
`ERR_METHOD_NOT_IMPLEMENTED()` requires argument, but it wasn't set.

Fixes `ERR_METHOD_NOT_IMPLEMENTED()` to
`ERR_METHOD_NOT_IMPLEMENTED('lchmod()')`.

PR-URL: nodejs#21435
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
@ChALkeR ChALkeR force-pushed the fix-fs-promises-err-method-not-implemented branch from 5aef2d8 to f54958e Compare July 10, 2018 17:02
@ChALkeR
Copy link
Member

ChALkeR commented Jul 10, 2018

Looks like no objections. Rebased, commit message updated to exclude mentions of lchown, preparing to land.

@ChALkeR
Copy link
Member

ChALkeR commented Jul 10, 2018

@ChALkeR
Copy link
Member

ChALkeR commented Jul 10, 2018

Landed in f54958e, thanks all! 🎉

@ChALkeR ChALkeR merged commit f54958e into nodejs:master Jul 10, 2018
targos pushed a commit that referenced this pull request Jul 12, 2018
On non-macOS, fsPromises.lchmod throws AssertionError.
Expected behavior is `Error [ERR_METHOD_NOT_IMPLEMENTED]`.
`ERR_METHOD_NOT_IMPLEMENTED()` requires argument, but it wasn't set.

Fixes `ERR_METHOD_NOT_IMPLEMENTED()` to
`ERR_METHOD_NOT_IMPLEMENTED('lchmod()')`.

PR-URL: #21435
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Yuta Hiroto <[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. errors Issues and PRs related to JavaScript errors originated in Node.js core. experimental Issues and PRs related to experimental features. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fs: fsPromises.lchmod throws AssertionError on Ubuntu