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

Jump to bottom

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 1 commit
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
5 changes: 3 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 @@ -93,9 +93,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
&& PlatformDetection.Is64BitProcess; // 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.

@JeremyKuhne do you perhaps know about any long path issues specific to x86?

Copy link
Member

Choose a reason for hiding this comment

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

can you explain what do you mean by flaky? what's happening? I'd expect this to be deterministic

Copy link
Member

@krwq krwq 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.

should this be !(Is32Bit && !IsArm) - perhaps we should add IsX86 in the PlatformDetection - we should also open the issue to try to understand why this is failing on x86 I think (unless we don't care for some reason).

Alternative: RuntimeInformation.ProcessArchitecture != Architecture.X86 - you should still consider adding that to platform detection

Copy link
Member Author

Choose a reason for hiding this comment

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

My guess is that it's rather related to assembly loading on x86 or process modules information not being refreshed rather than long paths itself.

I'd expect this to be deterministic

It's not ;( All tests have passed in #57335 where I've re-enabled this test but it fails from time to time: #57452.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should add more diagnostics.. I.e. print all module list, enumerate modules in the assembly returned by Assembly.LoadFile. This sound rather sketchy as is - perhaps it's unrelated issue completely but we should at least understand why it's happening before we disable I think

Copy link
Member Author

Choose a reason for hiding this comment

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

RuntimeInformation.ProcessArchitecture != Architecture.X86

done

Copy link
Member

Choose a reason for hiding this comment

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

@JeremyKuhne do you perhaps know about any long path issues specific to x86?

The only issues I've ever seen are bugs with the WOW layer when running an x86 binary on x64. I'm unaware of any outstanding issues though.


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