silverstripe / silverstripe-travis-support

Creates a SilverStripe project "around" a module, based on core version constraints and its composer.json definitions
Other
13 stars 16 forks source link

FIX Determine which core release to use including tags #38

Closed dhensby closed 7 years ago

dhensby commented 7 years ago

This is a better way to determine the core release to use on tests.

At the moment if we use 3.1 then this is resolved to 3.1.x-dev. This is implicitly the 3.1 branch (now deleted) and so we need a fallback so the latest 3.1 is used.

This keeps all the old behaviour but also moves to using tags if a 3.x-dev version can't be found.

helpfulrobot commented 7 years ago

@dhensby, thanks for your PR! By analyzing the blame information on this pull request, I identified @chillu and @kinglozzer to be potential reviewers

dhensby commented 7 years ago

I've had to include the vendor folder because the modules that depend on this module don't perform a composer install on the checked-out version

chillu commented 7 years ago

Looks good to me in general. It gets us further away from the idea of running builds without travis-support installed. Technically modules should not need to test against versions of SilverStripe which are no longer supported, right? So the fact that their builds break because a branch couldn't be found is a good thing: They should remove those builds. If there's a need to test against older framework releases in modules, you can always use tags - since you can rely on the "last" tag of an unsupported branch. Or am I missing something here?

tractorcow commented 7 years ago

I've checked this out and given it a thorough review... it looks good to me, however it would benefit from a unit test please. :) There are enough scaffolded mocks in the current test that you should be able to add a test without too much extra work.

Technically modules should not need to test against versions of SilverStripe which are no longer supported, right?

I don't think they should immediately break, otherwise we end up fighting phoney fires caused by core releases being outdated. I think we have enough open source noise as it is.

dhensby commented 7 years ago

Whilst we might not support 3.blah anymore, I don't think that means other vendors won't and people may want some reassurance the builds still work with 3.blah.

I'll add a unit test to this :)

chillu commented 7 years ago

The reassurance can be provided by module authors using tags rather than branches on unsupported releases. But yeah, the code is written already, and it means less noise - so I'm happy with merging it (once there are tests). Hopefully this will be less of a problem on the 4.x release line since the release line itself is supported for much longer, and modules wouldn't create separate builds for 4.0, 4.1 etc.

codecov-io commented 7 years ago

Current coverage is 46.47% (diff: 38.36%)

Merging #38 into master will decrease coverage by 53.52%

@@           master        #38   diff @@
========================================
  Files           1         10     +9   
  Lines         115        781   +666   
  Methods        12         73    +61   
  Messages        0          0          
  Branches        0          0          
========================================
+ Hits          115        363   +248   
- Misses          0        418   +418   
  Partials        0          0          

Powered by Codecov. Last update 98ec226...ee021aa

dhensby commented 7 years ago

OK - I can't seem to get codecov to ignore the vendor folder for some reason.

tractorcow commented 7 years ago

I found one bug... you need to get this new version to checkout the installer repo.

https://github.com/silverstripe/silverstripe-travis-support/blob/master/travis_setup.php#L131