Skip to content

WASM: Fix System.IO.MemoryMappedFiles tests #39355

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 15, 2020
Merged

WASM: Fix System.IO.MemoryMappedFiles tests #39355

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/libraries/Common/tests/TestUtilities/System/PlatformDetection.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,9 @@ public static partial class PlatformDetection
public static bool IsNotFedoraOrRedHatFamily => !IsFedora && !IsRedHatFamily;
public static bool IsNotDebian10 => !IsDebian10;

public static bool IsSuperUser => !IsWindows ?
public static bool IsSuperUser => IsBrowser ? false : (!IsWindows ?
libc.geteuid() == 0 :
throw new PlatformNotSupportedException();
throw new PlatformNotSupportedException());

public static Version OpenSslVersion => !IsOSXLike && !IsWindows ?
GetOpenSslVersion() :
Expand Down
6 changes: 4 additions & 2 deletions src/libraries/System.IO.MemoryMappedFiles/tests/MemoryMappedFile.CreateFromFile.Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -614,6 +614,7 @@ public void FileInUse_CreateFromFile_FailsWithExistingReadWriteMap()
/// Test exceptional behavior when trying to create a map for a non-shared file that's currently in use.
/// </summary>
[Fact]
[PlatformSpecific(~TestPlatforms.Browser)] // the emscripten implementation ignores FileShare.None
public void FileInUse_CreateFromFile_FailsWithExistingNoShareFile()
{
// Already opened with a FileStream
Expand Down Expand Up @@ -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));
Copy link
Member

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?

Copy link
Member

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.

Copy link
Member Author

@akoeplinger akoeplinger Jul 15, 2020 •
edited
Loading

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 😄

}

[Fact]
public void WriteToReadOnlyFile_CopyOnWrite()
{
WriteToReadOnlyFile(MemoryMappedFileAccess.CopyOnWrite, (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows) && geteuid() == 0));
WriteToReadOnlyFile(MemoryMappedFileAccess.CopyOnWrite, (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows) && PlatformDetection.IsSuperUser));
}

/// <summary>
Expand Down Expand Up @@ -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
Copy link
Contributor

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?

Copy link
Member Author

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)

Copy link
Contributor

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

Copy link
Member Author

Choose a reason for hiding this comment

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

#39428

public void MultipleMapsForTheSameFileStream()
{
const int Capacity = 4096;
Expand Down
6 changes: 3 additions & 3 deletions src/libraries/System.IO.MemoryMappedFiles/tests/MemoryMappedFilesTestsBase.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ public abstract partial class MemoryMappedFilesTestBase : FileCleanupTestBase
/// <summary>Gets the system's page size.</summary>
protected static Lazy<int> s_pageSize = new Lazy<int>(() =>
{
if (RuntimeInformation.IsOSPlatform(OSPlatform.Browser))
return Environment.SystemPageSize;

int pageSize;
const int _SC_PAGESIZE_FreeBSD = 47;
const int _SC_PAGESIZE_Linux = 30;
Expand All @@ -31,9 +34,6 @@ public abstract partial class MemoryMappedFilesTestBase : FileCleanupTestBase
[DllImport("libc", SetLastError = true)]
private static extern int sysconf(int name);

[DllImport("libc", SetLastError = true)]
protected static extern int geteuid();

/// <summary>Asserts that the handle's inheritability matches the specified value.</summary>
protected static void AssertInheritability(SafeHandle handle, HandleInheritability inheritability)
{
Expand Down
1 change: 1 addition & 0 deletions src/libraries/System.IO.MemoryMappedFiles/tests/MemoryMappedViewAccessor.Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,7 @@ public void FlushSupportedOnBothReadAndWriteAccessors(MemoryMappedFileAccess acc
/// Test to validate that multiple accessors over the same map share data appropriately.
/// </summary>
[Fact]
[PlatformSpecific(~TestPlatforms.Browser)] // the emscripten implementation doesn't share data
public void ViewsShareData()
{
const int MapLength = 256;
Expand Down
1 change: 1 addition & 0 deletions src/libraries/System.IO.MemoryMappedFiles/tests/MemoryMappedViewStream.Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,7 @@ public void FlushSupportedOnBothReadAndWriteAccessors(MemoryMappedFileAccess acc
/// Test to validate that multiple accessors over the same map share data appropriately.
/// </summary>
[Fact]
[PlatformSpecific(~TestPlatforms.Browser)] // the emscripten implementation doesn't share data
public void ViewsShareData()
{
const int MapLength = 256;
Expand Down
1 change: 0 additions & 1 deletion src/libraries/tests.proj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
<ProjectExclusions Include="$(MSBuildThisFileDirectory)System.Globalization.Extensions\tests\System.Globalization.Extensions.Tests.csproj" />
<ProjectExclusions Include="$(MSBuildThisFileDirectory)System.Globalization\tests\System.Globalization.Tests.csproj" />
<ProjectExclusions Include="$(MSBuildThisFileDirectory)System.IO.FileSystem\tests\System.IO.FileSystem.Tests.csproj" />
<ProjectExclusions Include="$(MSBuildThisFileDirectory)System.IO.MemoryMappedFiles\tests\System.IO.MemoryMappedFiles.Tests.csproj" />
<ProjectExclusions Include="$(MSBuildThisFileDirectory)System.IO.Packaging\tests\System.IO.Packaging.Tests.csproj" />
<ProjectExclusions Include="$(MSBuildThisFileDirectory)System.Linq.Expressions\tests\System.Linq.Expressions.Tests.csproj" />
<ProjectExclusions Include="$(MSBuildThisFileDirectory)System.Linq.Parallel\tests\System.Linq.Parallel.Tests.csproj" />
Expand Down