Skip to content

Separate tests on Windows CI to Admin-Required and Non-Admin-Required - #2280

Merged
Mike Richmond (mirichmo) merged 15 commits into
PowerShell:masterfrom
vors:admin-tests
Sep 29, 2016
Merged

Mike Richmond (mirichmo) merged 15 commits into
PowerShell:masterfrom
vors:admin-tests

Conversation

@vors

@vors sergei (vors) commented Sep 16, 2016 •

Copy link
Copy Markdown
Collaborator

Fixes #1486

  • Add 'RequireAdminOnWindows' tag to some tests suites
  • Change install scope for Install-PackageProvider.Tests.ps1
    This helps avoid problem when run as non-elevated user.
  • Restructure PowerShellGet.Tests.ps1 to enable 'RequireAdminOnWindows' tagging
  • Add Start-PSPester -Unelevated
    That will enable running tests without Admin in CI
  • Make Invoke-AppVeyorTest run elevated and non-elevated tests separately

Before merging, we would need to update documentation:

  • About using Start-PSPester -Unelevate
  • About Tags that are used in our Pester tests (add RequireAdminOnWindows)

@msftclas

Hi sergei (@vors), I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!


It looks like you're a Microsoft contributor (Sergei Vorobev). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

@vors

Copy link
Copy Markdown
Collaborator Author

I observed that we get test failures because we run tests that require admin without admin.
Currently I didn't find to Invoke-Pester with tests with all tests.
Opened pester/Pester#608 for discussion.

@vors
sergei (vors) force-pushed the admin-tests branch 2 times, most recently from 427ba0b to e3b5001 Compare September 22, 2016 19:24
@vors

Copy link
Copy Markdown
Collaborator Author

This is blocked on #2346
cc Mike Richmond (@mirichmo)

@vors sergei (vors) added the Blocked blocked on something external to this repo label Sep 22, 2016
Comment thread build.psm1 Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this looks like debug output - is it necessary?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

No, that's how the parent process gets notification that the child process ended.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

ok

Comment thread build.psm1 Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

art-UnelevatedProcess [](start = 18, length = 21)

it's too bad that runas doesn't give you any useful info about the running process

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Indeed.
I was trying to find a way to avoid what I'm doing here.

Comment thread build.psm1 Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

break [](start = 24, length = 5)

are you relying on CI to bail if things take too long?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

yeah. Should I include an explicit timeout logic?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, we control the timeout for all our project globally and it's currently 3 hours. I would leave it as is for now.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

empty line above this why?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Artifacts from my experiments. Fixing.

Comment thread build.psm1 Outdated

@vors sergei (vors) Sep 22, 2016 •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This should be

if (-not $PSBoundParameters.ContainsKey('ExcludeTag'))

Comment thread tools/appveyor.psm1 Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

coreClrTestParams [](start = 81, length = 17)

don't you need to exclude RequiresAdmin tag for non admin?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Great catch!

@vors

Copy link
Copy Markdown
Collaborator Author

Also found #2366 during this work. Will include fix for it in the next iteration of the PR.

Sergei Vorobev added 13 commits September 29, 2016 12:29
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
Split LocalMachine test case into two separate tests:
- One without overriden policy (throws exception)
- One with overriden policy
@vors

Copy link
Copy Markdown
Collaborator Author

Mike Richmond (@mirichmo) done

}

$errorId | Should Be 'ExecutionPolicyOverride,Microsoft.PowerShell.Commands.SetExecutionPolicyCommand'
Get-ExecutionPolicy -Scope LocalMachine | Should Be "ByPass"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'm going to add comments about it in the code, because it's indeed confusing

@mirichmo Mike Richmond (mirichmo) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd like to see two minor modifications. Other than that it looks good.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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' 

}

}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

done

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Add a comment here explaining why this is necessary.

'-Scope LocalMachine is Settable, but overridden'

@mirichmo Mike Richmond (mirichmo) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The ExecutionPolicy changes LGTM

@vors sergei (vors) removed the Blocked blocked on something external to this repo label Sep 29, 2016
@mirichmo
Mike Richmond (mirichmo) merged commit 374e1d1 into PowerShell:master Sep 29, 2016
@JamesWTruher

Copy link
Copy Markdown
Collaborator

:shipit:

@vors
sergei (vors) deleted the admin-tests branch February 9, 2017 05:18
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
Separate tests on Windows CI to Admin-Required and Non-Admin-Required
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants