Some Integration Tests Can Fail When Temporarily Unable to Connect to GitHub

[mhendric]

Details of the scenario you tried and the problem that is occurring

#447 hit an issue where CI failed multiple times for certain Integration tests, as they were temporarily unable to connect to GitHub. I suspect this was a transient issue. We should update the way GitHub is used in tests to make it more resilient, and able to deal with transient failures. Perhaps putting Git related code into a helper function that can retry if necessary would help.

Verbose logs showing the problem

Here's a couple examples of this from different CI runs:

[00:19:41] Executing script C:\projects\xpsdesiredstateconfiguration\Tests\Integration\MSFT_xWindowsPackageCab.Integration.Tests.ps1
[00:19:53]   [-] Error occurred in test script 'C:\projects\xpsdesiredstateconfiguration\Tests\Integration\MSFT_xWindowsPackageCab.Integration.Tests.ps1' 0ms
[00:19:53]     RemoteException: fatal: unable to access 'https://github.com/PowerShell/DscResource.Tests/': Could not resolve host: github.com
[00:19:53]     at Enter-DscResourceTestEnvironment, C:\projects\xpsdesiredstateconfiguration\Tests\CommonTestHelper.psm1: line 748
[00:19:53]     at <ScriptBlock>, C:\projects\xpsdesiredstateconfiguration\Tests\Integration\MSFT_xWindowsPackageCab.Integration.Tests.ps1: line 3
[00:19:53]     at <ScriptBlock>, C:\Program Files\WindowsPowerShell\Modules\Pester\4.6.0\Pester.psm1: line 1095
[00:19:53]     at Invoke-Pester<End>, C:\Program Files\WindowsPowerShell\Modules\Pester\4.6.0\Pester.psm1: line 1121
[00:19:53]     at Invoke-AppveyorTestScriptTask, C:\projects\xpsdesiredstateconfiguration\DscResource.Tests\AppVeyor.psm1: line 792

[00:22:55] Executing script C:\projects\xpsdesiredstateconfiguration\Tests\Integration\MSFT_xWindowsFeature.Integration.Tests.ps1
[00:22:56]   [-] Error occurred in test script 'C:\projects\xpsdesiredstateconfiguration\Tests\Integration\MSFT_xWindowsFeature.Integration.Tests.ps1' 0ms
[00:22:56]     RemoteException: fatal: unable to access 'https://github.com/PowerShell/DscResource.Tests/': OpenSSL SSL_connect: SSL_ERROR_SYSCALL in connection to github.com:443 
[00:22:56]     at Enter-DscResourceTestEnvironment, C:\projects\xpsdesiredstateconfiguration\Tests\CommonTestHelper.psm1: line 748
[00:22:56]     at <ScriptBlock>, C:\projects\xpsdesiredstateconfiguration\Tests\Integration\MSFT_xWindowsFeature.Integration.Tests.ps1: line 16
[00:22:56]     at <ScriptBlock>, C:\Program Files\WindowsPowerShell\Modules\Pester\4.6.0\Pester.psm1: line 1095
[00:22:56]     at Invoke-Pester<End>, C:\Program Files\WindowsPowerShell\Modules\Pester\4.6.0\Pester.psm1: line 1121
[00:22:56]     at Invoke-AppveyorTestScriptTask, C:\projects\xpsdesiredstateconfiguration\DscResource.Tests\AppVeyor.psm1: line 792

Version of the DSC module that was used ('dev' if using current dev branch)

dev

[PlagueHO]

I had this the other day too - across multiple repos (not just this one). I think there was a network blip between AppVeyor and GitHub. But in my case AppVeyor couldn't pull the source code at all:
https://ci.appveyor.com/project/PlagueHO/networkingdsc/builds/21910930

But I agree, a more resilient approach to cloning the DSCResouce.Tests would be good - with maybe a back off and retry x times policy.

[mhendric]

I realized after the fact that this 'git clone' command probably came from the DSC test templates. Do you think we should address this directly in DSCResources\Tests.Template (https://github.com/PowerShell/DscResources/blob/master/Tests.Template)?

[PlagueHO]

Yep, I reckon that would be the best place to do it.

[mhendric]

I did a little more research on this, and discovered a couple oddities.

First, I realized that different tests are trying to connect to GitHub. The code in the latest DSC test templates will only try to do a Git operation if the cloned repo doesn't already exist locally. The fact that this is getting called in potentially every test means we aren't checking to see whether the repos exist first. That could potentially be slowing down our tests significantly too. So if nothing else comes out of this, we may want to update the Git clone section to use the current code from the test templates:

if ( (-not (Test-Path -Path (Join-Path -Path $script:moduleRoot -ChildPath 'DSCResource.Tests'))) -or `
     (-not (Test-Path -Path (Join-Path -Path $script:moduleRoot -ChildPath 'DSCResource.Tests\TestHelper.psm1'))) )
{
    & git @('clone', 'https://github.com/PowerShell/DscResource.Tests.git', (Join-Path -Path $script:moduleRoot -ChildPath 'DscResource.Tests'))
}

Second, it looks like the Git operation is coming from function Enter-DscResourceTestEnvironment in Tests\CommonTestHelper.psm1. It looks like this code needs to be updated (perhaps taking the template code from above) so that we don't do a git pull every time the module is imported.

    $moduleRootPath = Split-Path -Path $PSScriptRoot -Parent
    $dscResourceTestsPath = Join-Path -Path $moduleRootPath -ChildPath 'DSCResource.Tests'
    $testHelperFilePath = Join-Path -Path $dscResourceTestsPath -ChildPath 'TestHelper.psm1'

    if (-not (Test-Path -Path $dscResourceTestsPath))
    {
        Push-Location $moduleRootPath
        git clone 'https://github.com/PowerShell/DscResource.Tests' --quiet
        Pop-Location
    }
    else
    {
        $gitInstalled = $null -ne (Get-Command -Name 'git' -ErrorAction 'SilentlyContinue')

        if ($gitInstalled)
        {
            Push-Location $dscResourceTestsPath
            git pull origin dev --quiet
            Pop-Location
        }
        else
        {
            Write-Verbose -Message 'Git not installed. Leaving current DSCResource.Tests as is.'
        }
    }

So I guess we have two options here. One would be to update the code in Enter-DscResourceTestEnvironment to use the DSC test template code for downloading DSCResources.Tests. The other would be to stop using this helper function and to put the same template code in every file. Any thoughts on which direction is best? I'd lean towards the former option because it would require less code updates, but that option doesn't help us align with the DSC test templates.

And I still think getting an update in the DSC test templates themselves to be able to perform retries would be good, but we can save that for another issue in another repo.

[PlagueHO]

I'm traveling for work this week so won't be able to respond in depth till the weekend. However a bit of context around the git clone behavior. The scenario we were trying to prevent by checking the git repo was up-to-date each time was if the user had an old version in their copy of a module repo and was using it to test against. This was a bit of a problem a year or so back when we were adding improvements to the tests in DSCResource.Tests quite frequently. But I'm sure we could find a little bit smarter way of doing this check that doesn't go out to GH every time. As you day that is slow and fragile.

[johlju]

We should switch to using the full template for both unit and integration test and remove any left over helper functions. These helper functions are probably the old and first version that later became the test framework.
So refactor the tests to use the full template so we can be “compliant” with the test framework and any future changes there. Any resilience that is added should first be added to the test framework if possible to minimize the logic in the header of the test templates.

My thoughts. 😄

[mhendric]

If we go that direction @johlju , do we need helper function Enter-DscResourceTestEnvironment at all? It looks like that calls Initialize-TestEnvironment, but we could just move that directly into the Unit and Integration test files themselves.

[johlju]

I don’t think Enter-DscResourceTestEnvironment is needed. That is already handled in the logic of the test template (see header). Same with exit* function, it can probably be removed from this repo too.

[johlju]

See the header here https://github.com/PowerShell/DscResources/blob/master/Tests.Template/unit_template.ps1

Same for the integration test template.

[PlagueHO]

The only problem with removing Enter-DscResourceTestEnvironment is that if we start increasing the complexity and amount of code in how we clone/pull the DSCResources.TEsts repo, the more code we end up duplicating in every test file.

Making a CommonTestHelper.psm1 file that contains these functions standard in the template might actually be the better approach? And moving all repos to be compliant with that pattern instead of the other way round (backing this out of xPSDesiredStateConfiguration).

[johlju]

Yes, then maybe we could remove code from the header of each unit- and integration test instead of adding more.
Although, not sure we can add code to the unit templates where they are present today (DscResource repo).
In this PowerShell/DscResources#334 (comment) Katie asks not to have code in the DscResources repo. A suggestion in that issue was to add a DscResource.Template repo that this kind of code could exist. I tired to get that repo during my time maintaining Dsc Resource Kit, but there was no one that had the time to create that repo for me. 😕 While I waited I built (not quite finished yet) https://github.com/johlju/DscResource.Template that I would merge into that new empty repo. Maybe we just continue building on that (my) repo and I can give you all permission to it? 🤔 Just a thought.

[PlagueHO]

Let's bring it up at the next community call!