xDscWebService: Implemented fix for issue 191 #462
Conversation
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @tmeckel)
a discussion (no related file):
Need to add a new entry under the Unreleased section of README.md
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 978 at r1 (raw file):
$iisInstallPath = (Get-ItemProperty "HKLM:\SOFTWARE\Microsoft\INetStp" -Name InstallPath).InstallPath
Get-ItemProperty should use Named Parameter (-Path)
Also, you should use single quotes instead of double quotes here.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 983 at r1 (raw file):
$assyPath = Join-Path $iisInstallPath "Microsoft.Web.Administration.dll" -Resolve -ErrorAction:SilentlyContinue
Join-Path should use Named Parameters (-Path and -ChildPath)Also, you should use single quotes instead of double quotes here.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 989 at r1 (raw file):
"Microsoft.Web.Administration.ServerManager"
Use single quotes here
DSCResources/MSFT_xDSCWebService/en-US/MSFT_xDSCWebService.psd1, line 3 at r1 (raw file):
ThrowUseSecurityBestPractice = Error: Cannot use best practice security settings with unencrypted traffic. Please set UseSecurityBestPractices to $false or use a certificate to encrypt pull server traffic. FindCertificateBySubjectMultiple = More than one certificate found with subject containing {0} and using template "{1}".
Recommend updating all of the old entries so that the equals sign aligns with the equals sign of the new entry.
|
Looks like this breaks the following Unit test. An update will be required for the corresponding Unit test: |
|
Hi @mhendric thanks for the review! I fully understand the remark about using single quotes and using explicit parameter names. But I'm not sure about the requirement aligning the '=' signs. Is it only about code style and is it part of the contribution guidelines for this project? And I'll try to setup a VM to reproduce the failed Pester test. And one word about the issue about having a Pull Server configured via this resource when IIS Express is installed aside a IIS: I wasn't simply able to deploy a Pull Server on two machines where this was the case in two different customer environments. Perhaps it's a esoteric setup, because you won't face the situation in a production environment but it's highly likely in development or test enviroments where you would normally test your DSC configuartions. |
I don't think it's an official style guideline. I think it's more of an unspoken rule that in the absence of guidance, follow the existing style (which in this case was with aligned equals signs). You can probably choose to disregard this one, it will just make those of us with OCD cringe a little :) . @johlju , do you think this is a topic worth adding to the DSC Resource Style Guidelines?
Just FYI, this particular Unit Test file (as with most from this module, I suspect) can be run from a non-Web Server. I just tested running the original MSFT_xDSCWebService.Tests.ps1 from a Windows 10 machine without any Web roles, and it worked just fine. |
When I think about it, the test is implemented incorrectly. Because of the other DSC Resources which could be applied on arbitrary computers the Which is used here, where the actual IIS plumbing is done. But that all doesn't explain why the test now fails because I only (funny I know , because every single IT system got broken at least once by a goo intention) changed the loading strategy for the correct IIS Webdministration.dll so that it won't pick the Webadminitration module of IIS Express. This is in fact the main reason for the bug that you can't deploy a pull server on a computer where IIS and IIS Express are installed side by side. The strong name of the IIS Express Webadminstration assembly only differs in it's version number and to make it worse it's higher than the one provided by IIS itself. And because the code is using Assembly::LoadByPartcialName to not having to change the code if the IIS guys change the strong name and that LoadByPartcialName always favors the highest Version for a partially specified Assembly name, you end up with the Webadminstration module of IIS Express and IIS Express does support the concept of Please also refer to #191 for the older discussion about the error. |
|
So, Unit tests should not rely on the results of external function calls, or rely on the machine being in a certain state. Essentially you're just trying to test that the code for a specific function flows properly under expected conditions. You're not trying to test that the function and all it's external calls work. That falls under the scope of Integration tests. The first two problems I see in the 'Update-LocationTagInApplicationHostConfigForAuthentication' tests after this change are that you are not Mock'ing the calls to Get-ItemProperty or Join-Path. Get-ItemProperty could only actually return a value on a real IIS web server, so you should Mock that function to just return a static path (valid or not). Next, you use -Resolve in the Join-Path, so unless the path was actually valid, that will fail. So you should just Mock Join-Path to return a fake string. Here's an example for both of these: Next set of problems are that the old Unit tests had verifiable Mock's for Add-Type and New-Object, but you removed those lines of code, so those verifications are going to fail when those functions aren't attempted to be called. You should remove those Mock statements entirely. Next up, and this is the biggest challenge, I'm not sure that the following lines are Mockable via Pester. I don't think Pester can Mock static class functions. You may need to come up with an alternate way to represent those calls that can be Mock'ed. You may also consider moving those to a dedicated function, and then in Update-LocationTagInApplicationHostConfigForAuthentication you can just Mock that function. But at some point you're going to need to be able to create Unit tests for these lines. For Mock'ing functions of objects (like $webAdminSrvMgr.GetApplicationHostConfiguration()), you'll want to have the base object be a custom PSObject, and then add a ScriptMethod member corresponding to the function. See line 82 of the following code for an example of how I've done this: https://github.com/PowerShell/xExchange/blob/dev/Tests/Unit/MSFT_xExchClientAccessServer.tests.ps1 Let me know if you have any questions on all of this. |
Researched this a bit further. I'm pretty sure you're going to have to move those lines to a dedicated function if we want Update-LocationTagInApplicationHostConfigForAuthentication to continue to be Unit testable. See: pester/Pester#592 |
Thanks @mhendric for the detailed explanation how the tests are structured here! I highly appreciate that! So and because I thought I understood something (LOL) I thought, well the test is now broken because I removed the
So with all what you said about how to setup unit tests and what I found about how the https://github.com/PowerShell/xPSDesiredStateConfiguration/blob/9fd047b90f3d74c4a83e0ae0011da1af5b916bbd/Tests/Unit/MSFT_xDSCWebService.Tests.ps1#L1095 is setup currently, as I said earlier, I personally don't understand how the test could've ever passed sucessfully. So from my point of view I would do two things now
|
I haven't tried to actually debug the old code while running the Unit tests, but my guess is that since Add-Type was being Mock'ed, it didn't actually matter what result, if any, came back from Get-GacAssemblyVersion. That being said, Get-GacAssemblyVersion probably should have been Mock'ed so that Update-LocationTagInApplicationHostConfigForAuthentication wouldn't rely on external calls.
Sounds like a good approach to me. |
…ct of type Microsoft.Web.Administration.ServerManager
…rAuthentication according to the new strategy to get an instance of Microsoft.Web.Administration.ServerManager
Codecov Report
@@ Coverage Diff @@
## dev #462 +/- ##
===================================
- Coverage 72% 72% -1%
===================================
Files 27 27
Lines 4024 4029 +5
Branches 4 4
===================================
- Hits 2922 2919 -3
- Misses 1098 1106 +8
Partials 4 4 |
|
@mhendric I changed the PR according our discussion and the rest of your findings, espcially adding a comment to README.md. The appveyor tests fail because of failed PSScriptAnalyzer rules, but as I can see they don't relate to my code. |
Added -Path parameter to Get-ItemProperty in function Get-IISServerManager
BTW I ran the unit test MSFT_xDSCWebService.Tests.ps1 on my Windows 8.1 box and surely enough the test Update-LocationTagInApplicationHostConfigForAuthentication failed as expected.
|
Hmmmm…. works just fine from my Windows 10 machine: |
Because I changed the implementation and the mock ups as planned I don't this we should waste any time on the old stuff ;-) As you can see in the appveyor results (https://ci.appveyor.com/project/PowerShell/xpsdesiredstateconfiguration/builds/20388250?fullLog=true#L5888) the changed implementation of the unit tests now passes although the module is now using the new strategy for loading the WebAdministration assembly. |
There was a problem hiding this comment.
Reviewed 1 of 4 files at r2.
Reviewable status: 1 of 4 files reviewed, 5 unresolved discussions (waiting on @tmeckel)
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 983 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
$assyPath = Join-Path $iisInstallPath "Microsoft.Web.Administration.dll" -Resolve -ErrorAction:SilentlyContinueJoin-Path should use Named Parameters (-Path and -ChildPath)Also, you should use single quotes instead of double quotes here.
Still need Named Parameters for Join-Path.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 987 at r3 (raw file):
{
Open brace for function should be on new line to align with the rest of the functions in this module. Function also needs comment based help: https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#functions
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 991 at r3 (raw file):
Quoted 8 lines of code…
if (-not $iisInstallPath) { throw ($LocalizedData.IISInstallationPathNotFound) } $assyPath = Join-Path $iisInstallPath 'Microsoft.Web.Administration.dll' -Resolve -ErrorAction:SilentlyContinue if (-not $assyPath) { throw ($LocalizedData.IISWebAdministrationAssemblyNotFound)
I think it may be best to leave these lines in the original function since they are in fact Mockable. That way the only place where we'll have UnMockable and Untestable code will be in Get-IISServerManager. Update-LocationTagInApplicationHostConfigForAuthentication will still be completely Unit testable.
If you did leave this code in here though, you should pass $LocalizedData to this function as a parameter, since it's defined outside of the function. I don't think we want to assume it will always be available in scope.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1003 at r3 (raw file):
}
Shouldn't this function have a return value? Assuming it should, also make sure to add the return type (at a minimum, System.Object, below [CmdletBinding()].
Added parameter -Path and -ChildPath to Join-Path call in Get-IISServerManager Changed brace alignment in Get-IISServerManager
There was a problem hiding this comment.
Reviewable status: 1 of 4 files reviewed, 5 unresolved discussions (waiting on @mhendric and @tmeckel)
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 983 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
Still need Named Parameters for Join-Path.
I added the parameters to Join-Path
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 987 at r3 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
{Open brace for function should be on new line to align with the rest of the functions in this module. Function also needs comment based help: https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#functions
Done
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 991 at r3 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
if (-not $iisInstallPath) { throw ($LocalizedData.IISInstallationPathNotFound) } $assyPath = Join-Path $iisInstallPath 'Microsoft.Web.Administration.dll' -Resolve -ErrorAction:SilentlyContinue if (-not $assyPath) { throw ($LocalizedData.IISWebAdministrationAssemblyNotFound)I think it may be best to leave these lines in the original function since they are in fact Mockable. That way the only place where we'll have UnMockable and Untestable code will be in Get-IISServerManager. Update-LocationTagInApplicationHostConfigForAuthentication will still be completely Unit testable.
If you did leave this code in here though, you should pass $LocalizedData to this function as a parameter, since it's defined outside of the function. I don't think we want to assume it will always be available in scope.
I added the output type declaration to Get-IISServerManager and I would prefer to leave the code for loading the ServerManager object in this function because it's separately mockable and monitorable that it's called by Update-LocationTagInApplicationHostConfigForAuthentication.
Furthermore I wouldn't add a parameter for the $LocalizedData now because as with Find-CertificateThumbprintWithSubjectAndTemplateName the $LocalizedData is initialized "globally" (i.e. in the scope of the PSM1 file) and is available for all functions therein.
Perhaps this isn't ideal because such supports functions should be placed in a different PS1 oder PSM1 file, but the code works right now and we should move all support functions at once and doing it like patchwork.
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 1003 at r3 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
}Shouldn't this function have a return value? Assuming it should, also make sure to add the return type (at a minimum, System.Object, below [CmdletBinding()].
A Powershell function doesn't need an explicit return statement because all values that are put into the pipeline will be returned from the function. The return statement terminates the current scope though. So at the end of a function the return statement is redundant thus I always omit it. But I've to admit that it's more readable to have a return statement, so I added it.
There was a problem hiding this comment.
Reviewable status: 1 of 4 files reviewed, 5 unresolved discussions (waiting on @mhendric)
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 978 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
$iisInstallPath = (Get-ItemProperty "HKLM:\SOFTWARE\Microsoft\INetStp" -Name InstallPath).InstallPathGet-ItemProperty should use Named Parameter (-Path)
Also, you should use single quotes instead of double quotes here.
Done
DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 989 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
"Microsoft.Web.Administration.ServerManager"Use single quotes here
Done
DSCResources/MSFT_xDSCWebService/en-US/MSFT_xDSCWebService.psd1, line 3 at r1 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
ThrowUseSecurityBestPractice = Error: Cannot use best practice security settings with unencrypted traffic. Please set UseSecurityBestPractices to $false or use a certificate to encrypt pull server traffic. FindCertificateBySubjectMultiple = More than one certificate found with subject containing {0} and using template "{1}".Recommend updating all of the old entries so that the equals sign aligns with the equals sign of the new entry.
Done.
There was a problem hiding this comment.
Reviewed 2 of 4 files at r2, 1 of 1 files at r4.
Reviewable status:complete! all files reviewed, all discussions resolved
|
@tmeckel , everything looks good to me, good work! Now if we can only find a Maintainer to check this in... In the meantime, have you tested this code for real to make sure it works on an actual IIS server? |
The situation with having PRs merged and Issues reviewed is definitely a problem with this DSC module and sadly to say with the PSDscResources module also. As you can see I submitted a PR for a nasty bug in the Group resource in PSDscResources PowerShell/PSDscResources#118 (which can be found here too #467 because the code base of these two modules are merely the same) but the PR is marked as abandoned. @johlju was kind enough to discuss how to setup the integration tests for the solution but I think he has more important things to do than reviewing an merging PRs and he's already the owner of the SQL DSC module. I personally submitted some PRs and issues with SharePoint Powershell PNP and it's core library project and there everything worked like a charm. So with the DSC modules it's kinda disappointing. I know every developer thinks he's the hub of the world, but why e.g. was a bot used here that puts issues and PRs on stale and abandoned when there's a lack of time to work on those things? And we do have serious issues here inside the DSC modules which render them unusable in some conditions or operational scenarios. Don't get me wrong I'm very sure that every single person here does a great job. I simply think there's too much work put on too little shoulders. So I definitely endorse your issue #466!
Yes I tested the new code, and all the other fixes for which I submitted PRs, in out test environment, which includes servers with IIS Express and IIS installed side by side and plain vanilla servers, because the fixes are required to have a DSC Pull Server deployed in our production environment and passing our quality tests. |
|
Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again. |
|
We should be able to pick this one up again once I've got all the integration tests passing again (and we've got a maintainer with merge permissions). |
|
I updated the branch on this one too and the tests are now running. Fingers crossed we can just get the review done 😁 |
There was a problem hiding this comment.
Reviewed 2 of 2 files at r5.
Reviewable status:
mhendric
added
ready for merge
Pull Request (PR) description
This Pull Request (PR) implements a fix for issue #191. Although this issue has been closed, I wasn't able to deploy a DSC Pull Server on a server where IIS and IIS Express are installed in parallel. The following errors are shown in while applying the configuration to the computer
The operating system the target node is running
Version and build of PowerShell the target node is running
Version of the DSC module that was used ('dev' if using current dev branch)
This Pull Request (PR) fixes the following issues
Fixes #191
Task list
Entry should say what was changed, and how that affects users (if applicable).
and comment-based help.
This change is