-
Notifications
You must be signed in to change notification settings - Fork 132
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
xPSDesiredStateConfiguration: Multiple AppVeyor jobs #509
Conversation
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.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 Report
@@ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 😄
I meant theoretically the change I did should work 😄 |
@mhendric Let me know if you find more improvements that can be made. 😄 |
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? |
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. 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 😄 |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
Tests/CommonTestHelper.psm1, line 826 at r4 (raw file):
The message says “...test for ...”, should it say “...tests in ...” instead? |
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 :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
I will fix the review comments tomorrow morning. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@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.
See AppVeyor documentation around Specializing matrix job configuration. I will test this out and report back. |
@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. |
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. |
I will put the latest OS to run first. Sounds like a good plan. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r5, 1 of 1 files at r7.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @PlagueHO)
I'll try and get onto a bunch of this stuff tomorrow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@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. Please retract the comments if I haven't been able to publish the comments until next time you review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
@PlagueHO No problem. See you can publish from Reviewable so it's me only. |
@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. 😄 |
Now it deploys on the job number that is set in appveyor.yml. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 43 files reviewed, 6 unresolved discussions (waiting on @mhendric and @PlagueHO)
appveyor.yml, line 134 at r6 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
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.
Did alternative 2. fast failing.
README.md, line 719 at r7 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
running -> run
Done.
README.md, line 720 at r7 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
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.
Done.
Tests/CommonTestHelper.psm1, line 805 at r7 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Could we use a more expressive name for this function? E.g.
Test-SkipContinuousIntegrationTask -Type 'Unit'
Done. Good name! Didn't thought of that I was abbreviating the function name. 🙂
Tests/Integration/MSFT_xEnvironmentResource.Integration.Tests.ps1, line 6 at r7 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Need to remove extra line here.
Done.
Tests/Integration/MSFT_xRemoteFile.Tests.ps1, line 1 at r7 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
It seems there are several different code snippets that are being used to import the CommonTestHelper module - are we able to standardize?
Yes. It itched in my fingers to fix it, but hold off doing that since I needed to change the tests to do that. I suggest we leave that to a PR that starts refactoring the tests so we get the tests templates and our normal approach of writing tests. What do you think?
Tests/Unit/MSFT_xRemoteFile.Tests.ps1, line 5 at r7 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
FYI: I think we're dealing with this in another issue, but we shouldn't have Global variables here. Should be script.
I fixed $global in tests scripts.
Tests/Unit/ResourceSetHelper.Tests.ps1, line 10 at r7 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
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.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r3, 42 of 43 files at r8, 1 of 1 files at r10.
Reviewable status: complete! all files reviewed, all discussions resolved
Tests/CommonTestHelper.psm1, line 805 at r7 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Done. Good name! Didn't thought of that I was abbreviating the function name. 🙂
I've been reading the "Clean Code" book again (read it 10 years ago) and so many good points in there. So been thinking about expressive naming a lot.
Tests/Integration/MSFT_xRemoteFile.Tests.ps1, line 1 at r7 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Yes. It itched in my fingers to fix it, but hold off doing that since I needed to change the tests to do that. I suggest we leave that to a PR that starts refactoring the tests so we get the tests templates and our normal approach of writing tests. What do you think?
Agreed - we should do that in a separate refactoring session 😁 I completely know what you mean about itching fingers to fix it. 100% on board!
Tests/Unit/MSFT_xRemoteFile.Tests.ps1, line 5 at r7 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
I fixed $global in tests scripts.
Agreed - lets fix this in a separate PR when we refactor the headers. I'm slowly trying to refactor the tests at the same time to meet Pester 4.0 guidelines so we could start to standardize as part of those.
Here we go! 😁 |
Awesome! Thank you @PlagueHO. Now I can copy the same helper function to SqlServerDsc as well. Didn’t want to do that until this was merged :) |
Pull Request (PR) description
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
Entry should say what was changed, and how that affects users (if applicable).
and comment-based help.
This change is