Fix MaxWorkingSet_GetNotStarted_ThrowsInvalidOperationException by gwr · Pull Request #105424 · dotnet/runtime

gwr · 2024-07-24T19:46:23Z

stephentoub

Thanks

stephentoub · 2024-07-25T04:19:40Z

These tests needs to be addressed:

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

Lines 659 to 673 in 54a9efd

    
           [Fact] 
        
           [SkipOnPlatform(TestPlatforms.OSX | TestPlatforms.FreeBSD | TestPlatforms.iOS | TestPlatforms.MacCatalyst | TestPlatforms.tvOS, "Getting MinWorkingSet is not supported on OSX, BSD, iOS, MacCatalyst, and tvOS.")] 
        
           public void MinWorkingSet_GetNotStarted_ThrowsInvalidOperationException() 
        
           { 
        
               var process = new Process(); 
        
               Assert.Throws<InvalidOperationException>(() => process.MinWorkingSet); 
        
           } 
        
           [Fact] 
        
           [PlatformSpecific(TestPlatforms.OSX | TestPlatforms.FreeBSD)] 
        
           public void MinWorkingSet_GetMacos_ThrowsPlatformSupportedException() 
        
           { 
        
               var process = new Process(); 
        
               Assert.Throws<PlatformNotSupportedException>(() => process.MinWorkingSet); 
        
           }

gwr · 2024-07-25T12:58:41Z

These tests needs to be addressed:

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

Lines 659 to 673 in 54a9efd

[Fact]

[SkipOnPlatform(TestPlatforms.OSX | TestPlatforms.FreeBSD | TestPlatforms.iOS | TestPlatforms.MacCatalyst | TestPlatforms.tvOS, "Getting MinWorkingSet is not supported on OSX, BSD, iOS, MacCatalyst, and tvOS.")]

public void MinWorkingSet_GetNotStarted_ThrowsInvalidOperationException()

{

var process = new Process();

Assert.Throws<InvalidOperationException>(() => process.MinWorkingSet);

}

[Fact]

[PlatformSpecific(TestPlatforms.OSX | TestPlatforms.FreeBSD)]

public void MinWorkingSet_GetMacos_ThrowsPlatformSupportedException()

{

var process = new Process();

Assert.Throws<PlatformNotSupportedException>(() => process.MinWorkingSet);

}

Yeah, the SkipOnPlatform line(s) probably need adjustment. I think these tests should now pass on FreeBSD and OSX.
Someone needs to find time to remove the OS "skip" for each of these and try the test with that fix.

am11 · 2024-07-25T13:09:39Z

In CI, this test is failing on osx with your changes:

    System.Diagnostics.Tests.ProcessTests.MinWorkingSet_GetMacos_ThrowsPlatformSupportedException [FAIL]
      Assert.Throws() Failure: Exception type was not an exact match
      Expected: typeof(System.PlatformNotSupportedException)
      Actual:   typeof(System.InvalidOperationException)
      ---- System.InvalidOperationException : No process is associated with this object.
      Stack Trace:
        /_/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs(672,0): at System.Diagnostics.Tests.ProcessTests.MinWorkingSet_GetMacos_ThrowsPlatformSupportedException()
        
           at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
        /_/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBaseInvoker.cs(50,0): at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
        ----- Inner Stack Trace -----
        /_/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs(949,0): at System.Diagnostics.Process.EnsureState(State state)
        /_/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs(961,0): at System.Diagnostics.Process.EnsureState(State state)
        /_/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.BSD.cs(60,0): at System.Diagnostics.Process.GetWorkingSetLimits(IntPtr& minWorkingSet, IntPtr& maxWorkingSet)
        /_/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs(1014,0): at System.Diagnostics.Process.EnsureWorkingSetLimits()
        /_/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs(295,0): at System.Diagnostics.Process.get_MinWorkingSet()
        /_/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs(672,0): at System.Diagnostics.Tests.ProcessTests.<>c__DisplayClass40_0.<MinWorkingSet_GetMacos_ThrowsPlatformSupportedException>b__0()
    System.Diagnostics.Tests.ProcessTests.TestProcessRecycledPid [SKIP]

https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-105424-merge-dd30dcce610b4a389e/System.Diagnostics.Process.Tests/1/console.4d1998ff.log?helixlogtype=result

am11 · 2024-07-25T13:18:42Z

Yeah, the SkipOnPlatform line(s) probably need adjustment. I think these tests should now pass on FreeBSD and OSX.

Feel free to merge both tests into one and delete the [Skip line here.

@Thefrank, a heads-up for the next test run on FreeBSD after this PR is merged.

gwr · 2024-07-25T14:15:02Z

In CI, this test is failing on osx with your changes:
    System.Diagnostics.Tests.ProcessTests.MinWorkingSet_GetMacos_ThrowsPlatformSupportedException [FAIL]
    System.Diagnostics.Tests.ProcessTests.TestProcessRecycledPid [SKIP]
https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-105424-merge-dd30dcce610b4a389e/System.Diagnostics.Process.Tests/1/console.4d1998ff.log?helixlogtype=result

Thanks. Clearly the fix in Process.BSD.cs needs testing on OSX and FreBSD.
I've taken a stab at what I think will be needed to fix it, pushed to the PR.

gwr · 2024-07-25T16:23:33Z

+	// XXX This appears to be testing for incorrect behavoir which has been fixed.
+	// XXX See https://github.com/dotnet/runtime/issues/105422
+	// The correct exception for a new process is: InvalidOperationException
+#if 0


Is this #if 0 not allowed? I don't mean for it to stay long term, just until testing is complete.

We wouldn't merge this, if that's what you mean. If you want to experiment with it in CI, that's fine. I'm going to move the PR to be draft until it's ready.

OK, though I was hoping the CI stuff could help me with some of this, and now it seems that doesn't happen.

CI still runs on drafts

Ah, OK. I guess I misunderstood what I saw on the web page. Thx.

gwr · 2024-07-27T18:11:36Z

I reorganized the changes related to this so it can go after #105403

gwr · 2024-08-18T15:42:45Z

Rebased onto main

Fixes issue dotnet#105422 On MacOS, FreeBSD, SunOS (the ports that share ProcessBSD.c) the get/set WorkingSet methods only work on the current process. Skip the parts of tests that operate on other processes. Remove now redundant MacOS-speicifc tests.

am11 · 2024-08-22T16:15:22Z

@stephentoub please give it another pass. :) (the temporary blocks were removed)

gwr · 2024-08-22T19:38:38Z

Can anyone tell me what to do about the two runtime checks that reported failure?

gwr · 2024-08-28T17:34:48Z

nudge 2

gwr · 2024-09-12T13:16:21Z

nudge 3

am11 · 2024-09-12T17:25:42Z

Can anyone tell me what to do about the two runtime checks that reported failure?

When Build Analysis leg is green, that means the remaining ones are known failures unrelated to your changes.

am11 · 2024-10-21T08:11:34Z

@marek-safar it applies to both of those operating systems.

danmoseley · 2024-12-02T20:33:46Z

@stephentoub this seems good to go?

gwr · 2024-12-19T17:54:36Z

Nudge...

stephentoub

One question, otherwise LGTM. Thanks.

ghost added the area-System.Diagnostics.Process label Jul 24, 2024

dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Jul 24, 2024

gwr mentioned this pull request Jul 24, 2024

SunOS process and thread support #105403

Merged

am11 added os-freebsd FreeBSD OS os-mac-os-x macOS aka OSX labels Jul 24, 2024

This was referenced Jul 24, 2024

[browser] HalfTests.ExplicitConversion_FromSingle failing due to NaN != NaN #103347

Open

TimeProviderTests.TestProviderTimer failed in CI #103459

Closed

stephentoub approved these changes Jul 25, 2024

View reviewed changes

stephentoub self-requested a review July 25, 2024 04:20

gwr force-pushed the bsd-ws-limits branch from f46bebc to f064815 Compare July 25, 2024 15:23

gwr commented Jul 25, 2024

View reviewed changes

gwr force-pushed the bsd-ws-limits branch from f064815 to 33b69d0 Compare July 25, 2024 16:25

stephentoub marked this pull request as draft July 25, 2024 16:28

gwr force-pushed the bsd-ws-limits branch 2 times, most recently from 2228476 to 6825a06 Compare July 27, 2024 18:09

gwr force-pushed the bsd-ws-limits branch 2 times, most recently from 5b1ce56 to 7115d6d Compare July 28, 2024 00:37

gwr force-pushed the bsd-ws-limits branch from d6d218d to 328af4c Compare August 18, 2024 15:41

gwr requested a review from am11 August 18, 2024 15:43

build-analysis Bot mentioned this pull request Aug 18, 2024

Test failure "RemoteExecutionException : Half-way through waiting for remote process." in System.Threading.ThreadPools.Tests.ThreadPoolTests.IOCompletionPortCountConfigVarTest #106494

Closed

am11 reviewed Aug 21, 2024

View reviewed changes

Comment thread src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.BSD.cs

gwr force-pushed the bsd-ws-limits branch 2 times, most recently from 311df71 to f72eec3 Compare August 21, 2024 22:01

gwr requested a review from am11 August 21, 2024 22:02

am11 reviewed Aug 21, 2024

View reviewed changes

Comment thread src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Linux.cs Outdated

gwr force-pushed the bsd-ws-limits branch from f72eec3 to 591973d Compare August 21, 2024 23:08

am11 approved these changes Aug 22, 2024

View reviewed changes

gwr force-pushed the bsd-ws-limits branch from 591973d to 10dfd9b Compare August 22, 2024 16:11

marek-safar added os-freebsd FreeBSD OS and removed os-freebsd FreeBSD OS os-mac-os-x macOS aka OSX labels Oct 21, 2024

am11 added the os-mac-os-x macOS aka OSX label Oct 21, 2024

stephentoub reviewed Mar 5, 2025

View reviewed changes

Comment thread src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Linux.cs

stephentoub approved these changes Mar 5, 2025

View reviewed changes

stephentoub merged commit 02ac51e into dotnet:main May 29, 2025

github-actions Bot locked and limited conversation to collaborators Jun 28, 2025

gwr deleted the bsd-ws-limits branch September 11, 2025 12:21

Uh oh!

Conversation

gwr commented Jul 24, 2024

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

stephentoub commented Jul 25, 2024

Uh oh!

gwr commented Jul 25, 2024

Uh oh!

am11 commented Jul 25, 2024

Uh oh!

am11 commented Jul 25, 2024

Uh oh!

gwr commented Jul 25, 2024

Uh oh!

gwr Jul 25, 2024

Choose a reason for hiding this comment

Uh oh!

stephentoub Jul 25, 2024

Choose a reason for hiding this comment

Uh oh!

gwr Jul 25, 2024

Choose a reason for hiding this comment

Uh oh!

stephentoub Jul 25, 2024

Choose a reason for hiding this comment

Uh oh!

gwr Jul 25, 2024

Choose a reason for hiding this comment

Uh oh!

gwr commented Jul 27, 2024

Uh oh!

gwr commented Aug 18, 2024

Uh oh!

Uh oh!

Uh oh!

am11 commented Aug 22, 2024

Uh oh!

gwr commented Aug 22, 2024

Uh oh!

gwr commented Aug 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gwr commented Sep 12, 2024

Uh oh!

am11 commented Sep 12, 2024

Uh oh!

am11 commented Oct 21, 2024

Uh oh!

danmoseley commented Dec 2, 2024

Uh oh!

gwr commented Dec 19, 2024

Uh oh!

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gwr commented Aug 28, 2024 •

edited

Loading