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

Switch to GH Actions - step 5: unit tests #18209

Merged
merged 4 commits into from Mar 15, 2022
Merged

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Mar 14, 2022

Context

  • CI/QA is now run via GitHub Actions for the PHP unit tests

Summary

This PR can be summarized in the following changelog entry:

  • CI/QA is now run via GitHub Actions for the PHP unit tests

Relevant technical choices:

CI: switch to GitHub Actions - step 5: PHP unit tests

This commit:

  • Adds a GH Actions workflow for the PHPUnit CI check.
  • Removes all references to that check from the .travis.yml configuration.
  • Adds a "Unit Tests" badge in the Readme using the results from the GH Unit Test Action runs.

Notes:

  1. Builds will run on:
    • Select pushes using branch filtering similar to before.
    • All pull requests.
    • When manually triggered.
      Note: manual triggering of builds has to be explicitly allowed. This is not a feature which is enabled by default.
  2. If a previous GH actions run for the same branch hadn't finished yet when the same branch is pushed again, the previous run will be cancelled.
    In Travis, this was an option on the "Settings" page - "Auto cancellation" -, which was turned on for most, if not all, Yoast repos. The concurrency configuration in the GHA script emulates the same behaviour.
  3. The default ini settings used by the setup-php action are based on the php.ini-production configuration.
    This means that error_reporting=E_ALL & ~E_DEPRECATED & ~E_STRICT, display_errors is set to Off and zend.assertions is set to -1 (= do not compile).
    For the purposes of CI, especially for linting and testing code, I'd recommend running with zend.assertions=-1, error_reporting=-1 and display_errors=On to ensure all PHP notices are shown.
    Note: the defaults will be changed in the next major of setup-php to be based on the php.ini-develop configuration, but that may still be a while off.
    Refs:
  4. While the setup-php action offers the ability to install the PHAR file for PHPUnit, I've elected not to use that option as we need to do a composer install anyway to get the other dependencies, like WP Test Utils.
  5. Composer dependency downloads will be cached for faster builds using a predefined GH action specifically created for this purpose.
    The alternative would be to handle the caching manually, which would add three extra steps to the script.
    Note: Caching works differently between Travis and GH Actions.
    On GH Actions, once a cache has been created, it can't be updated. It can only be replaced by a new cache with a different key.
    As the PHP version, the composer.json and a potential composer.lock hash are all part of the key used by the above mentioned action, this difference should not have a significant impact.
    Ref: https://docs.github.com/en/actions/advanced-guides/caching-dependencies-to-speed-up-workflows
  6. As the prefixing of the dependencies needs a higher PHP version, the setup-php action is used twice within this workflow.
    The first time, it sets up PHP 7.2 to allow for creating the vendor_prefixed directory.
    The second time, it sets up the PHP version on which we want to do the actual linting.
    Note: last time I checked, I found that within a workflow you can only switch PHP versions once, but that is sufficient for our purposes. (may have changed in the mean time)
  7. For these BrainMonkey based unit tests, we can (and should) use the most appropriate PHPUnit/BrainMonkey/Mockery/etc version for the PHP version the tests are being run on. Most notably, this is actually needed to get the tests running on PHP 8.0 and higher.
    As there is a committed composer.lock file and the PHPUnit version is locked at PHPUnit 5.x, this means we have to do an on-the-fly update of the PHPUnit version depending on the PHP version of the current build.
    As the composer.lock file for this plugin also contains dependencies which are locked at versions which are incompatible with the full range of PHP versions supported by the plugin, we need to do some juggling to allow the testing to work (on all supported PHP versions).
    Previously in Travis, an uncontrolled composer update would be run to get round this.
    Uncontrolled updates are generally not a good idea in CI as you may end up with passing tests based on updated dependencies, while in reality things would fail.
    So, for this workflow, I've chosen to approach this slightly differently (compared to Travis), but still tried to keep things as simple as possible.
    • First a full install is done on PHP 7.2. This will generate the vendor_prefixed directory and create the generated files for the dependency injection, as well as set up all dependencies.
    • Next, the dependencies which are only needed for the prefixing are removed.
      This will remove a couple of dependencies which would automatically be loaded by Composer on loading of the autoload file (due to the use of autoload.files in those dependencies) and would trigger a parse error on PHP < 7.2 as they contain non-cross-version compatible code.
    • Lastly, we remove the vendor directory.
    • Now those incompatible dependencies are removed, we switch to the actual PHP version we want to use for the tests.
    • Next, we remove the phpunit/phpunit root requirement as it is already a dependency of the WP Test Utils and we don't want to abide by the locked version of PHPUnit.
    • We then run a selective composer update to install the locked dependencies and update just and only the test dependencies to match the PHP version on which we will be running the tests.
      Altogether, this prevents the uncontrolled dependency update.
      Additionally, the --no-scripts addition to the Composer run commands when run on the real PHP version, will prevent the prefixing/dependency injection tasks from being run again on incompatible PHP versions.
      Note: no need for adding --no-interaction to the plain composer ... commands as the setup-php action already sets an environment variable to ensure Composer will always run with --no-interaction in these workflows.

Differences with the Travis implementation:

  • The most appropriate version of the test dependencies will now be used for all builds, not just for builds against PHP 8.0/nightly.
  • In Travis, there was a reference to a atanamo/php-codeshift package, which was removed after the prefixing was finished.
    This package is not a (root) dependency of the WPSEO Free plugin, so removing it doesn't actually have any effect.
    Instead, the league/oauth2-client package is removed, as that package should not be used during the test run as it will be prefixed and the prefixed version should be used instead.
  • Except for the test dependencies, no other dependencies will be updated during the test run.
  • In addition to the PHP versions against which the tests were previously run on Travis, the tests will now also be run against PHP 8.1.
    As PHP 8.1 has been released, the (remaining) test failures on this build need to be fixed, though as these are only deprecation notices, the fact that these tests are currently failing is nothing to be too concerned about for the time being.
    To prevent new failures from being introduced in the mean time, I'm not allowing the build to fail, but will instead add expectations for the deprecations or skip requirements to the few tests which still have them for the time being.

PHP 8.1 | Tests: temporarily ignore select test failures [1]

The test failures being ignored in this commit are on the list to be fixed via the "Filter extension"-CS action list.

Once the particular issue affecting these tests - Constant FILTER_SANITIZE_STRING is deprecated - has been fixed, the @requires tags should be removed again.

Note: unfortunately, a @requires tag is needed to get round this for now as the test code itself uses the deprecated FILTER_SANITIZE_STRING constant, which means that setting an expectation for a PHP notice will not suffice.

PHP 8.1 | Tests: temporarily ignore select test failures [2]

The test failures being handled by setting an expectation for a PHP deprecation notice are on the list to be fixed via the "Filter extension"-CS action list.

Once the particular issue affecting these tests has been fixed, these expectations can be removed (and this is safeguarded by the expectation as the test will start to fail because the deprecation will no longer occur).

PHP 8.1 | Tests: temporarily ignore select test failures [3]

The test failures being handled by setting an expectation for a PHP deprecation notice are on the list to be fixed via the "Filter extension"-CS action list.

Once the particular issue affecting these tests has been fixed, these expectations can be removed (and this is safeguarded by the expectation as the test will start to fail because the deprecation will no longer occur).

Note: the ->zeroOrMoreTimes() addition to the expectation for a call to the _deprecated_function() function should also removed at that time (though the test will not fail on it) !

Test instructions

Test instructions for the acceptance test before the PR gets merged

This PR can be acceptance tested by following these steps:

This workflow has been extensively tested already and needs no further testing.

Aside from the builds run for this PR, you can find the results of various specific tests also on the "Actions" page.
For each job, it has been verified that the build(s) will actually show as failed when code has been altered to induce a fail condition.

@jrfnl jrfnl added yoast cs/qa changelog: non-user-facing Needs to be included in the 'Non-userfacing' category in the changelog labels Mar 14, 2022
@jrfnl jrfnl added this to the 18.5 milestone Mar 14, 2022
This commit:
* Adds a GH Actions workflow for the PHPUnit CI check.
* Removes all references to that check from the `.travis.yml` configuration.
* Adds a "Unit Tests" badge in the Readme using the results from the GH `Unit Test` Action runs.

Notes:
1. Builds will run on:
    - Select pushes using branch filtering similar to before.
    - All pull requests.
    - When manually triggered.
        Note: manual triggering of builds has to be [explicitly allowed](https://github.blog/changelog/2020-07-06-github-actions-manual-triggers-with-workflow_dispatch/). This is not a feature which is enabled by default.
2. If a previous GH actions run for the same branch hadn't finished yet when the same branch is pushed again, the previous run will be cancelled.
    In Travis, this was an option on the "Settings" page - "Auto cancellation" -, which was turned on for most, if not all, Yoast repos. The `concurrency` configuration in the GHA script emulates the same behaviour.
3. The default ini settings used by the `setup-php` action are based on the `php.ini-production` configuration.
    This means that `error_reporting=E_ALL & ~E_DEPRECATED & ~E_STRICT`, `display_errors` is set to `Off` and `zend.assertions` is set to `-1` (= do not compile).
    For the purposes of CI, especially for linting and testing code, I'd recommend running with `zend.assertions=-1`, `error_reporting=-1` and `display_errors=On` to ensure **all** PHP notices are shown.
    Note: the defaults will be changed in the next major of `setup-php` to be based on the `php.ini-develop` configuration, but that may still be a while off.
    Refs:
    * shivammathur/setup-php#450
    * shivammathur/setup-php#469
4. While the `setup-php` action offers the ability to [install the PHAR file for PHPUnit](https://github.com/shivammathur/setup-php#wrench-tools-support), I've elected not to use that option as we need to do a `composer install` anyway to get the other dependencies, like WP Test Utils.
5. Composer dependency downloads will be cached for faster builds using a [predefined GH action](https://github.com/marketplace/actions/install-composer-dependencies) specifically created for this purpose.
    The alternative would be to handle the caching manually, which would add three extra steps to the script.
    Note: Caching works differently between Travis and GH Actions.
    On GH Actions, once a cache has been created, it can't be updated. It can only be replaced by a new cache with a different key.
    As the PHP version, the `composer.json` and a potential `composer.lock` hash are all part of the key used by the above mentioned action, this difference should not have a significant impact.
    Ref: https://docs.github.com/en/actions/advanced-guides/caching-dependencies-to-speed-up-workflows
6. As the prefixing of the dependencies needs a higher PHP version, the `setup-php` action is used twice within this workflow.
    The first time, it sets up PHP 7.2 to allow for creating the `vendor_prefixed` directory.
    The second time, it sets up the PHP version on which we want to do the actual linting.
    Note: last time I checked, I found that within a workflow you can only switch PHP versions once, but that is sufficient for our purposes. (may have changed in the mean time)
7. For these BrainMonkey based unit tests, we can (and should) use the most appropriate PHPUnit/BrainMonkey/Mockery/etc version for the PHP version the tests are being run on. Most notably, this is actually needed to get the tests running on PHP 8.0 and higher.
    As there is a committed `composer.lock` file and the PHPUnit version is locked at PHPUnit 5.x, this means we have to do an on-the-fly update of the PHPUnit version depending on the PHP version of the current build.
    As the `composer.lock` file for this plugin also contains dependencies which are locked at versions which are incompatible with the full range of PHP versions supported by the plugin, we need to do some juggling to allow the testing to work (on all supported PHP versions).
    Previously in Travis, an uncontrolled `composer update` would be run to get round this.
    Uncontrolled updates are generally not a good idea in CI as you may end up with passing tests based on updated dependencies, while in reality things would fail.
    So, for this workflow, I've chosen to approach this slightly differently (compared to Travis), but still tried to keep things as simple as possible.
    - First a full install is done on PHP 7.2. This will generate the `vendor_prefixed` directory and create the generated files for the dependency injection, as well as set up all dependencies.
    - Next, the dependencies which are only needed for the prefixing are removed.
        This will remove a couple of dependencies which would automatically be loaded by Composer on loading of the autoload file (due to the use of `autoload.files` in those dependencies) and would trigger a parse error on PHP < 7.2 as they contain non-cross-version compatible code.
    - Lastly, we remove the `vendor` directory.
    - Now those incompatible dependencies are removed, we switch to the actual PHP version we want to use for the tests.
    - Next, we remove the `phpunit/phpunit` root requirement as it is already a dependency of the WP Test Utils and we don't want to abide by the locked version of PHPUnit.
    - We then run a selective `composer update` to install the locked dependencies and update just and only the test dependencies to match the PHP version on which we will be running the  tests.
    Altogether, this prevents the uncontrolled dependency update.
    Additionally, the `--no-scripts` addition to the Composer run commands when run on the _real_ PHP version, will prevent the prefixing/dependency injection tasks from being run again on incompatible PHP versions.
    Note: no need for adding `--no-interaction` to the plain `composer ...` commands as the `setup-php` action already sets an environment variable to ensure Composer will always run with `--no-interaction` in these workflows.

Differences with the Travis implementation:
* The most appropriate version of the test dependencies will now be used for _all_ builds, not just for builds against PHP 8.0/`nightly`.
* In Travis, there was a reference to a `atanamo/php-codeshift` package, which was removed after the prefixing was finished.
    This package is not a (root) dependency of the WPSEO Free plugin, so removing it doesn't actually have any effect.
    Instead, the `league/oauth2-client` package is removed, as that package should not be used during the test run as it will be prefixed and the prefixed version should be used instead.
* Except for the test dependencies, no other dependencies will be updated during the test run.
* In addition to the PHP versions against which the tests were previously run on Travis, the tests will now also be run against PHP 8.1.
    As PHP 8.1 has been released, the (remaining) test failures on this build need to be fixed, though as these are only _deprecation notices_, the fact that these tests are currently failing is nothing to be too concerned about for the time being.
    To prevent new failures from being introduced in the mean time, I'm not allowing the build to fail, but will instead add expectations for the deprecations or skip requirements to the few tests which still have them for the time being.
The test failures being ignored in this commit are on the list to be fixed via the "Filter extension"-CS action list.

Once the particular issue affecting these tests - `Constant FILTER_SANITIZE_STRING is deprecated` - has been fixed, the `@requires` tags should be removed again.

Note: unfortunately, a `@requires` tag is needed to get round this for now as the test code itself uses the deprecated `FILTER_SANITIZE_STRING` constant, which means that setting an expectation for a PHP notice will not suffice.
The test failures being handled by setting an expectation for a PHP deprecation notice are on the list to be fixed via the "Filter extension"-CS action list.

Once the particular issue affecting these tests has been fixed, these expectations can be removed (and this is safeguarded by the expectation as the test will start to fail because the deprecation will no longer occur).
The test failures being handled by setting an expectation for a PHP deprecation notice are on the list to be fixed via the "Filter extension"-CS action list.

Once the particular issue affecting these tests has been fixed, these expectations can be removed (and this is safeguarded by the expectation as the test will start to fail because the deprecation will no longer occur).

Note: the call to the `_deprecated_function()`  function isn't actually part of the test - we're allowing for it to be called, not _expecting_ it to always be called, I've updated the test to _stub_ the function instead setting an expectation for it.
This is necessary, for now, for the tests to pass, but at the same time, it is the better way to handle this anyway, so only the conditional "expectDeprecation" block should be removed when the PHP 8.1 issue is fixed, not this change.
@jrfnl jrfnl force-pushed the JRF/switch-to-ghactions-step-5 branch from 13f3264 to e916076 Compare March 15, 2022 00:08
@jrfnl
Copy link
Contributor Author

jrfnl commented Mar 15, 2022

Rebased + made a small change in the Main_test (temp ignore commit 3) as discussed in our call yesterday: instead of setting an expectation for the WP _deprecated_function() function being called, the test now stubs the _deprecated_function() function instead.

@diedexx diedexx merged commit e4e2a27 into trunk Mar 15, 2022
@diedexx diedexx deleted the JRF/switch-to-ghactions-step-5 branch March 15, 2022 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: non-user-facing Needs to be included in the 'Non-userfacing' category in the changelog yoast cs/qa
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants