MSFT_MsiPackage: Fix HttpListener Issues with MsiPackage Integration Tests #157
Conversation
|
Alright @PlagueHO , I think this is the one. Mind reviewing this one? Thanks! |
There was a problem hiding this comment.
Great stuff @mhendric! Will be awesome to get this in!
Reviewed 7 of 7 files at r1.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @mhendric)
Tests/Integration/MSFT_MsiPackage.EndToEnd.Tests.ps1, line 416 at r1 (raw file):
$serverResult = Start-Server -FilePath $script:msiLocation -LogPath $script:logFile -Https $false -HttpPort $script:testHttpPort -HttpsPort $script:testHttpsPort $fileServerStarted = $serverResult.FileServerStarted $job = $serverResult.Job
Nitpick: Can you trim white space from this line? Suggest enabling this setting in VS code (even set it for the project workspace so it always corrects it):
Tests/Integration/MSFT_MsiPackage.EndToEnd.Tests.ps1, line 479 at r1 (raw file):
Test-PackageInstalledById -ProductId $script:packageId | Should Be $true }
Nitpick: Can you trim white space from this line?
Tests/Integration/MSFT_MsiPackage.EndToEnd.Tests.ps1, line 550 at r1 (raw file):
Test-PackageInstalledById -ProductId $script:packageId | Should Be $false }
Nitpick: Can you trim white space from this line?
Tests/Integration/MSFT_MsiPackage.Integration.Tests.ps1, line 177 at r1 (raw file):
It 'Should correctly install and remove a package from a HTTP URL' { $baseUrl = "http://localhost:$script:testHttpPort/"
Suggest using [System.UriBuilder] to assemble URIs:
$uriBuilder = [System.UriBuilder]::new('http','localhost',$script:testHttpPort)
$uriBuilder.Path = 'package.msi'
$msiUrl = $uriBuilder.Uri.AbsoluteUriTests/Integration/MSFT_MsiPackage.Integration.Tests.ps1, line 223 at r1 (raw file):
It 'Should correctly install and remove a package from a HTTPS URL' -Skip:$script:skipHttpsTest { $baseUrl = "https://localhost:$script:testHttpsPort/"
Suggest using [System.UriBuilder] to assemble URIs:
$uriBuilder = [System.UriBuilder]::new('http','localhost',$script:testHttpsPort)
$uriBuilder.Path = 'package.msi'
$msiUrl = $uriBuilder.Uri.AbsoluteUriTests/Integration/MSFT_MsiPackage.Integration.Tests.ps1, line 238 at r1 (raw file):
$serverResult = Start-Server -FilePath $script:msiLocation -LogPath $script:logFile -Https $true -HttpPort $script:testHttpPort -HttpsPort $script:testHttpsPort $fileServerStarted = $serverResult.FileServerStarted $job = $serverResult.Job
Trim trailing whitespace.
Tests/TestHelpers/CommonTestHelper.psm1, line 840 at r1 (raw file):
[System.UInt16] $unusedPort = 0 $usedPorts = (Get-NetTCPConnection).LocalPort | Where-Object -FilterScript {$_ -ge $LowestPortNumber -and $_ -le $HighestPortNumber}
Maybe split line so it isn't so long:
$usedPorts = (Get-NetTCPConnection).LocalPort | Where-Object -FilterScript {
$_ -ge $LowestPortNumber -and $_ -le $HighestPortNumber
}Tests/TestHelpers/CommonTestHelper.psm1, line 842 at r1 (raw file):
$usedPorts = (Get-NetTCPConnection).LocalPort | Where-Object -FilterScript {$_ -ge $LowestPortNumber -and $_ -le $HighestPortNumber} if (!(Test-Path -Path variable:usedPorts) -or ($usedPorts -eq $null))
Put $null on left hand side of comparison.
Tests/TestHelpers/CommonTestHelper.psm1, line 847 at r1 (raw file):
} if (!(Test-Path -Path variable:ExcludePorts) -or ($ExcludePorts -eq $null))
Put $null on left hand side of comparison.
Tests/TestHelpers/CommonTestHelper.psm1, line 852 at r1 (raw file):
} for ($i = $LowestPortNumber; $i -le $HighestPortNumber; $i++)
FYI, You could convert this to a foreach loop which may increase readability:
foreach ($port in $LowestPortNumber..$HighestPortNumber)Tests/TestHelpers/CommonTestHelper.psm1, line 854 at r1 (raw file):
for ($i = $LowestPortNumber; $i -le $HighestPortNumber; $i++) { if (!($usedPorts.Contains($i)) -and !($ExcludePorts.Contains($i)))
FYI, to optimize this you could Append the $ExcludedPorts to the $usedPorts prior to entering this loop. This would require casting the $usedPorts to an [System.Collection.ArrayList] and then using $null = $usedPorts.Add($ExcludedPorts).
If you did this, I'd suggest naming the variable $UsedAndExcludedPorts for clarity.
Tests/TestHelpers/MSFT_MsiPackageResource.TestHelper.psm1, line 216 at r1 (raw file):
Write-Log -LogFile $LogPath -Message 'Finished importing certificate into root. About to bind it to port.'
Trim white space 😁
Tests/TestHelpers/MSFT_MsiPackageResource.TestHelper.psm1, line 498 at r1 (raw file):
'Running Process Info' >> $LogPath Get-Process | fl | Out-String >> $LogPath
Use Format-List
Tests/TestHelpers/MSFT_MsiPackageResource.TestHelper.psm1, line 501 at r1 (raw file):
'Open TCP Connections Info' >> $LogPath Get-NetTCPConnection | fl | Out-String >> $LogPath
Use Format-List
Tests/TestHelpers/MSFT_MsiPackageResource.TestHelper.psm1, line 511 at r1 (raw file):
$fileServerStarted.Dispose() }
Trim whitespace 😁
Tests/TestHelpers/MSFT_MsiPackageResource.TestHelper.psm1, line 596 at r1 (raw file):
{ [CmdletBinding()] param
You can use param () here
Tests/Unit/MSFT_MsiPackage.Tests.ps1, line 216 at r1 (raw file):
Context 'Uri scheme is Http and installation succeeds' { $mocksCalled = @(
FYI, this is just a complete informational thing only (not something we can fix right now), but the fact that we have to mock so many things in a single test indicates that the function under test is doing too many things and should be broken into smaller component functions. I think we'd need to do major refactoring to break it up - so a job for another year 😁
Codecov Report
@@ Coverage Diff @@
## dev #157 +/- ##
===================================
Coverage 83% 83%
===================================
Files 19 19
Lines 2760 2760
Branches 4 4
===================================
Hits 2305 2305
Misses 451 451
Partials 4 4 |
There was a problem hiding this comment.
Thanks @PlagueHO ! Let me know if anything else is needed for this.
Reviewable status: 2 of 7 files reviewed, 14 unresolved discussions (waiting on @PlagueHO)
Tests/Integration/MSFT_MsiPackage.EndToEnd.Tests.ps1, line 416 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Nitpick: Can you trim white space from this line? Suggest enabling this setting in VS code (even set it for the project workspace so it always corrects it):
Done.
Tests/Integration/MSFT_MsiPackage.EndToEnd.Tests.ps1, line 479 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Nitpick: Can you trim white space from this line?
Done.
Tests/Integration/MSFT_MsiPackage.EndToEnd.Tests.ps1, line 550 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Nitpick: Can you trim white space from this line?
Done.
Tests/Integration/MSFT_MsiPackage.Integration.Tests.ps1, line 177 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Suggest using [System.UriBuilder] to assemble URIs:
$uriBuilder = [System.UriBuilder]::new('http','localhost',$script:testHttpPort) $uriBuilder.Path = 'package.msi' $msiUrl = $uriBuilder.Uri.AbsoluteUri
Done.
Tests/Integration/MSFT_MsiPackage.Integration.Tests.ps1, line 223 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Suggest using [System.UriBuilder] to assemble URIs:
$uriBuilder = [System.UriBuilder]::new('http','localhost',$script:testHttpsPort) $uriBuilder.Path = 'package.msi' $msiUrl = $uriBuilder.Uri.AbsoluteUri
Done.
Tests/Integration/MSFT_MsiPackage.Integration.Tests.ps1, line 238 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Trim trailing whitespace.
Done.
Tests/TestHelpers/CommonTestHelper.psm1, line 840 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Maybe split line so it isn't so long:
$usedPorts = (Get-NetTCPConnection).LocalPort | Where-Object -FilterScript { $_ -ge $LowestPortNumber -and $_ -le $HighestPortNumber }
Done.
Tests/TestHelpers/CommonTestHelper.psm1, line 842 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Put $null on left hand side of comparison.
Done.
Tests/TestHelpers/CommonTestHelper.psm1, line 847 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Put $null on left hand side of comparison.
Done.
Tests/TestHelpers/MSFT_MsiPackageResource.TestHelper.psm1, line 216 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Trim white space 😁
Done.
Tests/TestHelpers/MSFT_MsiPackageResource.TestHelper.psm1, line 498 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Use
Format-List
Done.
Tests/TestHelpers/MSFT_MsiPackageResource.TestHelper.psm1, line 501 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Use
Format-List
Done.
Tests/TestHelpers/MSFT_MsiPackageResource.TestHelper.psm1, line 511 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Trim whitespace 😁
Done.
Tests/TestHelpers/MSFT_MsiPackageResource.TestHelper.psm1, line 596 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
You can use
param ()here
Done.
There was a problem hiding this comment.
Just two tiny nitpicks left. Sorry @mhendric 😁 I can be so OCD 😢
Reviewed 4 of 5 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mhendric)
Tests/Integration/MSFT_MsiPackage.Integration.Tests.ps1, line 178 at r3 (raw file):
It 'Should correctly install and remove a package from a HTTP URL' { $uriBuilder = [System.UriBuilder]::new('http', 'localhost', $script:testHttpPort) $baseUrl = $uriBuilder.Uri.AbsoluteUri
I don't think this line is required.
Tests/Integration/MSFT_MsiPackage.Integration.Tests.ps1, line 226 at r3 (raw file):
It 'Should correctly install and remove a package from a HTTPS URL' -Skip:$script:skipHttpsTest { $uriBuilder = [System.UriBuilder]::new('https', 'localhost', $script:testHttpsPort) $baseUrl = $uriBuilder.Uri.AbsoluteUri
I don't think this line is required.
There was a problem hiding this comment.
No worries. I've been known to be a bit picky myself :)
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @PlagueHO)
Tests/Integration/MSFT_MsiPackage.Integration.Tests.ps1, line 178 at r3 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
I don't think this line is required.
Are you referring to the $baseUrl assignment? $baseUrl is used a little further below:
{ Set-TargetResource -Ensure 'Present' -Path $baseUrl -ProductId $script:packageId } | Should Throw
Tests/Integration/MSFT_MsiPackage.Integration.Tests.ps1, line 226 at r3 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
I don't think this line is required.
See above comment
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved
Tests/Integration/MSFT_MsiPackage.Integration.Tests.ps1, line 178 at r3 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
Are you referring to the $baseUrl assignment? $baseUrl is used a little further below:
{ Set-TargetResource -Ensure 'Present' -Path $baseUrl -ProductId $script:packageId } | Should Throw
You're right - sorry, I couldn't find it, but it was late. Ignore this.
Tests/Integration/MSFT_MsiPackage.Integration.Tests.ps1, line 226 at r3 (raw file):
Previously, mhendric (Mike Hendrickson) wrote…
See above comment
You're right - sorry, I couldn't find it, but it was late. Ignore this.
Pull Request (PR) description
fails to start. Moves the test HttpListener objects to dynamically assigned,
higher numbered ports to avoid conflicts with other services, and also checks
to ensure that the ports are available before using them. Adds checks to
ensure that no outstanding HTTP server jobs are running before attempting to
setup a new one. Also adds additional instrumentation to make it easier to
troubleshoot issues with the test HttpListener objects in the future.
This Pull Request (PR) fixes the following issues
Task list
Entry should say what was changed, and how that affects users (if applicable).
and comment-based help.
This change is