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

Modify ErrorHandlingTests for browser #40659

Merged
merged 1 commit into from
Aug 28, 2020

Conversation

mdh1418
Copy link
Member

@mdh1418 mdh1418 commented Aug 11, 2020

Fixes #40531

Emscripten, using musl libc, sets NAME_MAX to 255
https://github.com/emscripten-core/emscripten/blob/master/system/include/libc/limits.h
https://git.musl-libc.org/cgit/musl/tree/include/limits.h#n47

Running tests before changes
Tests run: 3903, Errors: 0, Failures: 0, Skipped: 117. Time: 10.217902s
Running tests after changes
Tests run: 3905, Errors: 0, Failures: 0, Skipped: 117. Time: 10.4601469s

@mdh1418 mdh1418 added the arch-wasm WebAssembly architecture label Aug 11, 2020
@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@mdh1418 mdh1418 force-pushed the mdhwang/browser_error_handling_tests branch from 0bb9bea to 3d2d6b0 Compare August 11, 2020 14:19
@mdh1418 mdh1418 force-pushed the mdhwang/browser_error_handling_tests branch 2 times, most recently from 45e4b25 to a8cc3f7 Compare August 11, 2020 14:24
@mdh1418 mdh1418 force-pushed the mdhwang/browser_error_handling_tests branch from a8cc3f7 to ad2f5a9 Compare August 11, 2020 14:25
{
DirectoryInfo testDirectory = Directory.CreateDirectory(GetTestFilePath());
var names = new List<string>();
var lengthCap = PlatformDetection.IsBrowser ? 256 : 10_000; // On Browser NAME_MAX is 255, otherwise arbitrarily large limit for the test
Copy link
Member

Choose a reason for hiding this comment

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

So, in memfs, there is no enforcement of NAME_MAX, which is 255. As a result, you'll get 9999 files, 255 of which have increasing "a" names and the rest get their names truncated.

I'm not sure if this test has much value.

Copy link
Member Author

Choose a reason for hiding this comment

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

As in the test should be removed all together? If its any worth testing enumerability, changing the length upper limit still allows for that.

@mdh1418
Copy link
Member Author

mdh1418 commented Aug 28, 2020

These library tests are failing because emscripten does not throw an exception when truncating filenames longer than 255 characters. When emscripten begins to properly throw an exception, this change can be removed.

@mdh1418 mdh1418 merged commit 48b299c into dotnet:master Aug 28, 2020
@mdh1418 mdh1418 deleted the mdhwang/browser_error_handling_tests branch August 28, 2020 21:29
@mdh1418
Copy link
Member Author

mdh1418 commented Aug 28, 2020

@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-System.IO
Projects
None yet
Development

Successfully merging this pull request may close these issues.

System.IO.Filesystem.ErrorHandlingTests tests failing on Browser wasm
5 participants