Repository navigation
Separate tests on Windows CI to Admin-Required and Non-Admin-Required - #2280
Conversation
|
Hi sergei (@vors), I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
TTYL, MSBOT; |
cac4ad4 to
0bf65b3
Compare
|
I observed that we get test failures because we run tests that require admin without admin. |
427ba0b to
e3b5001
Compare
|
This is blocked on #2346 |
There was a problem hiding this comment.
this looks like debug output - is it necessary?
There was a problem hiding this comment.
No, that's how the parent process gets notification that the child process ended.
There was a problem hiding this comment.
art-UnelevatedProcess [](start = 18, length = 21)
it's too bad that runas doesn't give you any useful info about the running process
There was a problem hiding this comment.
Indeed.
I was trying to find a way to avoid what I'm doing here.
There was a problem hiding this comment.
break [](start = 24, length = 5)
are you relying on CI to bail if things take too long?
There was a problem hiding this comment.
yeah. Should I include an explicit timeout logic?
There was a problem hiding this comment.
possibly, i believe that appveyor has it's own time limitations on execution, it may be that nothing is needed here (or rather, any number you select will probably be wrong, up to the value that AppVeyor has)
There was a problem hiding this comment.
Yeah, we control the timeout for all our project globally and it's currently 3 hours. I would leave it as is for now.
There was a problem hiding this comment.
empty line above this why?
There was a problem hiding this comment.
Artifacts from my experiments. Fixing.
There was a problem hiding this comment.
This should be
if (-not $PSBoundParameters.ContainsKey('ExcludeTag'))
There was a problem hiding this comment.
coreClrTestParams [](start = 81, length = 17)
don't you need to exclude RequiresAdmin tag for non admin?
There was a problem hiding this comment.
Great catch!
2778ea8 to
322a959
Compare
|
Also found #2366 during this work. Will include fix for it in the next iteration of the PR. |
bcf53ac to
c05eae3
Compare
This helps avoid problem when run as non-elevated user.
That will enable running tests without Admin in CI
- Add RequireAdminOnWindows to the list of blessed tags - Fix a bug in Get-PesterTag where we accepted 'Slow' as a proper priority - Fix missed comma in Start-PSPester parameters - Add documentation about new Pester tag - Fix finishing logic for Start-PSPester -Unelevate
due to the issue PowerShell#2334
Split LocalMachine test case into two separate tests: - One without overriden policy (throws exception) - One with overriden policy
c758b2e to
735bcca
Compare
| } | ||
|
|
||
| $errorId | Should Be 'ExecutionPolicyOverride,Microsoft.PowerShell.Commands.SetExecutionPolicyCommand' | ||
| Get-ExecutionPolicy -Scope LocalMachine | Should Be "ByPass" |
There was a problem hiding this comment.
Get-ExecutionPolicy -Scope LocalMachine | Should Be "ByPass" [](start = 12, length = 60)
I don't quite understand this:
attempt to set the localmachine policy, which is supposed to fail
then check the localmachine policy to see if it is what you just tried to set it to?
Shouldn't you get the localmachine policy in the BeforeAll, store it, then check against that value (rather than assume that ByPass will be correct)
There was a problem hiding this comment.
Nope, that's how PowerShell works. The full error message text is:
Set-ExecutionPolicy : Windows PowerShell updated your execution policy successfully, but the setting is overridden by
a policy defined at a more specific scope. Due to the override, your shell will retain its current effective
execution policy of RemoteSigned. Type "Get-ExecutionPolicy -List" to view your execution policy settings. For more
information please see "Get-Help Set-ExecutionPolicy".
FullyQualifiedErrorId : ExecutionPolicyOverride,Microsoft.PowerShell.Commands.SetExecutionPolicyCommand
Windows PowerShell updated your execution policy successfully, but the setting is overridden by
a policy defined at a more specific scope.
There was a problem hiding this comment.
I'm going to add comments about it in the code, because it's indeed confusing
Mike Richmond (mirichmo)
left a comment
There was a problem hiding this comment.
I'd like to see two minor modifications. Other than that it looks good.
There was a problem hiding this comment.
The guidance for FQEID validation is:
try {
Set-ExecutionPolicy -Scope LocalMachine -ExecutionPolicy ByPass
throw "Expected exception: ExecutionPolicyOverride"
}
catch {
$_.FullyQualifiedErrorId | Should Be 'ExecutionPolicyOverride,Microsoft.PowerShell.Commands.SetExecutionPolicyCommand'
}
}
There was a problem hiding this comment.
Add a comment here explaining why this is necessary.
78eebbd to
de75f7b
Compare
'-Scope LocalMachine is Settable, but overridden'
de75f7b to
9cc905d
Compare
Mike Richmond (mirichmo)
left a comment
There was a problem hiding this comment.
The ExecutionPolicy changes LGTM
|
|
Separate tests on Windows CI to Admin-Required and Non-Admin-Required
Fixes #1486
This helps avoid problem when run as non-elevated user.
That will enable running tests without Admin in CI
Before merging, we would need to update documentation:
Start-PSPester -UnelevateRequireAdminOnWindows)