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

Travis: use a mix of PHPCS versions in the matrix #91

Merged

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Dec 15, 2019

Proposed Changes

Using a normal composer install means that while the build was testing against various PHP versions, it would only ever test against the latest PHPCS 3.x version (3.5.3 at the time of writing) for PHP 5.4-7.4 and PHPCS 2.9.2 for PHP 5.3.

This PR changes the build matrix to test against a variety of PHPCS versions to try to ensure compatibility with all versions this plugin claims to support.

Some explanation is needed about the choices made in this changeset:

Adding an install section

The install section is intended for installing prerequisites for test build tests.

AFAICS, this wasn't previously used to avoid the PHP lint command running over the vendor directory, but that can be solved by just removing the vendor directory from the list PHP lint is run over.

Having the install instructions in the install section makes it clearer that the installation is not part of the tests and what's being installed.

Changes to the PHP lint command

As mentioned above, the command now excludes the vendor directory.

Also, as most PHP versions now have a second build, adding a LINT environment variable so the PHP linting is only done on one build per PHP version.

Changes to the PHPCS run test command

The PSR12 ruleset which is part of the phpcs.xml.dist ruleset was only added in PHPCS 3.3.0.

As testing is now done against a wider range of PHPCS versions, this PSR12 ruleset won't always be available, so I've added a PHPCS environment variable to only run the PHPCS check against the repo specific ruleset once per build.

I've added this variable to a build against the latest PHPCS version. While I'd like to use dev-master for this, that may be prone to intermittent bugs in PHPCS, so using a stable PHPCS version instead makes the build more sane.

On all the other builds, only a check against PHPCompatibility is run which then tests whether the setting of the installed_paths for external standards worked correctly.

Now there are two caveats to this:

  • PHPCompatibility 9.0.0 dropped support for PHPCS < 2.3.0.
    For testing against PHPCS 2.2.0 - 2.3.0 we need to explicitly install PHPCompatibility 8.x.
    For testing against PHPCS < 2.2.0 we need to explicitly install PHPCompatibility 7.x, however PHPCompatibility 7.x wasn't compatible with Composer installs yet, so in that case we also need to rename the directory within vendor to make things work.
    Note/advance notice: PHPCompatibility 10.0.0, which is expected over the next few months, will drop support for PHPCS < 2.6.0 and PHP < 5.4.
    Also see: Proposal: drop support for PHP 5.3 and PHPCS < 2.6.0 when PHP 7.4 is released PHPCompatibility/PHPCompatibility#835
  • The PHPCS --exclude option previously used to exclude the PHPCompatibility.Upgrade.LowPHPCS sniff is only available since PHPCS 2.6.2, so cannot be used to exclude that sniff on older PHPCS versions.
    With that in mind, switching to using the --sniffs= command line option to limit the PHPCS run to just one sniff.
    This option has been available since PHPCS 1.x (or even before), so can be used without problem.
    Additionally, limiting the test run to just one sniff will make it faster as well.
    I've just selected one of the sniffs from PHPCompatibility which has been around for a while for this. The actual sniff doesn't matter after all, as this test is just about PHPCS recognizing the external standard correctly.

Custom PHP 5.3 script

As the script for the PHPCS run has changed now, there is no need for the custom script section for the PHP 5.3 build anymore. This can now revert back to using the normal test script again, so to that end, I've remove the custom script.

Future improvements

This is a first iteration to improve the build script.

In a further iteration, I'd like to suggest exploring the following additional variants:

  • Having some builds with a global install of PHPCS + global install of the external standard used for testing.
  • Having some builds with a mix of global/local installs of external standards(s).
  • Having some builds using Windows as OS instead of Linux.

Using a normal `composer install` means that while the build was testing against various PHP versions, it would only ever test against the latest PHPCS 3.x version (`3.5.3` at the time of writing) for PHP 5.4-7.4 and PHPCS 2.9.2 for PHP 5.3.

This PR changes the build matrix to test against a variety of PHPCS versions to try to ensure compatibility with all versions this plugin claims to support.

Some explanation is needed about the choices made in this changeset:

### Adding an `install` section

The `install` section is intended for installing prerequisites for test build tests.

AFAICS, this wasn't previously used to avoid the PHP `lint` command running over the `vendor` directory, but that can be solved by just removing the `vendor` directory from the list PHP `lint` is run over.

Having the install instructions in the `install` section makes it clearer that the installation is not part of the tests.

### Changes to the PHP `lint` command

As mentioned above, the command now excludes the `vendor` directory.

Also, as most PHP versions now have a second build, adding a `LINT` environment variable so the PHP linting is only done on one build per PHP version.

### Changes to the PHPCS run test command

The PSR12 ruleset which is part of the `phpcs.xml.dist` ruleset was only added in PHPCS 3.3.0.

As testing is now done against a wider range of PHPCS versions, this `PSR12` ruleset won't always be available, so I've added a `PHPCS` environment variable to only run the PHPCS check against the repo specific ruleset _once_ per build.

I've added this variable to a build against the latest PHPCS version. While I'd like to use `dev-master` for this, that may be prone to intermittent bugs in PHPCS, so using a stable PHPCS version instead makes the build more sane.

On all the other builds, only a check against `PHPCompatibility` is run which then tests whether the setting of the `installed_paths` for external standards worked correctly.

Now there are two caveats to this:
* PHPCompatibility 9.0.0 dropped support for PHPCS < 2.3.0.
    For testing against PHPCS 2.2.0 - 2.3.0 we need to explicitly install PHPCompatibility 8.x.
    For testing against PHPCS < 2.2.0 we need to explicitly install PHPCompatibility 7.x, however PHPCompatibility 7.x wasn't compatible with Composer installs yet, so in that case we also need to rename the directory within `vendor` to make things work.
    Note/advance notice: PHPCompatibility 10.0.0, which is expected over the next few months, will drop support for PHPCS < 2.6.0 and PHP < 5.4.
    Also see: PHPCompatibility/PHPCompatibility#835
* The PHPCS `--exclude` option previously used to exclude the `PHPCompatibility.Upgrade.LowPHPCS` sniff is only available since PHPCS 2.6.2, so cannot be used to exclude that sniff on older PHPCS versions.
    With that in mind, switching to using the `--sniffs=` command line option to limit the PHPCS run to just one sniff.
    This option has been available since PHPCS 1.x (or even before), so can be used without problem.
    Additionally, limiting the test run to just one sniff will make it faster as well.
    I've just selected one of the sniffs from PHPCompatibility which has been around for a while for this. The actual sniff doesn't matter after all, as this test is just about PHPCS recognizing the external standard correctly.

### Custom PHP 5.3 script

As the script for the PHPCS run has changed now, there is no need for the custom `script` section for the PHP 5.3 build anymore. This can now revert back to using the normal test `script` again, so to that end, I've remove the custom script.

## Future improvements

This is a first iteration to improve the build script.

In a further iteration, I'd like to suggest exploring the following additional variants:
* Having some builds with a global install of PHPCS + global install of the external standard used for testing.
* Having some builds with a mix of global/local installs of external standards(s).
* Having some builds using Windows as OS instead of Linux.
fi

script:
- if [[ "$LINT" == "1" ]]; then if find . -path ./vendor -prune -o -name "*.php" -exec php -l {} \; | grep "^[Parse error|Fatal error]"; then exit 1; fi; fi
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if, instead of using

find . -path ./vendor -prune -o -name "*.php" -exec php -l {} \; 

it might be easier to just use

find ./src/ -name "*.php" -exec php -l {} \;

🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Potherca I understand the question. I didn't in the PR as it's really easy to overlook something like that and forget to change it again when - for instance - a tests/ directory would be added at a later time.

As it is now, the logic is consistent with the in/exclusion rules also used in the phpcs.xml.dist file

Copy link
Member

@Potherca Potherca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got one comment, mostly stylistic. Other than that this looks good to me.

@mjrider If you don't have any concerns I'd like to merge this.

Copy link
Contributor

@mjrider mjrider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mjrider mjrider merged commit c20ba7d into master Dec 16, 2019
@jrfnl jrfnl deleted the feature/travis-test-against-variety-of-phpcs-versions branch December 17, 2019 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants