Added WSMan Config provider tests #4756

SteveL-MSFT · 2017-09-05T20:51:13Z

added tests for get-item, get-childitem, set-item, new-item remove-item, clear-item, dynamic parameters
merged CoreClr and FullClr code
did not add tests for clientcert handling

Part of #4158

Should get line coverage from 50.39% to 72.2%

Lots of error handling code not hit

iSazonov · 2017-09-12T03:57:12Z

test/powershell/Modules/Microsoft.WSMan.Management/ConfigProvider.Tests.ps1

Should we wait that the service stop?

Stop-Service should be waiting by default, it has a -nowait switch

iSazonov · 2017-09-12T03:59:15Z

test/powershell/Modules/Microsoft.WSMan.Management/ConfigProvider.Tests.ps1

Do we want have all tab complete tests in one file TabCompletion.Tests.ps1?

I'll move them

iSazonov · 2017-09-12T04:04:01Z

test/powershell/Modules/Microsoft.WSMan.Management/ConfigProvider.Tests.ps1

Why the Should in finally block? Maybe add a comment?

The tests are for new-item and corresponding remove-item. Should validates that the remove-item worked. The test title indicates this and I thought was obvious enough. In case some other Should validation fails before the remove-item I still want to cleanup. Do you think a comment is needed?

Thanks for clarify.

Closed.

iSazonov · 2017-09-12T04:08:30Z

test/powershell/Modules/Microsoft.WSMan.Management/ConfigProvider.Tests.ps1

Should we move the test to a file with all Help tests? Tum more what we want to move Help subsystem in separate module.

This test is a bit different from the HelpSystem.Tests.ps1 as it's testing the implementation of ICmdletProviderSupportsHelp in this provider (not testing the HelpSystem). But more than that, it's specific to Windows and requires elevation.

Thanks for clarify. I think we should mention ICmdletProviderSupportsHelp in a comment or Context description.

mirichmo · 2017-09-12T01:00:35Z

test/powershell/Modules/Microsoft.WSMan.Management/ConfigProvider.Tests.ps1

Microsoft.powershell is the Windows PowerShell endpoint. Ideally, PowerShell Core should work with the PowerShell Core endpoint as its template. It would require a call to Enable-PSRemoting within PowerShell Core to set up.

I'll make the change

Had to change it back as using Install-PowerShellRemoting or Enable-PSRemoting was making things complicated even though my tests had no dependency on remoting actually working. Since I'm just relying on a well known existing plugin configuration, I think using microsoft.powershell seems reasonable.

mirichmo · 2017-09-12T01:01:00Z

test/powershell/Modules/Microsoft.WSMan.Management/ConfigProvider.Tests.ps1

Should this user be added to the Administrators group?

Not needed, I don't do any actual remoting. I just need a valid local account to set configuration (runas)

mirichmo · 2017-09-12T01:08:50Z

test/powershell/Modules/Microsoft.WSMan.Management/ConfigProvider.Tests.ps1

Please use more descriptive variable names

will change

mirichmo · 2017-09-12T01:12:46Z

test/powershell/Modules/Microsoft.WSMan.Management/ConfigProvider.Tests.ps1

Would it be useful to also test that it resolves ambiguous parameters in a consistent manner? "-pr" and "-pl" collide at "-p".

There should already be tabcomplete tests to cover that. This is specifically testing WSMan Config Provider's returning dynamic parameters

mirichmo · 2017-09-12T01:14:42Z

test/powershell/Modules/Microsoft.WSMan.Management/ConfigProvider.Tests.ps1

Extra space

mirichmo · 2017-09-13T16:13:57Z

test/powershell/Modules/Microsoft.WSMan.Management/ConfigProvider.Tests.ps1

Out of curiosity, where did these SDDL values come from?

I'll add a comment, but the second one is the default SDDL, the first is a truncated version

mirichmo · 2017-09-13T16:48:26Z

test/powershell/Modules/Microsoft.WSMan.Management/ConfigProvider.Tests.ps1

I'm starting to see patterns here among the tests. I like the descriptive names of the It's, but I'm wondering if they can be combined as TestCases to include the path to the property as well as the property and value. A change isn't necessarily required, but I'm curious if you considered it.

I thought about it while writing the tests, but there's enough differences in the properties and validation I think it would be overly complicated to combine them

mirichmo · 2017-09-13T17:08:48Z

test/powershell/Modules/Microsoft.WSMan.Management/ConfigProvider.Tests.ps1

This is the Windows PowerShell plugin path. It matches the config you have been using throughout, but it will need to be updated

I'll update it to the one for PSCore6

mirichmo · 2017-09-13T17:22:01Z

test/powershell/Modules/Microsoft.WSMan.Management/ConfigProvider.Tests.ps1

We should not use "foo" and "bar" since they violate PolyCheck. Please use other string values.

I believe policheck allows it for code, but not in strings given to users. In any case, if we need to fix this, we should have a separate PR for all the code.

mirichmo · 2017-09-13T17:24:04Z

test/powershell/Modules/Microsoft.WSMan.Management/ConfigProvider.Tests.ps1

Does Remove-Item on a non-existant path throw an error?

I'm testing Remove-Item and also trying to cleanup. I think it may be better since @iSazonov gave similar feedback and have some redundancy where I do the remove-item test within the try and still do cleanup.

SteveL-MSFT · 2017-09-14T05:09:48Z

CI failure waiting on #4802 to be merged

adityapatwardhan · 2017-09-14T16:43:57Z

test/powershell/Host/TabCompletion/TabCompletion.Tests.ps1

Please set $Global:PSDefaultParameterValues to the originalDefaultParameterValues.

Why would we want to mess with global scope?

:-) I asked @JamesWTruher previously the same and he said that is good to exclude scoped isssues.

@JamesWTruher could you point to the PR where we had problems with scoping and PSDefaultParameterValues

Actually, had an issue with scoping where it affected the CIM cmdlet tests on non-Windows. Changed it to global and it fixed the issue. Would like to understand what is happening here as it's not intuitive.

adityapatwardhan · 2017-09-14T16:45:13Z

test/powershell/Host/TabCompletion/TabCompletion.Tests.ps1

Please use a more descriptive variable name.

adityapatwardhan · 2017-09-14T17:08:12Z

test/powershell/Modules/Microsoft.WSMan.Management/ConfigProvider.Tests.ps1

Set $Global:PSDefaultParameterValues instead

adityapatwardhan · 2017-09-14T17:09:28Z

test/powershell/Modules/Microsoft.WSMan.Management/ConfigProvider.Tests.ps1

This check seems unnecessary as $plugin.PSPath is already checked.

true, will remove

adityapatwardhan · 2017-09-14T17:11:23Z

test/powershell/Modules/Microsoft.WSMan.Management/ConfigProvider.Tests.ps1

Please move the value calculated for Should Be to a variable. It will make it more readable.

adityapatwardhan · 2017-09-14T17:24:06Z

test/powershell/Modules/Microsoft.WSMan.Management/ConfigProvider.Tests.ps1

Should this be BeExactly

will change

adityapatwardhan · 2017-09-14T17:27:16Z

test/powershell/Modules/Microsoft.WSMan.Management/ConfigProvider.Tests.ps1

This can be Remove-Item "WSMan:\localhost\Plugin\TestPlugin2\" -Force -ErrorAction SilentlyContinue and remove the Test-Path.

true, will change

adityapatwardhan · 2017-09-14T17:27:30Z

test/powershell/Modules/Microsoft.WSMan.Management/ConfigProvider.Tests.ps1

Same as above.

will change

adityapatwardhan · 2017-09-14T17:28:03Z

test/powershell/Modules/Microsoft.WSMan.Management/ConfigProvider.Tests.ps1

Same as above.

will change

adityapatwardhan · 2017-09-14T17:28:19Z

test/powershell/Modules/Microsoft.WSMan.Management/ConfigProvider.Tests.ps1

Same as above.

will change

mirichmo · 2017-09-15T15:32:56Z

test/powershell/Modules/Microsoft.WSMan.Management/ConfigProvider.Tests.ps1

Can you use Enable-PSRemoting instead? I'd like to steer everyone in that direction now that the cmdlet is supported. An alternative is to modify the script to use the cmdlet internally. That way everyone will use it without noticing.

Thoughts?

I don't need remoting to actual work for the purpose of these tests, I think I'll simplify it by just getting the first endpoint and validating against that

mirichmo · 2017-09-16T00:45:10Z

@SteveL-MSFT Looks like a few tests are failing

SteveL-MSFT · 2017-09-16T02:38:41Z

@mirichmo I'm going to revert back to using microsoft.powershell plugin as my base plugin since that seems to be well defined and trying to make it work with enable-psremoting turned out to be more complicated than it's worth particularly since I'm not actually using remoting for these tests.

SteveL-MSFT · 2017-09-18T04:58:07Z

@adityapatwardhan your feedback has been addressed

iSazonov

LGTM with monor comments.

iSazonov · 2017-09-18T08:50:41Z

test/powershell/Modules/Microsoft.WSMan.Management/ConfigProvider.Tests.ps1

Maybe use as in line 50 > $null?

I'll change it to $null = for consistency

iSazonov · 2017-09-18T08:54:35Z

test/powershell/Modules/Microsoft.WSMan.Management/ConfigProvider.Tests.ps1

Please place a comment on new line.

I think the request here was to move the comment on line 115 to a new line, not insert a newline character after it.

This isn't a blocking issue for the PR. I'm just clarifying intent.

TravisEz13 · 2017-09-18T16:23:03Z

@adityapatwardhan Can you update your review

adityapatwardhan

LGTM

mirichmo · 2017-09-18T17:20:03Z

Thanks for all the updates @SteveL-MSFT. I'll merge the PR once the tests pass

fixed issues in WSMan Config Provider added tests for get-item, get-childitem, set-item, new-item remove-item, clear-item, dynamic parameters merged CoreClr and FullClr code

address PR feedback

revert to using microsoft.powershell plugin as base for test keeps it simple rather than relying on additional scripts or cmdlets since these tests don't require remoting

address Aditya's feedback

address some cosmetic feedback

based on discussion with Jim, we should skip resx checks on non-Windows

SteveL-MSFT · 2017-09-18T21:10:00Z

Travis-CI failures due to resx tests not finding resources that don't exist there (like cimcmdlets). After discussion with @JamesWTruher we decided that it is ok to skip those tests on non-Windows. Easier to just make that change in this PR than making another PR and pulling it in.

SteveL-MSFT assigned mirichmo Sep 5, 2017

SteveL-MSFT requested review from adityapatwardhan and mirichmo September 5, 2017 20:51

SteveL-MSFT requested review from TravisEz13 and daxian-dbw as code owners September 5, 2017 20:51

msftclas added the cla-not-required label Sep 5, 2017

SteveL-MSFT force-pushed the wsman-config-tests branch 3 times, most recently from 0792984 to 5480257 Compare September 5, 2017 22:42

SteveL-MSFT mentioned this pull request Sep 7, 2017

Add tests for WSMan provider, cmdlets and CredSSP #4158

Closed

iSazonov reviewed Sep 12, 2017

View reviewed changes

mirichmo suggested changes Sep 13, 2017

View reviewed changes

SteveL-MSFT force-pushed the wsman-config-tests branch from 39133cf to 833984d Compare September 14, 2017 04:28

adityapatwardhan requested changes Sep 14, 2017

View reviewed changes

mirichmo reviewed Sep 15, 2017

View reviewed changes

mirichmo approved these changes Sep 15, 2017

View reviewed changes

SteveL-MSFT force-pushed the wsman-config-tests branch 3 times, most recently from b1b9b93 to 25d9d99 Compare September 18, 2017 03:42

iSazonov reviewed Sep 18, 2017

View reviewed changes

adityapatwardhan approved these changes Sep 18, 2017

View reviewed changes

SteveL-MSFT added 3 commits September 18, 2017 14:07

[Feature]

859127e

fixed issues in WSMan Config Provider added tests for get-item, get-childitem, set-item, new-item remove-item, clear-item, dynamic parameters merged CoreClr and FullClr code

[feature]

f54e9aa

address PR feedback

[feature]

9032f6c

revert to using microsoft.powershell plugin as base for test keeps it simple rather than relying on additional scripts or cmdlets since these tests don't require remoting

SteveL-MSFT added 3 commits September 18, 2017 14:07

[feature]

6c5d945

address Aditya's feedback

[feature]

0a3bfdc

address some cosmetic feedback

[feature]

99eef43

based on discussion with Jim, we should skip resx checks on non-Windows

SteveL-MSFT force-pushed the wsman-config-tests branch from 54e71f5 to 99eef43 Compare September 18, 2017 21:08

mirichmo merged commit 2342064 into PowerShell:master Sep 18, 2017

SteveL-MSFT deleted the wsman-config-tests branch September 18, 2017 23:18

daxian-dbw mentioned this pull request Sep 19, 2017

Enable ApartmentState support #4772

Closed

Added WSMan Config provider tests #4756

Added WSMan Config provider tests #4756

Uh oh!

Conversation

SteveL-MSFT commented Sep 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT commented Sep 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov Sep 14, 2017 •

edited

Loading