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

Consolidate approach for conditional tests that check for privileged processes #78793

Merged
merged 9 commits into from
Nov 30, 2022
Merged

Consolidate approach for conditional tests that check for privileged processes #78793

merged 9 commits into from
Nov 30, 2022

Conversation

jeffhandley
Copy link
Member

@jeffhandley jeffhandley added this to the 8.0.0 milestone Nov 24, 2022
@jeffhandley jeffhandley self-assigned this Nov 24, 2022
@ghost
Copy link

ghost commented Nov 24, 2022

Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti
See info in area-owners.md if you want to be subscribed.

Issue Details

Spawned from a PR comment by @stephentoub in Use Environment.IsPrivilegedProcess #77847

Author: jeffhandley
Assignees: jeffhandley
Labels:

area-System.Diagnostics.Tracing, test-enhancement
Milestone: 8.0.0

@jeffhandley jeffhandley mentioned this pull request Nov 24, 2022
Merged
This was referenced Nov 24, 2022
Closed
Open
@build-analysis build-analysis bot mentioned this pull request Nov 27, 2022
Closed
@jeffhandley
Copy link
Member Author

@stephentoub This should be ready for re-review. I've addressed the failing checks and found that on browser there's no concept of a privileged process, so that's now accounted for in AdminHelpers.IsProcessElevated().

@stephentoub
Copy link
Member

I've addressed the failing checks and found that on browser there's no concept of a privileged process

Shouldn't Environment.IsPrivilegedProcess just return false on browser then?

@jeffhandley
Copy link
Member Author

Shouldn't Environment.IsPrivilegedProcess just return false on browser then?

It does. Environment.IsPrivilegedProcess calls the platform-specific IsPrivilegedProcessCore, which just returns false.

I also walked the blame back for the affected tests and found that when they were being enabled for WASM, that's when the IsBrowser conditions were added to short-circuit to false despite satisfying !IsWindows.

I did just find a unit test that was being skipped on browser that can now be enabled since AdminHelpers.IsProcessElevated() will now return false on browser instead of throwing.

@stephentoub
Copy link
Member

It does. Environment.IsPrivilegedProcess calls the platform-specific IsPrivilegedProcessCore, which just returns false.

Ah, I forgot that AdminHelpers.IsProcessElevated is reimplementing the equivalent of IsPrivilegedProcess.

Thanks.

stephentoub
stephentoub approved these changes Nov 30, 2022
View reviewed changes
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!

@jeffhandley
PR feedback
cf6c143 
Omit [PlatformSpecific] in modified tests that are in platform-specific test files.

Remove [Outerloop] attributes from privileged process tests.

Better multithreaded handling in PlatformDetection.IsPrivilegedProcess
@jeffhandley
Copy link
Member Author

MacCatalyst failure is #78778

@jeffhandley jeffhandley merged commit 5553691 into dotnet:main Nov 30, 2022
@jeffhandley jeffhandley deleted the jeffhandley/superuser branch November 30, 2022 18:26
@ghost ghost locked as resolved and limited conversation to collaborators Dec 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub approved these changes

Assignees

@jeffhandley jeffhandley

Labels
area-System.Diagnostics.Tracing test-enhancement Improvements of test source code
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@jeffhandley @stephentoub