-
Notifications
You must be signed in to change notification settings - Fork 896
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 6: integration tests #18230
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jrfnl
added
yoast cs/qa
changelog: non-user-facing
Needs to be included in the 'Non-userfacing' category in the changelog
labels
Mar 17, 2022
jrfnl
changed the title
Jrf/switch to ghactions step 6
Switch to GH Actions - step 6: integration tests
Mar 17, 2022
jrfnl
force-pushed
the
JRF/switch-to-ghactions-step-6
branch
from
March 17, 2022 09:32
a4dbd73
to
3d975b5
Compare
diedexx
requested changes
Mar 17, 2022
This commit: * Adds a GH Actions workflow for the PHPUnit Integration test CI check. * Removes all references to that check from the `.travis.yml` configuration. * Replaces the "Build Status" badge in the Readme with a badge using the results from the GH `IntegrationTest` Action runs. All other workflows have their own status badges, so the Travis badge can now be safely removed. 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. In contrast to various other scripts, the matrix for these integration tests is explicitly set with all variables explicitly defined for each build, as we want full control over the three variables (`php_version`, `wp_version` and `multisite`) used for each run. 4. As a MySQL database is needed, we need to explicitly add that service. The MySQL version used varies based on the PHP version against which the tests are run for two reasons: 1. It's good practice to test against multiple different MySQL versions to verify that any queries run are MySQL cross-version compatible. 2. While WP is _supposed to_ support MySQL 8.0 since WP 5.4, in actual fact, WordPress doesn't fully support MySQL 8.0 on PHP < 7.4. Also see: https://core.trac.wordpress.org/ticket/52496 Note: in the Travis script, the PHP 5.6 build was explicitly set to use the `trusty` dist to force the use of MySQL 5.6. In GH Actions, this is not directly linked to the Ubuntu image used. All the same, the script has been set up to explicitly use MySQL 5.6 for PHP 5.6, same as before. 5. 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 6. 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. 7. The PHPUnit version(s) supported and needed to run the tests depend on **both** the WP version used as well as the PHP version used. As an `if` condition on a step would get pretty complicated to handle this, I've introduced a separate - fully documented - step to determine the "type" of install we need. The output of that step is subsequently used in the `if` condition for follow-on steps to make sure the correct PHPUnit version will be installed by Composer. 8. 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 9. As there is a committed `composer.lock` file and the PHPUnit version is locked at PHPUnit 5.x, we have to do an on-the-fly update of the PHPUnit version depending on the WP and PHP version used for the current build. As we still also want to benefit from the Composer caching which the `ramsey/composer-install` action sets up, I've done some nifty trickery with the options passed to the `composer-install` action to get that working. - The `dependency-versions: "highest"` basically changes the command used by the action from `composer install` to `composer update`, however we don't want to update _all_ dependencies as run-time dependencies should remain locked at their original version. - To that end, I'm passing additional `composer-options` which will limit the update to just and only the test dependencies. - These type of updates are only run when needed based on the "install type" determination as described in (7). 10. Installing WordPress was previously done via inline commands in the Travis script. This has now been replaced by the more comprehensive `tests/bin/install-wp-tests.sh` script as made available by the WP CLI `scaffold` command. As WPCLI is not used anywhere else in the workflow, a copy of the script, with source annotations, is being introduced into the plugin codebase. Note: this script needs to have the `x` flag set to allow it to be executed. Ref: https://github.com/wp-cli/scaffold-command/blob/master/templates/install-wp-tests.sh Some differences between the scripts: - The plugin will no longer be copied into the WordPress install, as, to be honest, that's just not necessary with the current test setup/test bootstrap. - The location to which WP is downloaded and installed is different between the scripts, however, this makes no difference for our purposes as the WP Test Utils library already takes the default install locations from the `install-wp-tests.sh` script into account. - To test against the "nightly" of WordPress, the matrix needs to set the `wp_version` to `trunk` or `nightly` instead of `master` (as was previously used in some scripts). - WordPress will be set up to always use `mysqli` for database commands, even on PHP 5.6. 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`. * Except for the test dependencies, no other dependencies will be updated during the test run. * 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. * The "mix" of PHP/WP versions + Multisite used in the matrix has been adjusted slightly compared to Travis. The current mix in GHA ensure there is a run against high + low WP on low PHP, a run against high + low WP on high PHP and a run against mulisite for all supported WP versions. * 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. Builds against WP `trunk`, will be "allowed to fail". Note: if a "allowed to fail" job actually fails, the workflow will show as successful, but there will still be a red `x` next to a PR. This is a known issue in GHA: https://github.com/actions/toolkit/issues/399 There are work-arounds possible for this, however, the work-arounds would hide failures even more, meaning that chances are they won't be seen until they actually become a problem (no longer allowed to fail), which is too late. * Instead of using Git to clone WP, the WP-CLI script is now used to retrieve the relevant parts of WP and initiate the database and the `wp-config` settings. * There are a number of files which are usually created by Grunt and are needed during the test run. In Travis, these files were created on the fly from within the Travis script to ensure tests would not error out on these files missing.. As this was only done in Travis, PHP focussed plugin developers who rarely use Grunt locally, would still end up with these errors when they would run the tests locally. As the need for these files is inherent to the integration tests, creating these files from within the integration test bootstrap file will make this more stable and will allow this to work both in CI as for dev-local test runs. * In Travis, test runs would run either as single site or as multisite. In GH Actions, each build will always run the tests against WP in single site mode and for those builds with the `multisite` flag set to `true` in the matrix, the test will **_also_** be run in multisite mode. * In Travis, for select test runs, code coverage would be calculated and uploaded to CodeClimate. As CodeClimate did not signal back to PRs if a PR changed the code coverage and by how much, this was not very effective, other than to keep track over time, though in practice, nobody seemed to even check on this or check on the reports at all. With this in mind, code coverage will no longer be run, nor uploaded to CodeClimate. The intention is to eventually run code coverage on the Jenkins server.
The test failure being handled by setting an expectation for a PHP deprecation notice is on the list to be fixed via the "Filter extension"-CS action list. Once the particular issue affecting this test has been fixed, the change from this commit should be reverted (and this is safeguarded by the expectation as the test will start to fail because the deprecation will no longer occur).
jrfnl
force-pushed
the
JRF/switch-to-ghactions-step-6
branch
from
March 17, 2022 11:12
3d975b5
to
451af0b
Compare
diedexx
approved these changes
Mar 17, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! 👍
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
Summary
This PR can be summarized in the following changelog entry:
Relevant technical choices:
CI: switch to GitHub Actions - step 6: PHP integration tests
This commit:
.travis.yml
configuration.IntegrationTest
Action runs.All other workflows have their own status badges, so the Travis badge can now be safely removed.
Notes:
Note: manual triggering of builds has to be explicitly allowed. This is not a feature which is enabled by default.
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.php_version
,wp_version
andmultisite
) used for each run.The MySQL version used varies based on the PHP version against which the tests are run for two reasons:
Also see: https://core.trac.wordpress.org/ticket/52496
Note: in the Travis script, the PHP 5.6 build was explicitly set to use the
trusty
dist to force the use of MySQL 5.6. In GH Actions, this is not directly linked to the Ubuntu image used. All the same, the script has been set up to explicitly use MySQL 5.6 for PHP 5.6, same as before.setup-php
action are based on thephp.ini-production
configuration.This means that
error_reporting=E_ALL & ~E_DEPRECATED & ~E_STRICT
,display_errors
is set toOff
andzend.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
anddisplay_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 thephp.ini-develop
configuration, but that may still be a while off.Refs:
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 acomposer install
anyway to get the other dependencies, like WP Test Utils.As an
if
condition on a step would get pretty complicated to handle this, I've introduced a separate - fully documented - step to determine the "type" of install we need.The output of that step is subsequently used in the
if
condition for follow-on steps to make sure the correct PHPUnit version will be installed by Composer.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 potentialcomposer.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
composer.lock
file and the PHPUnit version is locked at PHPUnit 5.x, we have to do an on-the-fly update of the PHPUnit version depending on the WP and PHP version used for the current build.As we still also want to benefit from the Composer caching which the
ramsey/composer-install
action sets up, I've done some nifty trickery with the options passed to thecomposer-install
action to get that working.dependency-versions: "highest"
basically changes the command used by the action fromcomposer install
tocomposer update
, however we don't want to update all dependencies as run-time dependencies should remain locked at their original version.composer-options
which will limit the update to just and only the test dependencies.tests/bin/install-wp-tests.sh
script as made available by the WP CLIscaffold
command.As WPCLI is not used anywhere else in the workflow, a copy of the script, with source annotations, is being introduced into the plugin codebase.
Note: this script needs to have the
x
flag set to allow it to be executed.Ref: https://github.com/wp-cli/scaffold-command/blob/master/templates/install-wp-tests.sh
Some differences between the scripts:
install-wp-tests.sh
script into account.wp_version
totrunk
ornightly
instead ofmaster
(as was previously used in some scripts).mysqli
for database commands, even on PHP 5.6.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
.Except for the test dependencies, no other dependencies will be updated during the test run.
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.The "mix" of PHP/WP versions + Multisite used in the matrix has been adjusted slightly compared to Travis.
The current mix in GHA ensure there is a run against high + low WP on low PHP, a run against high + low WP on high PHP and a run against mulisite for all supported WP versions.
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.
Builds against WP
trunk
, will be "allowed to fail".Note: if a "allowed to fail" job actually fails, the workflow will show as successful, but there will still be a red
x
next to a PR.This is a known issue in GHA: Please support something like "allow-failure" for a given job actions/runner#2347
There are work-arounds possible for this, however, the work-arounds would hide failures even more, meaning that chances are they won't be seen until they actually become a problem (no longer allowed to fail), which is too late.
Instead of using Git to clone WP, the WP-CLI script is now used to retrieve the relevant parts of WP and initiate the database and the
wp-config
settings.There are a number of files which are usually created by Grunt and are needed during the test run.
In Travis, these files were created on the fly from within the Travis script to ensure tests would not error out on these files missing..
As this was only done in Travis, PHP focussed plugin developers who rarely use Grunt locally, would still end up with these errors when they would run the tests locally.
As the need for these files is inherent to the integration tests, creating these files from within the integration test bootstrap file will make this more stable and will allow this to work both in CI as for dev-local test runs.
In Travis, test runs would run either as single site or as multisite. In GH Actions, each build will always run the tests against WP in single site mode and for those builds with the
multisite
flag set totrue
in the matrix, the test will also be run in multisite mode.In Travis, for select test runs, code coverage would be calculated and uploaded to CodeClimate.
As CodeClimate did not signal back to PRs if a PR changed the code coverage and by how much, this was not very effective, other than to keep track over time, though in practice, nobody seemed to even check on this or check on the reports at all.
With this in mind, code coverage will no longer be run, nor uploaded to CodeClimate.
The intention is to eventually run code coverage on the Jenkins server.
PHP 8.1 | Tests: temporarily ignore select test failure
The test failure being handled by setting an expectation for a PHP deprecation notice is on the list to be fixed via the "Filter extension"-CS action list.
Once the particular issue affecting this test has been fixed, the change from this commit should be reverted (and this is safeguarded by the expectation as the test will start to fail because the deprecation will no longer occur).
Test instructions
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.