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

Fix error being overridden incorrectly on FS.Rename #11812

Merged
merged 6 commits into from
Aug 19, 2020
Merged

Fix error being overridden incorrectly on FS.Rename #11812

merged 6 commits into from
Aug 19, 2020

Conversation

kjpou1
Copy link
Contributor

@kjpou1 kjpou1 commented Aug 5, 2020

While renaming a file and if one of the parent paths do not exist the error code is being overwritten to EBUSY instead of reporting the actual error:

Error no 44 Error: ENOENT Message: No such file or directory 

Closes #11804

@welcome
Copy link

welcome bot commented Aug 5, 2020

Thank you for submitting a pull request! If this is your first PR, make sure to add yourself to AUTHORS.

src/library_fs.js Outdated Show resolved Hide resolved
mdh1418 pushed a commit to dotnet/runtime that referenced this pull request Aug 10, 2020
* [browser][file system] Tests System.IO.FileSystem

* Remove FileSystem from test project exclusions.

* [browser][filesystem] Comment on skipped tests due to IO.Pipes not supported

* [browser][filesystem] Comment on skipped tests due to monitor not supported

* Address review comments

* Address review comments on how to handle monitor PNSE

* Update src/libraries/System.IO.FileSystem/tests/File/GetSetTimes.cs

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

* Add ActiveIssue for browser platform

```
        [ActiveIssue("#39955", TestPlatforms.Browser)]
        public void NotFoundErrorIsExpected()
```

* Simplify test case with `PlatformDetection.IsSuperUser` as per review comment

* Remove unused variable from tests

* Browser platform volume does not limit each component of the path to a total of 255 characters.

- Remove the test `DirectoryWithComponentLongerThanMaxComponentAsPath_ThrowsException` for Browser platform.
- Add new test `DirectoryWithComponentLongerThanMaxComponentAsPath_BrowserDoesNotThrowsException` for Browser platform in case this check is added in the future.

* Fix windows tests

* Add ActiveIssue to test System.IO.Tests.DirectoryInfo_Name.CurrentDirectory

- System.IO.Tests.DirectoryInfo_Name.CurrentDirectory does not pass because it is using Path.GetFileName on the current directory of "/".
- See issue #39998 for more information

* Add active issue for tests that need to support file locking.

* Add active issue for tests that need to support file locking.

* Browser volume does not have a limit on segments

* Remove browser platform test

* Remove active issue for NotFoundErrorIsExpected

* Remove platform specific from SkippingHiddenFiles

* Fix enumeration test errors failing from `HiddenFilesAreReturned`.

- WebAssembly (BROWSER) has dirent d_type but is not identifying correctly but by returning UNKNOWN the managed code properly stats the file to detect if entry is directory or not.

* Fix enumeration test errors failing from `HiddenFilesAreReturned`.

- WebAssembly (BROWSER) has dirent d_type but is not identifying correctly but by returning UNKNOWN the managed code properly stats the file to detect if entry is directory or not.

* Fix build for TARGET_WASM not defined

* Address review comment by add check for IsWindows to IsSuperUser.

* Address review comment by add check for IsWindows to IsSuperUser.

* Platform Specific test for hidden files no longer needed.

* Platform Specific test fno longer needed.

* Use active issue for rename issue

* ActiveIssue no longer needed

- PR #40310 works around the issue for now while waiting for fix emscripten-core/emscripten#11804 / emscripten-core/emscripten#11812

* Use ActiveIssue for SettingUpdatesProperties

* Use ActiveIssue for TimesIncludeMillisecondPart

* Use ActiveIssue for ErrorHandlingTests failures

* Use ActiveIssue for SetUptoNanoseconds

* Use ActiveIssue for GetSetTimes test failures

* Use ActiveIssue for tests failing with DirectoryNotFoundException

* Use ActiveIssue for tests failing with UnauthorizedAccessException

* Remove debugging statement from pal_io

* Address type in CreateDirectory method name

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
Co-authored-by: Mitchell Hwang <mitchell.hwang@microsoft.com>
kjpou1 added 2 commits August 18, 2020 09:58
- The error will percolate up and not be overridden with 'EBUSY'
@kjpou1 kjpou1 requested review from kripken and sbc100 August 18, 2020 08:03
Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Thanks! lgtm, but can you add test to the existing test_rename.c?

- Return ENONT instead of EBUSY as was previously overridden to do.
@kjpou1
Copy link
Contributor Author

kjpou1 commented Aug 18, 2020

Test added

@kjpou1 kjpou1 requested a review from sbc100 August 18, 2020 14:19
@sbc100
Copy link
Collaborator

sbc100 commented Aug 18, 2020

Looks like you have a conflict.

While you are there can you add a comment to the test code saying why that test case is needed. e.g. "test that non-existant parent during rename generates the correct error code"?

…into rename-error-overridding

# Conflicts:
#	tests/stdio/test_rename.c
@kjpou1 kjpou1 requested a review from sbc100 August 19, 2020 04:41
@sbc100 sbc100 merged commit afbdbd2 into emscripten-core:master Aug 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error being overridden incorrectly on FS.Rename
3 participants