-
Notifications
You must be signed in to change notification settings - Fork 134
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
Test against 2012R2 and 2016 #403
Test against 2012R2 and 2016 #403
Conversation
I am submitting this for test and consideration - this would improve the xPSDesiredStateConfiguration resource quality by testing against both Windows Server 2012R2 and Windows Server 2016 (full desktop in both cases).
Codecov Report
@@ Coverage Diff @@
## dev #403 +/- ##
====================================
+ Coverage 72% 73% +<1%
====================================
Files 27 27
Lines 4031 4031
Branches 4 4
====================================
+ Hits 2922 2945 +23
+ Misses 1105 1082 -23
Partials 4 4 |
@mgreenegit a good plan testing against different platforms. A test fails on the platform that wasn't tested previously, can you look at that? https://ci.appveyor.com/project/PowerShell/xpsdesiredstateconfiguration/build/6.0.1031.0/job/o9lgudehyrerixae?fullLog=true#L2901 |
Labeling this 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 the PR is taken up again. |
Hey @mgreenegit (or @PlagueHO or @johlju). Any idea what this change would do to the CI run time? Would it approximately double the time it takes to do tests? Or would the separate jobs run concurrently? Per #477, we're about to hit the 60 minute AppVeyor limit, so anything which significantly increases the build time may not be good. And per @johlju 's comment, it looks like we had a failing test that will need to be resolved. May be worth a rebase (and rebuild) now that @PlagueHO fixed a bunch of the failing tests and see if we still have issues.... |
Per the build results, they run independently of each other so it shouldn't increase the build time: |
Perfect, thanks @mgreenegit . Mind doing a rebase to see if your build issue was transient, or is still an issue? And we'll ultimately need an Unreleased entry in the README for this to be checked in... |
@mhendric It will increase the time it takes to run the tests on a AppVeyor free account where the jobs run sequentially, but on the PowerShell AppVeyor account they would run in parallel since it is as paid account (and have the ability to run jobs in parallel). Although each job (build worker) can run for 60 minutes on a free account, and 180 minutes on the PowerShell AppVeyor account. Update: Wrote wrong minutes on the PowerShell AppVeyor account. |
That is a good point @johlju - we do want to make sure contributors can still run CI builds on their own free accounts. |
I think we need to look over how the Codecov is reported, by making sure only sending Codecov for one of the jobs (images)? The same goes for the |
Regarding sending Codecov from one job, see #477 (comment) for a suggestion. |
FYI, I just resolved the conflicts in this one so we can see if CI will pass. Just to make sure I'm clear, if we do approve this, people would still be able to run this against their own free AppVeyor accounts as long as the individual jobs are under 60 minutes, correct? It doesn't matter that the cumulative job length would exceed 60 minutes, as long as individual jobs stay under 60 mins. Also, is there an upper limit on how long the total build can take for a free account? Any which way, I'm wondering if we should delay this one until we can somewhat address #477. What do you all think? |
Hey and look at that! Both the CI jobs passed. Magic. |
I'm good with delaying this until #477 has been addressed. It is good this will still work on free AppVeyor accounts. But these test runs will run in series, which I assume will take around 45-60 mins each, making 90-120 minutes total. I'd definitely find that painful personally. |
@mhendric yes as long as each individual job stays under 60 minutes it is all good on a free account. @PlagueHO It is only painful if we have to sit and watch it. 😉 Not sure it is meant for CI to be fast - but rather give quality? 😄 I'm good with it taking time - but speed up the tests so they don't exceed the 60 minute limit per job and there is room to add more tests. |
Haha @johlju - you are right there - as long as we don't have to watch it. 😁 but what might be nice is to enable a lighter set of tests to run in working branches, with the full suite running in PR and master/dev branches. Not worth the complexity though. I'm used to only running unit/linting during CI and then Integration/E2E during CD pipeline. But that isn't really possible here. |
The way I speed up things. In a working branch I usually just remove a resources, examples, and tests that is not needed for the thing I'm writing and commit that as 'debug-appveyor'. Then when I finsihed, is just
Although, I have thought if we could add a check in the test framework (or in the test templates) that if an AppVeyor environment variable exist (either in the appveyor.yml or in the AppVeyor GUI), that test for that resource would not run. For example
|
Loving your info and ideas there @johlju - something we should definitely look into once we've got some time! |
@PlagueHO Yes I've had that last bit on the todo list for some time now. :) |
I am submitting this for test and consideration - this would improve the xPSDesiredStateConfiguration resource quality by testing against both Windows Server 2012R2 and Windows Server 2016 (full desktop in both cases).
This change is