xPSDesiredStateConfiguration: Multiple AppVeyor jobs #509

johlju · 2019-01-28T18:57:05Z

Pull Request (PR) description

Changes to xPSDesiredStateConfiguration
- In AppVeyor CI the tests are split into three separate jobs, and also
  running tests on two different build worker images (Windows Server
  2012R2 and Windows Server 2016). The common tests are run on only
  one of the build worker images (Windows Server 2012R2). Helps with
  issue #477.

@mhendric @PlagueHO I send in this PR if this is of any interest. If not then please close it.

This Pull Request (PR) fixes the following issues

Helps with issue #477.
Closes #403

Task list

Added an entry under the Unreleased section of the change log in the README.md.
Entry should say what was changed, and how that affects users (if applicable).
Resource documentation added/updated in README.md.
Resource parameter descriptions added/updated in README.md, schema.mof
and comment-based help.
Comment-based help added/updated.
Localization strings added/updated in all localization files as appropriate.
Examples appropriately added/updated.
Unit tests added/updated. See DSC Resource Testing Guidelines.
Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

This change is

johlju · 2019-01-28T19:04:48Z

I will rebase this as soon as PR #510 is merged (can't checkout dev correctly until then).

- In AppVeyor CI the tests are split into three separate jobs, and also running tests on two different build worker images (Windows Server 2012R2 and Windows Server 2016). The common tests are run on only one of the build worker images (Windows Server 2012R2). Helps with issue dsccommunity#477.

johlju · 2019-01-28T19:13:59Z

This one is ready for review now. It was my local repository folder that had a problem. All fixed, and I was able to rebase this PR.

mhendric

Reviewable status: 0 of 43 files reviewed, 1 unresolved discussion (waiting on @johlju)

Tests/Integration/MSFT_xArchive.EndToEnd.Tests.ps1, line 4 at r1 (raw file):

Quoted 5 lines of code…

if ($env:APPVEYOR -eq $true -and $env:CONFIGURATION -ne 'Integration')
{
    Write-Verbose -Message 'Integration test for will be skipped unless $env:CONFIGURATION is set to ''Integration''.' -Verbose
    return
}

Hey @johlju , what do you think about moving the common code that you added to all the test files into a helper function that returns true or false? Then you could shorten the line in each test file to something like: if (HELPERFUNCTION) { return }

Throughout

codecov-io · 2019-01-28T19:47:18Z

Codecov Report

Merging #509 into dev will increase coverage by 2%.
The diff coverage is n/a.

@@         Coverage Diff         @@
##            dev   #509   +/-   ##
===================================
+ Coverage    72%    74%   +2%     
===================================
  Files        27     27           
  Lines      4031   4031           
  Branches      4      4           
===================================
+ Hits       2922   3005   +83     
+ Misses     1105   1022   -83     
  Partials      4      4

johlju

Reviewable status: 0 of 43 files reviewed, 1 unresolved discussion (waiting on @mhendric)

Tests/Integration/MSFT_xArchive.EndToEnd.Tests.ps1, line 4 at r1 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

if ($env:APPVEYOR -eq $true -and $env:CONFIGURATION -ne 'Integration')
{
    Write-Verbose -Message 'Integration test for will be skipped unless $env:CONFIGURATION is set to ''Integration''.' -Verbose
    return
}
Hey @johlju , what do you think about moving the common code that you added to all the test files into a helper function that returns true or false? Then you could shorten the line in each test file to something like: if (HELPERFUNCTION) { return }

Throughout

Done. If the tests fail after this change I will fix them tomorrow.

mhendric

Reviewable status: 0 of 43 files reviewed, 2 unresolved discussions (waiting on @johlju)

Tests/CommonTestHelper.psm1, line 801 at r3 (raw file):

    [CmdletBinding()]
    param

Probably need an output type here. I envision the script analyzer complaining about this...

Tests/CommonTestHelper.psm1, line 803 at r3 (raw file):

Quoted 4 lines of code…

        [Parameter(Mandatory = $true)]
        [ValidateNotNullOrEmpty()]
        [System.String]
        $Name,

Wondering if we should remove the Name parameter, and just obtain the caller using Get-PSCallStack. That would simplify the calls to the helper function even further (and make it easier to do bulk replaces later if we ever have to). We could also make it an optional parameter, and if nothing is passed, just obtain the caller through one of the below options. What do you think?

(Get-PSCallStack)[1].ScriptName.Split('')[-1]

or

(Get-PSCallStack)[1].FunctionName

johlju

Reviewable status: 0 of 43 files reviewed, 2 unresolved discussions (waiting on @mhendric)

Tests/CommonTestHelper.psm1, line 801 at r3 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

    [CmdletBinding()]
    param
Probably need an output type here. I envision the script analyzer complaining about this...

Done.

Tests/CommonTestHelper.psm1, line 803 at r3 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

        [Parameter(Mandatory = $true)]
        [ValidateNotNullOrEmpty()]
        [System.String]
        $Name,
Wondering if we should remove the Name parameter, and just obtain the caller using Get-PSCallStack. That would simplify the calls to the helper function even further (and make it easier to do bulk replaces later if we ever have to). We could also make it an optional parameter, and if nothing is passed, just obtain the caller through one of the below options. What do you think?

(Get-PSCallStack)[1].ScriptName.Split('')[-1]

or

(Get-PSCallStack)[1].FunctionName

Done. I have to walk to work now, so I hope this works. But theoretically this should work. 😄

johlju · 2019-01-29T09:13:22Z

I meant theoretically the change I did should work 😄

johlju · 2019-01-29T09:14:16Z

Yep, it did work! Yay! https://ci.appveyor.com/project/PowerShell/xpsdesiredstateconfiguration/builds/21962178/job/rk7d44iucc30j005#L65

johlju · 2019-01-29T19:12:25Z

@mhendric Let me know if you find more improvements that can be made. 😄

mhendric · 2019-01-29T19:57:46Z

Hey @johlju , sorry, haven't had a chance to dig into this yet today, but will do another CR later. It probably wouldn't hurt to have another set of eyes on this though, especially with the YAML (@PlagueHO ?).

And just for my clarity, what's the expected behavior here when you run this against a free AppVeyor account. Would it still run all 5 jobs (Meta + 2x(Integration + Unit)), or would it only run against the preferred image (thus reducing to 3 jobs)? I think we opted to delay adding multiple images via #403 until we can reduce test times (#477). If the same concern still exists, I'm wondering if for now we should take everything you have here except the multiple image part, and then add the multiple images at a later date. What do you think?

johlju · 2019-01-29T20:04:24Z

No worries. No hurry at all.

On a free account it will run all 5 jobs sequentially. Time wise it is not a problem because each individual job is allowed to run for 60 minutes.
Also, we want to run the test on both images for two free account as well.

I have no problem leaving this as is until the tests have been refactored. But having both images while refactoring the test will make sure the resources still work on both images during refactoring.

Anyways, let me know what you decide. I change to what ever you choose 😄

mhendric · 2019-01-29T20:12:52Z

I think I'm probably OK with adding multiple images now. Nothing like feeling the pain (long overall build time on free accounts in this case) to motivate people to fix stuff, right? Let's let @PlagueHO weigh in on this though.

mhendric

Reviewed 2 of 43 files at r1, 2 of 41 files at r2, 37 of 39 files at r4.
Reviewable status: 41 of 43 files reviewed, 3 unresolved discussions (waiting on @johlju and @PlagueHO)

Tests/CommonTestHelper.psm1, line 826 at r4 (raw file):

        Write-Verbose -Message ('{1} tests for {0} will be skipped unless $env:CONFIGURATION is set to ''{1}''.' -f $Name, $Type) -Verbose

I don't think it should be a blocker since there isn't already a localized string file, but if there was, this perhaps should go in it.

Tests/Integration/MSFT_xWindowsOptionalFeature.Integration.Tests.ps1, line 1 at r4 (raw file):

Quoted 5 lines of code…

if ($env:APPVEYOR -eq $true -and $env:CONFIGURATION -ne 'Integration')
{
    Write-Verbose -Message 'Integration test for will be skipped unless $env:CONFIGURATION is set to ''Integration''.' -Verbose
    return
}

Missed removal of this block...

Tests/Integration/MSFT_xWindowsPackageCab.Integration.Tests.ps1, line 1 at r4 (raw file):

Quoted 5 lines of code…

if ($env:APPVEYOR -eq $true -and $env:CONFIGURATION -ne 'Integration')
{
    Write-Verbose -Message 'Integration test for will be skipped unless $env:CONFIGURATION is set to ''Integration''.' -Verbose
    return
}

Missed removal...

johlju · 2019-01-29T20:59:57Z

Tests/CommonTestHelper.psm1, line 826 at r4 (raw file):

    if ($env:APPVEYOR -eq $true -and $env:CONFIGURATION -ne $Type)
    {
        Write-Verbose -Message ('{1} tests for {0} will be skipped unless $env:CONFIGURATION is set to ''{1}''.' -f $Name, $Type) -Verbose

The message says “...test for ...”, should it say “...tests in ...” instead?

johlju · 2019-01-29T21:04:35Z

My fingers itched to fix the tests to the correct template when I added the code snippet to each file. I saw files using different variations too. And importing a module in the beforeeach-block. Definitely room for improvements in these tests, and surely if it takes time running tests maybe there are more contributors wanting to help resolve that :)

mhendric

Reviewable status: 41 of 43 files reviewed, 4 unresolved discussions (waiting on @johlju and @PlagueHO)

Tests/CommonTestHelper.psm1, line 826 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

The message says “...test for ...”, should it say “...tests in ...” instead?

Good catch. I think 'tests in' reads better.

johlju · 2019-01-29T21:05:50Z

I will fix the review comments tomorrow morning. :)

johlju

Reviewable status: 40 of 43 files reviewed, 3 unresolved discussions (waiting on @mhendric and @PlagueHO)

Tests/CommonTestHelper.psm1, line 826 at r4 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

        Write-Verbose -Message ('{1} tests for {0} will be skipped unless $env:CONFIGURATION is set to ''{1}''.' -f $Name, $Type) -Verbose
I don't think it should be a blocker since there isn't already a localized string file, but if there was, this perhaps should go in it.

Done. I don't think it should be a blocker to, since the helper function will only report this if run in AppVeyor CI. Agree it could be localized, but maybe we can do that after the tests have been refactored any maybe moved this helper module file to a separate folder that would more naturally contain localization folders?

Tests/Integration/MSFT_xWindowsOptionalFeature.Integration.Tests.ps1, line 1 at r4 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

if ($env:APPVEYOR -eq $true -and $env:CONFIGURATION -ne 'Integration')
{
    Write-Verbose -Message 'Integration test for will be skipped unless $env:CONFIGURATION is set to ''Integration''.' -Verbose
    return
}

Missed removal of this block...

Done.

Tests/Integration/MSFT_xWindowsPackageCab.Integration.Tests.ps1, line 1 at r4 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

if ($env:APPVEYOR -eq $true -and $env:CONFIGURATION -ne 'Integration')
{
    Write-Verbose -Message 'Integration test for will be skipped unless $env:CONFIGURATION is set to ''Integration''.' -Verbose
    return
}

Missed removal...

Done.

johlju · 2019-01-30T07:00:21Z

@mhendric @PlagueHO There is only one potential problem with this PR that I see now, and this is that both the jobs that runs the unit tests, both will upload the Codcov result. They should report up the same information, so might be a non-issue.
To solve it there is two options.

Exclude the unit tests from one of the images. I rather not like this idea.

Add a rule to the matrix that when running the configuration 'Unit' on one of the images, the parameter CodeCovIo of the function Invoke-AppveyorTestScriptTask is skipped in that job. Though, not sure it is possible to make this rule, so might need to ask the AppVeyor team. See example below.

-
  matrix:
    only:
      - configuration: Unit
      - image: Visual Studio 2015
  environment:
    SkipAllCommonTests: True

  test_script:
    - ps: |
        Invoke-AppveyorTestScriptTask `
            -Type 'Default' `
            -CodeCoverage `
            -CodeCovIo `
            -ExcludeTag @()

-
  matrix:
    only:
      - configuration: Unit
      - image: Visual Studio 2017
  environment:
    SkipAllCommonTests: True

  test_script:
    - ps: |
        Invoke-AppveyorTestScriptTask `
            -Type 'Default' `
            -CodeCoverage `
            -ExcludeTag @()

See AppVeyor documentation around Specializing matrix job configuration.

I will test this out and report back.

johlju · 2019-01-30T15:57:39Z

@mhendric @PlagueHO I have fixed the above. Codecov is now only reported by one of the jobs. Also added comments to the appveyor.yml to explain a bit more of what the different parts mean.

mhendric · 2019-02-01T15:17:05Z

@johlju , first, sorry for being all indecisive on this one. But after working on #515 and utilizing my free AppVeyor account to check for errors, I'm totally OK with adding multiple images. When I utilize my free account, I'm generally checking to see if I hit any build/test errors, or if the output is what I expected it to be. If I do see something unexpected, I'll go try to fix it, and submit my changes, and won't wait for the current build to finish. Even if the jobs were to run sequentially, I'd have a pretty good idea after the first set of Unit and Integration tests whether the overall build is going to run as expected without having to wait for the second set of Unit and Integration tests to finish. All that being said, my vote is to add the multiple images.

On the topic of multiple images though, I wonder if the newest image should be executed first (when running sequentially)? I'm not sure what's more common these days (2012 R2 or 2016), but I think it would make sense to have the more common build run first.

johlju · 2019-02-01T15:34:10Z

No worries. Sorry I did not have time to answer you until now, been a busy week. I think it is good that we test on multiple images, there might be scenarios that we don't think of until the integration tests run on a specific OS. As you say, there is no need to wait for the full test suit to run finished before sending in a PR. If there is errors after sending in a PR those are probably pretty minor so a few additional commits is not a problem (IMHO). If there are more wpork it as easy closing the PR, and continue working "offline", and then reopen the PR and all the new commits will be added automatically. Although, rebasing should only been done while the PR is open, otherwise the PR "breaks" and cannot reopen.

johlju · 2019-02-01T15:34:40Z

I will put the latest OS to run first. Sounds like a good plan.

mhendric

Reviewed 3 of 3 files at r5, 1 of 1 files at r7.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @PlagueHO)

PlagueHO · 2019-02-01T20:12:44Z

I'll try and get onto a bunch of this stuff tomorrow.

PlagueHO

Great stuff here @johlju!!!! Awesome!

When I was reviewing this, I noticed there are many different format for the initialization blocks used at the top of each test. It would make it a little bit easier to review and maintain if we picked one and standardized them. Why don't we also move to using the standard test template headers where possible?

What do you think @mhendric, @johlju?

Reviewed 1 of 43 files at r1, 2 of 41 files at r2, 36 of 39 files at r4, 3 of 3 files at r5, 1 of 1 files at r7.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @johlju)

appveyor.yml, line 134 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

# Runs for all jobs, but only when test step is successful.
deploy_script:
  - ps: |
        Invoke-AppVeyorDeployTask
I think this should not run for all jobs? What job do we want to run this step?

The Meta job since it is the quickest to finish.

The last job, and enable fast_finish: true on the job matrix to stop AppVeyor build as soon as an error occurs (meaning no Deploy if any tests fail on merge to master branch)

I suggest alternative 1 since we should have made sure everything will test successfully on the dev branch.

I'm thinking perhaps 2, because although we thing everything should always be working in Dev, it doesn't always work out 😁 (E.g. see 8.5.0.0 release 😆 ). So I'd rather we ensure it only kicks once all other jobs have successfully run.

README.md, line 719 at r7 (raw file):

* Changes to xPSDesiredStateConfiguration
  * In AppVeyor CI the tests are split into three separate jobs, and also
    running tests on two different build worker images (Windows Server

running -> run

README.md, line 720 at r7 (raw file):

  * In AppVeyor CI the tests are split into three separate jobs, and also
    running tests on two different build worker images (Windows Server
    2012R2 and Windows Server 2016). The common tests are run on only

Perhaps:
The common tests are run on only one of the build worker images (Windows Server 2012R2)

To:
The common tests are only run on the Windows Server 2012R2 build worker image.

Tests/CommonTestHelper.psm1, line 805 at r7 (raw file):

        Type of tests in the test file. Can be set to Unit or Integration.
#>
function Test-SkipCi

Could we use a more expressive name for this function? E.g.

Test-SkipContinuousIntegrationTask -Type 'Unit'

Tests/Integration/MSFT_xEnvironmentResource.Integration.Tests.ps1, line 6 at r7 (raw file):

Set-StrictMode -Version 'Latest'

Need to remove extra line here.

Tests/Integration/MSFT_xRemoteFile.Tests.ps1, line 1 at r7 (raw file):

Import-Module -Name (Join-Path -Path (Split-Path $PSScriptRoot -Parent) `

It seems there are several different code snippets that are being used to import the CommonTestHelper module - are we able to standardize?

Tests/Unit/MSFT_xRemoteFile.Tests.ps1, line 5 at r7 (raw file):

Import-Module -Name $commonTestHelperFilePath

$Global:DSCModuleName      = 'xPSDesiredStateConfiguration'

FYI: I think we're dealing with this in another issue, but we shouldn't have Global variables here. Should be script.

Tests/Unit/ResourceSetHelper.Tests.ps1, line 10 at r7 (raw file):

                               -ChildPath 'CommonTestHelper.psm1')

if ((Test-SkipCi -Type 'Unit'))

Would this work instead?

if (Test-SkipCi -Type 'Unit')

Also, should there be a comment here to clarify why we're doing this? We could also use a function name that is more expressive:
E.g.

Test-SkipContinuousIntegrationTask -Type 'Unit'

And throughout.

johlju · 2019-02-02T09:13:31Z

@PlagueHO I have done the requested changes, except change the script to load the module the same way. I can't publish my Reviewable comments for some reason, see isssue Reviewable/Reviewable#613.

Hers is my comments:

Please retract the comments if I haven't been able to publish the comments until next time you review.

PlagueHO

Oh wow - strange! Checking to see if I can publish 😁

Reviewable status: 0 of 43 files reviewed, 7 unresolved discussions (waiting on @johlju, @mhendric, and @PlagueHO)

PlagueHO

I'll finish up review tomorrow as it is getting a bit late now.

Reviewable status: 0 of 43 files reviewed, 7 unresolved discussions (waiting on @johlju, @mhendric, and @PlagueHO)

johlju · 2019-02-02T09:26:19Z

@PlagueHO No problem. See you can publish from Reviewable so it's me only.

johlju · 2019-02-02T10:19:51Z

@PlagueHO I reverted the change to MSFT_xRemoteFile.Tests.ps1 where I replace $global with $script. It takes more changes to that test file than is the scope for this PR. Let's do that in another PR. 😄

johlju · 2019-02-02T15:57:42Z

Now it deploys on the job number that is set in appveyor.yml.
Job 1-4 do not deploy: https://ci.appveyor.com/project/johlju/xpsdesiredstateconfiguration/builds/22075321/job/l6ix4xh03su3b8ol#L2951
Job 5 deploy: https://ci.appveyor.com/project/johlju/xpsdesiredstateconfiguration/builds/22075321/job/en2b9a2t789x0s6c#L2657

johlju

Reviewable status: 0 of 43 files reviewed, 6 unresolved discussions (waiting on @mhendric and @PlagueHO)

appveyor.yml, line 134 at r6 (raw file):