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.

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

Fix MaxWorkingSet_GetNotStarted_ThrowsInvalidOperationException #105424

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gwr
Copy link
Contributor

@gwr gwr commented Jul 24, 2024

Fixes #105422

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Thanks

@stephentoub
Copy link
Member

These tests needs to be addressed:

[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);
}

@stephentoub stephentoub self-requested a review July 25, 2024 04:20
@gwr
Copy link
Contributor Author

gwr commented Jul 25, 2024

These tests needs to be addressed:

[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
Copy link
Member

am11 commented Jul 25, 2024

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
Copy link
Member

am11 commented Jul 25, 2024

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
Copy link
Contributor Author

gwr commented Jul 25, 2024

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.

// 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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

CI still runs on drafts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@gwr
Copy link
Contributor Author

gwr commented Jul 27, 2024

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

@am11
Copy link
Member

am11 commented Jul 28, 2024

Why is MacOS apparently skipping these tests when the SkipOnPlatform does not list OSX?


How did you determine that they are skipping? Change the exception in Assert.Throws<InvalidOperationException> to something like InvalidCastException, if it fails osx CI that means it is running. The success logs aren’t as verbose as the failure logs.

@gwr
Copy link
Contributor Author

gwr commented Jul 29, 2024

Why is MacOS apparently skipping these tests when the SkipOnPlatform does not list OSX?


How did you determine that they are skipping? Change the exception in Assert.Throws<InvalidOperationException> to something like InvalidCastException, if it fails osx CI that means it is running. The success logs aren’t as verbose as the failure logs.

OK, Thank-you! That was it! So I've learned that the test output cannot be trusted to tell you want ran.
With the Assert.Throws lines changed as you suggest, I've verified that MacOS runs all of those tests.

�[m�[37m      Condition(s) not met: "IsSupported"
�[m�[31;1m    System.Diagnostics.Tests.ProcessTests.MaxWorkingSet_GetNotStarted_ThrowsInvalidOperationException [FAIL]
�[m�[37m      Assert.Throws() Failure: Exception type was not an exact match
�[m�[37m      Expected: typeof(System.InvalidCastException)
�[m�[37m      Actual:   typeof(System.InvalidOperationException)
�[m�[37m      ---- System.InvalidOperationException : No process is associated with this object.

I believe this means wen can just remove the (now duplicate) MacOS-specific tests.

@gwr
Copy link
Contributor Author

gwr commented Aug 18, 2024

Rebased onto main

@gwr gwr force-pushed the bsd-ws-limits branch 2 times, most recently from 311df71 to f72eec3 Compare August 21, 2024 22:01
@gwr gwr requested a review from am11 August 21, 2024 22:02
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
Copy link
Member

am11 commented Aug 22, 2024

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

@gwr
Copy link
Contributor Author

gwr commented Aug 22, 2024

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

@gwr
Copy link
Contributor Author

gwr commented Aug 28, 2024

nudge 2

@gwr
Copy link
Contributor Author

gwr commented Sep 12, 2024

nudge 3

@am11
Copy link
Member

am11 commented Sep 12, 2024

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.

@marek-safar 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 am11 added the os-mac-os-x macOS aka OSX label Oct 21, 2024
@am11
Copy link
Member

am11 commented Oct 21, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Diagnostics.Process community-contribution Indicates that the PR has been added by a community member os-freebsd FreeBSD OS os-mac-os-x macOS aka OSX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

System.Diagnostics.Tests.ProcessTests.MaxWorkingSet_GetNotStarted wrong exception code on BSD-ish systems
4 participants