don't run LongModuleFileNamesAreSupported test on x86 #57471

adamsitnik · 2021-08-16T06:19:17Z

This particular tests uses Assembly.LoadFile(path) to load a test assembly:

runtime/src/libraries/System.Diagnostics.Process/tests/ProcessModuleTests.cs

Lines 101 to 107 in 80392a9

    
           [ConditionalFact(typeof(ProcessModuleTests), nameof(Is_LongModuleFileNamesAreSupported_TestEnabled))] 
        
           public void LongModuleFileNamesAreSupported() 
        
           { 
        
               // To be able to test Long Path support for ProcessModule.FileName we need a .dll that has a path > 260 chars. 
        
               // Since Long Paths support can be disabled (see the ConditionalFact attribute usage above), 
        
               // we just copy "LongName.dll" from bin to a temp directory with a long name and load it from there. 
        
               // Loading from new path is possible because the type exposed by the assembly is not referenced in any explicit way.

for some reason it's flaky on x86. I've tried to repro it on two Windows machines and failed. Since I don't have better idea, I think it's just OK not to run this test on x86.

The previous check of Windows version (OperatingSystem.IsWindowsVersionAtLeast(8)) can be removed as in the past this test was failing only on Windows 7 x86.

fixes #57452

…n it's flaky)

ghost · 2021-08-16T06:19:21Z

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

Issue Details

for some reason it's flaky

fixes #57452

Author:	adamsitnik
Assignees:	-
Labels:	`area-System.Diagnostics.Process`
Milestone:	6.0.0

krwq · 2021-08-16T09:33:06Z

src/libraries/System.Diagnostics.Process/tests/ProcessModuleTests.cs

            && !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


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

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

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

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.

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

RuntimeInformation.ProcessArchitecture != Architecture.X86

done

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

stephentoub · 2021-08-16T10:38:36Z

src/libraries/System.Diagnostics.Process/tests/ProcessModuleTests.cs

            && !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


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

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.

@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?

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.

…$(CommonPath)Interop\ can be included in the test project

…f (x86)

lewing · 2021-08-16T23:40:22Z

/azp run runtime

azure-pipelines · 2021-08-16T23:40:54Z

Azure Pipelines successfully started running 1 pipeline(s).

lewing · 2021-08-17T04:12:49Z

If this is the solution to #57452 is definitely 6.0 milestone

adamsitnik · 2021-08-17T07:11:55Z

src/libraries/System.Diagnostics.Process/tests/Interop.cs

 {
-    internal static partial class Interop


this Interop type was causing a conflict with the other Interop that we use in product code and because of that I was not able to call Interop.Kernel32.LoadLibrary in the test code. I decided to just remove the namespace so it de-facto becames a single Interop type.

adamsitnik · 2021-08-17T07:13:34Z

the CI failures are not related, as soon as I get an approval the PR is ready to be merged

jozkee

LGTM

don't run LongModuleFileNamesAreSupported test on x86 (for some reaso…

80392a9

…n it's flaky)

adamsitnik added the area-System.Diagnostics.Process label Aug 16, 2021

adamsitnik added this to the 6.0.0 milestone Aug 16, 2021

krwq reviewed Aug 16, 2021

View reviewed changes

apply code review suggestion

97c2857

stephentoub reviewed Aug 16, 2021

View reviewed changes

adamsitnik modified the milestones: 6.0.0, 7.0.0 Aug 16, 2021

adamsitnik added 3 commits August 16, 2021 16:53

ensure type called "Interop" has no namespace, so Interop files from …

ee99091

…$(CommonPath)Interop\ can be included in the test project

try JanK suggestion and use LoadLibrary instead of Assembly.LoadFile

14c02bc

don't try to check module file path if we can't load the module itsel…

487748d

…f (x86)

lewing mentioned this pull request Aug 17, 2021

[main] Update dependencies from dotnet/runtime dotnet/emsdk #57491

Merged

lewing added the blocking-clean-ci Blocking PR or rolling runs of 'runtime' or 'runtime-extra-platforms' label Aug 17, 2021

jaredpar mentioned this pull request Aug 17, 2021

Bundle_Is_Extracted test failing intermittently on Linux #43316

Closed

adamsitnik commented Aug 17, 2021

View reviewed changes

adamsitnik requested review from krwq, jkotas, stephentoub and jozkee August 17, 2021 07:12

adamsitnik modified the milestones: 7.0.0, 6.0.0 Aug 17, 2021

jozkee approved these changes Aug 17, 2021

View reviewed changes

adamsitnik merged commit 14b34eb into dotnet:main Aug 17, 2021

adamsitnik deleted the issue57452 branch August 17, 2021 07:49

jaredpar mentioned this pull request Aug 17, 2021

RunContinueWithStressTestsNoState timing out in CI #2271

Closed

This was referenced Aug 17, 2021

Forbid fgMorphCommutative during optValnumCSE_phase #57160

Merged

Fix ARM64 unsigned div by const perf regression #57400

Merged

ghost locked as resolved and limited conversation to collaborators Sep 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

don't run LongModuleFileNamesAreSupported test on x86 #57471

don't run LongModuleFileNamesAreSupported test on x86 #57471

adamsitnik commented Aug 16, 2021 •

edited

Loading

ghost commented Aug 16, 2021

krwq Aug 16, 2021

krwq Aug 16, 2021

krwq Aug 16, 2021 •

edited

Loading

adamsitnik Aug 16, 2021

krwq Aug 16, 2021

adamsitnik Aug 16, 2021

JeremyKuhne Aug 16, 2021

stephentoub Aug 16, 2021

jkotas Aug 16, 2021 •

edited

Loading

adamsitnik Aug 16, 2021

jkotas Aug 16, 2021

lewing commented Aug 16, 2021

azure-pipelines bot commented Aug 16, 2021

lewing commented Aug 17, 2021

adamsitnik Aug 17, 2021

adamsitnik commented Aug 17, 2021

jozkee left a comment

	[ConditionalFact(typeof(ProcessModuleTests), nameof(Is_LongModuleFileNamesAreSupported_TestEnabled))]
	public void LongModuleFileNamesAreSupported()
	{
	// To be able to test Long Path support for ProcessModule.FileName we need a .dll that has a path > 260 chars.
	// Since Long Paths support can be disabled (see the ConditionalFact attribute usage above),
	// we just copy "LongName.dll" from bin to a temp directory with a long name and load it from there.
	// Loading from new path is possible because the type exposed by the assembly is not referenced in any explicit way.

don't run LongModuleFileNamesAreSupported test on x86 #57471

don't run LongModuleFileNamesAreSupported test on x86 #57471

Conversation

adamsitnik commented Aug 16, 2021 • edited Loading

ghost commented Aug 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krwq Aug 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Aug 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lewing commented Aug 16, 2021

azure-pipelines bot commented Aug 16, 2021

lewing commented Aug 17, 2021

Choose a reason for hiding this comment

adamsitnik commented Aug 17, 2021

jozkee left a comment

Choose a reason for hiding this comment

adamsitnik commented Aug 16, 2021 •

edited

Loading

krwq Aug 16, 2021 •

edited

Loading

jkotas Aug 16, 2021 •

edited

Loading