/PowerShell

fix set-service failing test #4802

Merged
merged 6 commits into from Sep 15, 2017
+76 −48

Conversation

Reviewers

@daxian-dbw daxian-dbw

@iSazonov iSazonov

@adityapatwardhan adityapatwardhan

Assignees

@iSazonov iSazonov

Labels
Breaking-Change
Projects
None yet
Milestone
No milestone
5 participants
@SteveL-MSFT @iSazonov @markekraus @daxian-dbw @msftclas
@SteveL-MSFT
Owner

SteveL-MSFT commented Sep 10, 2017 •
Edited 1 time

missed -ErrorAction Stop
changed new-service to return error when given unsupported startuptype

Fix #4800
Fix #4803

@SteveL-MSFT SteveL-MSFT assigned iSazonov Sep 10, 2017

@SteveL-MSFT SteveL-MSFT requested a review from iSazonov Sep 10, 2017

@msftclas msftclas added the cla-not-required label Sep 10, 2017

@SteveL-MSFT SteveL-MSFT referenced this pull request Sep 10, 2017

Merged

Set-Service and New-Service test coverage #4785

@iSazonov
Collaborator

iSazonov commented Sep 11, 2017

Blocked by #4803.

SteveL-MSFT added some commits Sep 10, 2017

@SteveL-MSFT
[feature] 
missed -ErrorAction Stop

Some checks were not successful

1 failing and 1 successful checks
continuous-integration/appveyor/pr — AppVeyor build failed
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
babc348
@SteveL-MSFT
[feature] 
made cmdlet write error when using unsupported startuptype
re-enabled test

All checks have passed

2 successful checks
continuous-integration/appveyor/pr — AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
c67a75e

@SteveL-MSFT SteveL-MSFT requested review from adityapatwardhan and daxian-dbw as code owners Sep 11, 2017

@SteveL-MSFT SteveL-MSFT added the Breaking-Change label Sep 11, 2017

@markekraus
Collaborator

markekraus commented Sep 11, 2017

@SteveL-MSFT something should probably be done about Set-Service.Tests.ps1#L25. That will continue to fail on Linux and macOS as the literal will still be parsed even though the IT is being skipped, and Get-Service is unavailable on Linux/macOS.
@SteveL-MSFT
Owner

SteveL-MSFT commented Sep 11, 2017

@markekraus I see. will fix.
@SteveL-MSFT
[feature] 
fix test so that a script block only runs if It is run

All checks have passed

2 successful checks
continuous-integration/appveyor/pr — AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
4bde7fb
src/Microsoft.PowerShell.Commands.Management/commands/management/Service.cs
- break;
+ WriteNonTerminatingError(StartupType.ToString(), DisplayName, Name,
+ new ArgumentException(), "CouldNotNewService",
+ ServiceResources.UnsupportedStartupType,
@daxian-dbw

daxian-dbw Sep 11, 2017

Member

There is another instance of Assert(((ServiceStartMode)(-1)) == StartupType, "bad StartupType" in Set-Service. It would be the best if all can be fixed here.

@SteveL-MSFT

SteveL-MSFT Sep 12, 2017

Owner

will fix

...Microsoft.PowerShell.Commands.Management/resources/ServiceResources.resx
@@ -201,4 +201,7 @@
<data name="ComputerAccessDenied" xml:space="preserve">
<value>The command cannot be used to configure the service '{0}' because access to computer '{1}' is denied. Run PowerShell as admin and run your command again.</value>
</data>
+ <data name="UnsupportedStartupType" xml:space="preserve">
+ <value>The startup type '{0}' is not supported by New-Service.</value>
@daxian-dbw

daxian-dbw Sep 11, 2017

Member

by New-Service needs to be changed since the same applies to set-service as well.

@SteveL-MSFT

SteveL-MSFT Sep 12, 2017

Owner

will fix

...powershell/Modules/Microsoft.PowerShell.Management/Set-Service.Tests.ps1
@@ -163,8 +166,7 @@ Describe "Set/New-Service cmdlet tests" -Tags "Feature", "RequireAdminOnWindows"
[System.Management.Automation.PSCredential]::new("username",
(ConvertTo-SecureString "PlainTextPassword" -AsPlainText -Force)))
},
- # This test case fails due to #4803. Disabled for now.
- # @{name = 'badstarttype'; parameter = "StartupType"; value = "System"},
+ @{name = 'badstarttype'; parameter = "StartupType"; value = "System"},
@iSazonov

iSazonov Sep 12, 2017

Collaborator

Should we add "Boot" too?
Should we check valid and invalid values from System.ServiceProcess.ServiceStartMode? If yes it is better refactor the tests. And for Set-Service too.

@SteveL-MSFT

SteveL-MSFT Sep 12, 2017

Owner

will change

@SteveL-MSFT
[feature] 
turned redundant code into helper function used by New/Set-Service
added tests for Set-Service and unsupported StartupTypes

All checks have passed

2 successful checks
continuous-integration/appveyor/pr — AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
94561f4
@SteveL-MSFT
Owner

SteveL-MSFT commented Sep 12, 2017

@iSazonov @daxian-dbw feedback addressed
src/Microsoft.PowerShell.Commands.Management/commands/management/Service.cs
@@ -1684,22 +1682,14 @@ protected override void ProcessRecord()
|| (ServiceStartMode)(-1) != StartupType)
{
DWORD dwStartType = NativeMethods.SERVICE_NO_CHANGE;
@iSazonov

iSazonov Sep 13, 2017

Collaborator

We do the initialiazation in GetNativeStartupType. So we could remove this and below call by C# 7.0 pattern NativeMethods.GetNativeStartupType(StartupType, out DWORD dwStartType)).

@SteveL-MSFT

SteveL-MSFT Sep 13, 2017

Owner

I can make that change for New-Service, but for Set-Service, I need to use dwStartType on line 1697 so it has to be initialized.

@iSazonov

iSazonov Sep 13, 2017

Collaborator

Thanks for clafiry. I think we should leave as is.
Closed.

@daxian-dbw

daxian-dbw Sep 14, 2017

Member

In New-Service, dwStartType is also used in line 2045 when calling NativeMethods.CreateServiceW.
It turns out the following code works:

static void Blah ()
{
    if (!int.TryParse("1", out int result))
    {
        Console.WriteLine("Failed");
    }

    Console.WriteLine(result);
}

It would be the best if we can do it in a consistent way in those 2 places.

@SteveL-MSFT

SteveL-MSFT Sep 14, 2017

Owner

Addressed

src/Microsoft.PowerShell.Commands.Management/commands/management/Service.cs
+ /// <returns>
+ /// If a supported StartupType is provided, funciton returns true, otherwise false.
+ /// </returns>
+ internal static bool GetNativeStartupType(ServiceStartMode StartupType, out DWORD dwStartType)
@iSazonov

iSazonov Sep 13, 2017

Collaborator

Maybe TryGetNativeStartupType()?

@SteveL-MSFT

SteveL-MSFT Sep 13, 2017

Owner

Will change

@SteveL-MSFT
[feature] 
address PR feedback

Some checks were not successful

1 failing and 1 successful checks
continuous-integration/appveyor/pr — AppVeyor build failed
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
25b43a6
src/Microsoft.PowerShell.Commands.Management/commands/management/Service.cs
@@ -1684,22 +1682,14 @@ protected override void ProcessRecord()
|| (ServiceStartMode)(-1) != StartupType)
{
DWORD dwStartType = NativeMethods.SERVICE_NO_CHANGE;
- switch (StartupType)
+ if ((ServiceStartMode)(-1) != StartupType &&
@daxian-dbw

daxian-dbw Sep 13, 2017

Member

It seems (ServiceStartMode)(-1) != StartupType && should be removed.

@SteveL-MSFT

SteveL-MSFT Sep 13, 2017

Owner

Will change, but will need to change some other code to make this work. Seems like -1 shouldn't be needed at all.

@SteveL-MSFT

SteveL-MSFT Sep 13, 2017

Owner

Actually, still need -1 which will be treated as equivalent to SERVICE_NO_CHANGE

@daxian-dbw

daxian-dbw Sep 13, 2017

Member

By looking at the original code, for New-Service, SERVICE_AUTO_START would be used in case StartupType is -1. So it seems we cannot just use dwStartType = NativeMethods.SERVICE_NO_CHANGE for (ServiceStartMode)(-1)

@SteveL-MSFT

SteveL-MSFT Sep 14, 2017

Owner

New-Service defaults to Automatic whereas Set-Service defaults to -1 to indicate no change.

@daxian-dbw

daxian-dbw Sep 14, 2017 •
Edited 1 time

Member

Is it possible that the value of StartupType can be set to (-1)?
In the original code, dwStartType will still be SERVICE_AUTO_START even if StartupType is (-1), while it will be SERVICE_NO_CHANGE after the change.

Turns out we can't do [System.ServiceProcess.ServiceStartMode]-1 in PowerShell, so we are good.

@SteveL-MSFT

SteveL-MSFT Sep 14, 2017

Owner

I'll fix it so if it's (-1), it should default to Auto for New-Service

@SteveL-MSFT
[feature] 
made StartupType(-1) equivalent to Win32 SERVICE_NO_CHANGE and moved to TryGetNativeStartupType

All checks have passed

2 successful checks
continuous-integration/appveyor/pr — AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
7b86b32
@SteveL-MSFT
Owner

SteveL-MSFT commented Sep 14, 2017

@daxian-dbw feedback addressed

@SteveL-MSFT SteveL-MSFT referenced this pull request Sep 14, 2017

Merged

Added WSMan Config provider tests #4756

@daxian-dbw

daxian-dbw approved these changes Sep 14, 2017
View changes

LGTM except for a non-blocking comment.

src/Microsoft.PowerShell.Commands.Management/commands/management/Service.cs
@@ -1684,22 +1682,14 @@ protected override void ProcessRecord()
|| (ServiceStartMode)(-1) != StartupType)
{
DWORD dwStartType = NativeMethods.SERVICE_NO_CHANGE;
@iSazonov

iSazonov Sep 13, 2017

Collaborator

We do the initialiazation in GetNativeStartupType. So we could remove this and below call by C# 7.0 pattern NativeMethods.GetNativeStartupType(StartupType, out DWORD dwStartType)).

@SteveL-MSFT

SteveL-MSFT Sep 13, 2017

Owner

I can make that change for New-Service, but for Set-Service, I need to use dwStartType on line 1697 so it has to be initialized.

@iSazonov

iSazonov Sep 13, 2017

Collaborator

Thanks for clafiry. I think we should leave as is.
Closed.

@daxian-dbw

daxian-dbw Sep 14, 2017

Member

In New-Service, dwStartType is also used in line 2045 when calling NativeMethods.CreateServiceW.
It turns out the following code works:

static void Blah ()
{
    if (!int.TryParse("1", out int result))
    {
        Console.WriteLine("Failed");
    }

    Console.WriteLine(result);
}

It would be the best if we can do it in a consistent way in those 2 places.

@SteveL-MSFT

SteveL-MSFT Sep 14, 2017

Owner

Addressed

@SteveL-MSFT
Owner

SteveL-MSFT commented Sep 14, 2017

@daxian-dbw can you review latest change?
@daxian-dbw
Member

daxian-dbw commented Sep 14, 2017 •
Edited 1 time

@SteveL-MSFT we cannot pass the value (-1) to -StartupType parameter. The parameter binding will fail because powershell cannot convert -1 to [System.ServiceProcess.ServiceStartMode]. That means StartupType in New-Service can never be -1, so we should be good without the last commit.
That's why I crossed out my original comment :)
@SteveL-MSFT
Owner

SteveL-MSFT commented Sep 14, 2017

@daxian-dbw yeah, I went through the code and for New-Service, StartupType defaults to Auto and can't ever be set to (-1), so I'll revert the last commit.
@SteveL-MSFT
Owner

SteveL-MSFT commented Sep 14, 2017

Last commit was reverted. We should be good to go.
@iSazonov
Collaborator

iSazonov commented Sep 15, 2017

@SteveL-MSFT @daxian-dbw Is the PR ready to merge?
@daxian-dbw
Member

daxian-dbw commented Sep 15, 2017 •
Edited 1 time

@iSazonov Yes, it's good to go.
BTW, please don't directly use the history commit messages as the description when squashing and merging. Instead, it's better to summarize the changes of the PR.

@iSazonov iSazonov merged commit b723d6b into PowerShell:master Sep 15, 2017
2 checks passed

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@SteveL-MSFT SteveL-MSFT deleted the SteveL-MSFT:set-service-fail branch Sep 15, 2017

@SteveL-MSFT SteveL-MSFT referenced this pull request Oct 4, 2017

Merged

beta.8 changelog draft #5006

powercode pushed a commit to powercode/PowerShell that referenced this pull request Oct 19, 2017

@SteveL-MSFT
fix set-service failing test (#4802) 
* Add '-ErrorAction Stop' in Set-Service.Tests.ps1 to correctly test exceptions.
* Refactor StartupType service code and add a throw for disabled startup types (System, Boot).
* Made StartupType(-1) equivalent to Win32 SERVICE_NO_CHANGE.
3396c44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment