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.

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

don't run LongModuleFileNamesAreSupported test on x86 #57471

Merged
merged 5 commits into from
Aug 17, 2021
Merged

don't run LongModuleFileNamesAreSupported test on x86 #57471

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
80392a9
don't run LongModuleFileNamesAreSupported test on x86 (for some reaso…
adamsitnik Aug 16, 2021
97c2857
apply code review suggestion
adamsitnik Aug 16, 2021
ee99091
ensure type called "Interop" has no namespace, so Interop files from …
adamsitnik Aug 16, 2021
14c02bc
try JanK suggestion and use LoadLibrary instead of Assembly.LoadFile
adamsitnik Aug 16, 2021
487748d
don't try to check module file path if we can't load the module itsel…
adamsitnik Aug 16, 2021
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
6 changes: 4 additions & 2 deletions src/libraries/System.Diagnostics.Process/tests/ProcessModuleTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.IO;
using System.Linq;
using System.Reflection;
using System.Runtime.InteropServices;
using Microsoft.DotNet.RemoteExecutor;
using Xunit;

Expand Down Expand Up @@ -93,9 +94,10 @@ public void ModulesAreDisposedWhenProcessIsDisposed()
}

public static bool Is_LongModuleFileNamesAreSupported_TestEnabled
=> PathFeatures.AreAllLongPathsAvailable() // we want to test long paths
=> OperatingSystem.IsWindows() // it's specific to Windows
&& PathFeatures.AreAllLongPathsAvailable() // we want to test long paths
&& !PlatformDetection.IsMonoRuntime // Assembly.LoadFile used the way this test is implemented fails on Mono
&& OperatingSystem.IsWindowsVersionAtLeast(8); // it's specific to Windows and does not work on Windows 7
&& RuntimeInformation.ProcessArchitecture != Architecture.X86; // for some reason it's flaky on x86
Copy link
Member

Choose a reason for hiding this comment

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

We're confident this is an OS limitation rather than a product or test bug?

Copy link
Member

@jkotas jkotas Aug 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

My guess is that we end up taking a fallback path in the assembly loader on x86 for some reason. The assembly loader has number of fallback paths for historic reasons to compensate for OS and compiler issues.

The assembly loader behavior that this test depends on is not guaranteed. The test is disabled on Mono for this reason. If once we enable these tests in single-file or NativeAOT modes, the test may need to be disabled for those configurations as well.

It would be better if the test came with its own .dll and called LoadLibrary on it. Then it can depend on that the library has to show up in the list.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jkotas I've followed your suggestion and used LoadLibrary directly. On one of the x86 CI legs it fails with ERROR_MOD_NOT_FOUND:

    System.Diagnostics.Tests.ProcessModuleTests.LongModuleFileNamesAreSupported [FAIL]
      LoadLibrary failed with 126
      Expected: False
      Actual:   True
      Stack Trace:
        /_/src/libraries/System.Diagnostics.Process/tests/ProcessModuleTests.Windows.cs(37,0): at System.Diagnostics.Tests.ProcessModuleTests.LongModuleFileNamesAreSupported()

I can't repro it locally and find out which module is missing (this is just a lib with a single class). Since this test is basically best effort to test long paths support for process module file paths, would it be OK to just skip the test on ERROR_MOD_NOT_FOUND on x86?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think it is ok to skip the test when LoadLibrary fails to load the module with the long file name. My guess is that the long paths are not properly enabled on the system when it happens.


[ConditionalFact(typeof(ProcessModuleTests), nameof(Is_LongModuleFileNamesAreSupported_TestEnabled))]
public void LongModuleFileNamesAreSupported()
Expand Down