Skip to content
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

Closed
wants to merge 4 commits into from
Closed

Test against 2012R2 and 2016 #403

wants to merge 4 commits into from

Conversation

mgreenegit
Copy link
Contributor

@mgreenegit mgreenegit commented Mar 30, 2018

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 Reviewable

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-io
Copy link

codecov-io commented Mar 30, 2018

Codecov Report

Merging #403 into dev will increase coverage by <1%.
The diff coverage is n/a.

Impacted file tree graph

@@         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

@johlju
Copy link
Member

johlju commented Apr 22, 2018

@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

@johlju johlju added the waiting for author response The pull request is waiting for the author to respond to comments in the pull request. label Apr 22, 2018
@johlju johlju added waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. and removed waiting for author response The pull request is waiting for the author to respond to comments in the pull request. labels May 10, 2018
@johlju
Copy link
Member

johlju commented May 23, 2018

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.

@johlju johlju added abandoned The pull request has been abandoned. and removed waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. labels May 23, 2018
@mhendric
Copy link
Contributor

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....

@mgreenegit
Copy link
Contributor Author

Per the build results, they run independently of each other so it shouldn't increase the build time:
https://ci.appveyor.com/project/PowerShell/xpsdesiredstateconfiguration/build/6.0.1031.0

@mhendric
Copy link
Contributor

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...

@johlju
Copy link
Member

johlju commented Jan 25, 2019

@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.

@PlagueHO
Copy link
Member

That is a good point @johlju - we do want to make sure contributors can still run CI builds on their own free accounts.

@johlju
Copy link
Member

johlju commented Jan 26, 2019

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 after_test: should run for each job, but if this would use Wiki, then maybe Wiki update should be moved to the deploy function (Invoke-AppVeyorDeployTask) in the test framework?

@johlju
Copy link
Member

johlju commented Jan 26, 2019

Regarding sending Codecov from one job, see #477 (comment) for a suggestion.

@stale stale bot removed the abandoned The pull request has been abandoned. label Jan 26, 2019
@mhendric
Copy link
Contributor

mhendric commented Jan 26, 2019

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?

@mhendric
Copy link
Contributor

mhendric commented Jan 26, 2019

Hey and look at that! Both the CI jobs passed. Magic.

@PlagueHO
Copy link
Member

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 mhendric added the on hold The issue or pull request has been put on hold by a maintainer. label Jan 27, 2019
@johlju
Copy link
Member

johlju commented Jan 27, 2019

@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.

@PlagueHO
Copy link
Member

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.

@johlju
Copy link
Member

johlju commented Jan 27, 2019

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 git rebase -i my/dev and drop that commit and I can run all the tests, and send in a PR with my changes. It is also possible to speed up the working branch tests by disabling meta tests

environment:
  SkipAllCommonTests: True

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

environment:
  CI_xService: False
  CI_xProcess: False

@PlagueHO
Copy link
Member

Loving your info and ideas there @johlju - something we should definitely look into once we've got some time!

@johlju
Copy link
Member

johlju commented Jan 28, 2019

@PlagueHO Yes I've had that last bit on the todo list for some time now. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
on hold The issue or pull request has been put on hold by a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants