-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
WASM: Fix System.IO.MemoryMappedFiles tests #39355
WASM: Fix System.IO.MemoryMappedFiles tests #39355
Conversation
@@ -696,13 +697,13 @@ private void WriteToReadOnlyFile(MemoryMappedFileAccess access, bool succeeds) | |||
public void WriteToReadOnlyFile_ReadWrite(MemoryMappedFileAccess access) | |||
{ | |||
WriteToReadOnlyFile(access, access == MemoryMappedFileAccess.Read || | |||
(!RuntimeInformation.IsOSPlatform(OSPlatform.Windows) && geteuid() == 0)); | |||
(!RuntimeInformation.IsOSPlatform(OSPlatform.Windows) && PlatformDetection.IsSuperUser)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it fine to remove !RuntimeInformation.IsOSPlatform(OSPlatform.Windows) &&
since it feels redundant to check for windows twice when IsBrowser
is false? Would hitting PNSE on windows be a problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the way that IsSuperUser
is written, if we remove the Windows
check here, it would throw PlatformNotSupportedException
and crash the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no we need to keep this since PlatformDetection.IsSuperUser
would throw PlatformNotSupportedException
if we ran it on Windows
edit GitHub didn't load @safern's comment at first so sorry for the double post 😄
@@ -768,6 +769,7 @@ public void LeaveOpenRespected_OutstandingViews(bool leaveOpen) | |||
/// Test to validate we can create multiple maps from the same FileStream. | |||
/// </summary> | |||
[Fact] | |||
[PlatformSpecific(~TestPlatforms.Browser)] // the emscripten implementation doesn't share data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does it mean? Is the behaviour unexpected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marek-safar kind of, though sharing an mmap'd file is not the common case. we could probably looking into detecting the situation and throwing a proper exception (right now the second mmap just doesn't see the modifications from the first mmap)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please create a tracking issue instead of ignoring the tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allows the tests to run on WebAssembly:
Tests run: 329, Errors: 0, Failures: 0, Skipped: 5. Time: 1.0009759s