WPTT / WPThemeReview

PHP_CodeSniffer rules (sniffs) to enforce WordPress theme review coding conventions
MIT License
207 stars 38 forks source link

Add allow failures for WPCS dev-develop #259

Closed dingo-d closed 3 years ago

dingo-d commented 3 years ago

This is an attempt to fix the failing builds based on comment.

I'm not an expert with travis, so not 100% sure the allow_failures is correct.

The WPCS_BRANCH is set to dev-master so that it's stable (develop is in the works currently).

jrfnl commented 3 years ago

Oh and you may also want to have a look at WPCS PR https://github.com/WordPress/WordPress-Coding-Standards/pull/1906 (line 161-164 - using composer install --ignore-platform-reqs) to see how to get the nightly builds working again.

dingo-d commented 3 years ago

@jrfnl I've added the lines but it still fails on if [[ ${TRAVIS_PHP_VERSION:0:3} > "7.1" ]]; then for PHP versions 7.0 and 7.1. I'm missing something. Also, I think the 7c3b58f commit and e4b2e96 where I've put the nightly out of if/elseif/else conditional are the same (both failed on the same thing).

jrfnl commented 3 years ago

@dingo-d Just had a look at the error Travis is showing. This has to do with the change from trusty to xenial. Basically, a number of Travis PHP images ship with incompatible PHPUnit versions (yeah, the stupidest thing ever, but why would they care... ). For the images where this previously happened on trusty, we had the condition you refer to in place to get round it. Now the builds are using xenial, we either need to investigate which images are problematic and adjust the condition based on that, or better yet; stop relying on the PHPUnit version shipped with Travis and for all test builds do a full composer install and run the tests via vendor/bin/phpunit or composer run-tests.

Does that help ?

jrfnl commented 3 years ago

Nightly

As for the builds against PHP nightly, as PHPUnit 7.x is not officially supported on PHP 8.0, you'll need to do a composer install with --ignore-platform-reqs to get PHPUnit 7.x to run on PHP 8. Once that's done, the tests should run fine on PHPUnit 7.x as the test suite doesn't use the mocking framework.

The tests will probably fail on PHP 8.0, but that will be mostly solved once TRTCS moves to PHPCSUtils/with a newer minimal PHPCS version (unreleased as of yet). Most failures are to do with some BC-breaks in PHP 8, which have so far, partially been mitigated by me in PHPCS itself, though most fixes are not yet included in a released version. For most things, PHPCSUtils already works around them in a cross-version way.

WPCS dev-develop

As for WPCS dev-develop, as PHPCSUtils is regarded (for now) as "unstable", you either need to add the minimum-stability setting to composer.json, but that affects all packages, while we only really want it for the one build, so better yet, you can adjust this in the . travis.yml script with something like the below above the composer require command:

  - |
    if [[ ${WPCS_BRANCH} == "dev-develop" ]]; then
      composer config minimum-stability dev
    fi
jrfnl commented 3 years ago

@dingo-d Let me know if you want me to finish this off.

dingo-d commented 3 years ago

The xenial issue is super frustrating 😣

I'll give this another go and then let you do your magic 😄

jrfnl commented 3 years ago

The xenial issue is super frustrating

Only the first time you run into it. After that you know what's going on ;-)

If Travis caching for the composer downloads is setup properly, you're basically better off not relying on the Travis images for dependencies and just doing a composer install. Once this repo switches over to using PHPCSUtils, you'd need that anyway, so may as well get it over with now.

dingo-d commented 3 years ago

Now I'm getting

The command "phpenv global 5.5" failed and exited with 1 during .

For PHP 5.5 and 5.4. Can you check what I missed?

jrfnl commented 3 years ago

For PHP 5.5 and 5.4. Can you check what I missed?

You put them back in the php key, which is what won't work anymore on xenial. Those are failing.

The ones you put in the jobs key with the dist key are installing fine, but those are incomplete as you now test them only against the "low" settings, not the "high" settings.

dingo-d commented 3 years ago

@jrfnl The builds are passing, is this ok now? 😂

I suck at travis, GH actions are a lot easier to understand.

jrfnl commented 3 years ago

I suck at travis, GH actions are a lot easier to understand.

Well, if the same can be done with GH Actions, you can always change the CI setup ;-)

dingo-d commented 3 years ago

Well, if the same can be done with GH Actions, you can always change the CI setup ;-)

I'll try to play with it :+1:

dingo-d commented 3 years ago

@jrfnl This seems to pass. Can you give it another look? Thanks!

jrfnl commented 3 years ago

Closing as superseded by the now merged PR #261.