#50401 closed task (blessed) (fixed)
Tests: Move tests into Github Actions
Reported by: | whyisjake | Owned by: | desrosj |
---|---|---|---|
Milestone: | 5.7 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Build/Test Tools | Keywords: | has-patch |
Focuses: | performance | Cc: |
Description
Our Travis testing environments can't get bogged down during periods of heavy development. (Let's be real, all the time...)
Let's do some experimentation and move some of our tests into GitHub Actions.
Attachments (4)
Change History (166)
#2
@
4 years ago
Great to see this being started!
I experimented a little with this a few weeks back: https://github.com/Ayesh/wordpress-develop/tree/github-actions/.github/workflows
I will open a PR with those changes. The tests are not fully migrated because we have JS, and database tests, which should be spread across different workflow files. But hopefully the PR is a starting point.
This ticket was mentioned in PR #337 on WordPress/wordpress-develop by Ayesh.
4 years ago
#3
- Keywords has-patch added
A PR to test GitHub Actions for Wordpress-Develop repo.
I will experiment changes with Ayesh/Wordpress-develop github-actions-2
branch first, and cherry-pick commits to this PR branch.
Trac ticket: https://core.trac.wordpress.org/ticket/50401
This ticket was mentioned in Slack in #core by whyisjake. View the logs.
4 years ago
#7
@
4 years ago
Working through this with @desrosj, linting is working here (you can see some forced PHP errors and also interesting some JS ones): https://github.com/helen/wordpress-develop/pull/1/files
@desrosj is going to pull that together with his work on getting the rest of the tests running shortly.
This ticket was mentioned in PR #593 on WordPress/wordpress-develop by desrosj.
4 years ago
#8
- Keywords has-patch added
Trac ticket: https://core.trac.wordpress.org/ticket/50401
#9
@
4 years ago
- Keywords needs-testing added
- Version 5.5 deleted
I've spent a chunk of time working through this, and I think I have a PR that can be considered. Before I dive in to what the PR contains, I want to document what the current CI processes are across the two services WordPress currently utilizes.
Appveyor
This service is used to test that NPM errors are not encountered on Windows based environments (context: #44276).
The workflow:
- Runs only on pushes to
master
branch. - Installs the NodeJS version specified in the config file (currently
10.13.0
). - Installs
npm
globally. - Runs
npm install
to installpackage.json
dependencies. - Runs
npm run build
to ensure WordPress builds correctly.
Build failures are reported to #core in Slack when a build fails, or when a build succeeds with a previous failure.
Travis CI
The Travis CI configuration has many more jobs within a build. Here are their summaries:
- E2E Tests: Runs the E2E test suite using
npm run test:e2e
. - PHP Linting: Runs the code base through PHPCS rulesets to check that coding standards are followed.
- The
phpcs.xml.dist
ruleset is run against the entire codebase with warnings suppressed (-n
) usingcomposer lint:errors
. - The
phpcs.xml.dist
ruleset is then re-run, but only against thetest
directory without warnings suppressed (warnings are not allowed within the test suite) usingcomposer lint tests
.
- The
- PHP Compatibility: The
phpcompat.xml.dist
ruleset is run against the codebase usingcomposer compat
. - JavaScript Linting & Testing: JSHint linting and QUnit tests are run on the codebase using
npm run grunt travis:js
. - PHPUnit Tests: All tests below are run using
npm run test:php
within the WordPress Docker container unless otherwise noted.- PHP 8.0: The PHPUnit tests are run within the Docker container with
npm run test:php-composer
. This ensures the Composer installed version of PHPUnit (with the necessary modifications made to backport the required parts for PHP 8 support). - PHP 7.4
- PHP 7.3
- PHP 7.3 with memcached
- PHP 7.2
- PHP 7.1
- PHP 5.6
- PHP 8.0: The PHPUnit tests are run within the Docker container with
If any job within the build fails, the build is considered failed and the results are posted to #core in Slack. All failures and successful builds with a previous failure. Pull requests are ignored.
GitHub Actions
In GitHub actions, builds are called "workflows". Each workflow is comprised of jobs, and jobs are comprised of steps with actions (if you're not familiar with GitHub Actions yet, I recommend reading this quick breakdown).
I've set up 7 new workflow files to accomplish all of the current testing performed outlined above.
verify-npm-windows.yml
: Replaces the NPM related testing on Windows currently handled by Appveyor.coding-standards.yml
: Runs the PHP coding standards checks done with PHPCS.end-to-end-tests.yml
: Runs the E2E test suite.javascript-tests.yml
: Runs JSHint linting and QUnit tests.php-compatibility-testing.yml
: Runs the PHP compatibility checks with PHPCS.test-wordpress.yml
: Runs the PHPUnit tests on various versions of PHP.welcome-new-contributors.yml
: Posts a comment welcoming new contributors with some helpful links and resources (when opening their first PR).
Some notes:
- The PHPUnit workflow is broken down into two steps. The first downloads the additional files needed (WordPress Importer, for example), installs NPM dependencies, and builds WordPress before zipping and uploading the result of these actions as an artifact. The artifact is then downloaded in the second step (which expands the matrix of PHP versions to test) before setting up Docker and running the PHPUnit tests. Theoretically, this should be faster (WordPress is built once vs. once for each version of PHP), the ZIP could be downloaded and inspected if there are weird issues with test results, and these artifacts could potentially be useful in some way for someone looking to test a WordPress at a specific commit (they could download the zip into a local environment and just run WordPress without having to run
npm install/npm run build
). This may be overkill though, and these artifacts could be deleted. - PHPCS is currently run from inside the Docker container on Travis. I believe that this was done because the version of PHP (and the environment in general) was unreliable in Travis. In Actions, there is a setup-php action that is more reliable, so I don't think we need to utilize the Docker containers for these scans. This will also not be a problem when we start backporting workflows because coding standards checks were not added to the test suite until WordPress 5.1, which ran the checks using PHP 7.2. This applies to both the coding standards and PHP compatibility workflows.
- The PHP compatibility ruleset explicitly specifies
./src/
as the only directory to run the checks on. However,lint-action
has.
hard coded when invokingphpcs
. I was able to fix this by specifying/tests/
as an exclude pattern within the ruleset. - Unzipping files cannot be performed by an action. The file permissions and ownership did not match the runner's and will cause permission issues with Docker.
There are a handful of items that need to be fixed before these can be merged:
- The
PHP_FPM_UID
andPHP_FPM_GID
environment variables need to be made dynamic. They are currently hard coded. I was having trouble figuring out the correct syntax to set those variables to the results ofid -u
andid -g
,respectively. - The Slack notifications will need to be configured to work similarly to how they do currently for Travis builds for failed workflows. There seem to be a lot of Slack actions in the GitHub Marketplace, so looking for a recommendation form someone that has set these up in another project. This will also require a Slack admin, which @helen should be.
- The test reporter will need to be configured with the necessary API key added as a repository secret.
- For some reason, failures are not always reported on the correct workflow. For example, if coding standards or compatibility checks fail, they frequently are reported on other random workflows run from the same commit.
- Because the lint-action hard codes
.
as the directory to run PHPCS, re-running the ruleset and allowing warnings within just thetest
directory is not currently being done. There is an open pull request on the action's repository to allow this, but I haven't figured out the best way around this. We could introduce a new PHPCS ruleset that uses the one defined withinphpcs.xml.dist
and adds an exclude pattern forsrc
, but that's not really ideal. - There are some issues with some permission issues with actions and forks. I am not clear how these will translate once these workflows are merged. For example, the lint-action I am using states "The action doesn't have permission to create annotations for commits on forks and can therefore not display linting errors." But I'm not sure if the annotations will display within the context of the pull request itself. It's also not clear if these issues will prevent the Welcome workflow from working correctly when a PR is opened from a forked repository.
Here are some other "would be nice" items that don't necessarily need to be solved right now:
- The version of NodeJS/NPM that is installed is currently hard coded as 12. This is fine for now, but this will cause a problem when the workflows are backported to older branches. Ideally, the version specified within
.nvmrc
should be read and installed instead.nvm
is installed in the runner, sonvm install
is an option, but using theactions/setup-node
action seemed like the better path. - The idea behind Appveyor was to perform more regular testing for Windows based environments. The PHPUnit tests could easily be run on both Windows and Ubuntu by adding
windows-latest
to theos
strategy. But there are a few issues that need to be solved first (totally outside the scope for this ticket, but just want to report all of my findings here for future reference):- The
bridge
network driver is not supported in Windows. Excluding that line from thedocker-compose.yml
file seems to work fine (bridge
is the default on Linux machines, and Windows uses a different default), but that then causes an error attempting to pull themysql:5.7
container.
- The
After fixing the blockers outlined above and allowing some time for feedback (this is my first dive into GitHub actions, so I may very well be over complicating something somewhere), I think we could commit these workflows and keep Travis CI/Appveyor as the preferred testing services for a week or so in order to identify any issues.
😅 I think I covered everything I needed to touch on! I'll also upload a patch of the changes here in case someone prefers to look at patches in Trac.
The PR attached to the ticket will not run the workflows. TO see them in action, I have a PR against my fork that will show them running: https://github.com/desrosj/wordpress-develop/pull/2.
#10
@
4 years ago
A few more things that just came to me.
- The JSHint linting should eventually be moved to the coding standards workflow, making the JavaScript workflow strictly about QUnit testing. However, the action I used for the PHPCS linting does not yet support JSHint. There is a ticket to move away from JSHint to ESLint (the docs related JS scans already are, see #43828), but I think we should explore that separately. For the purpose of moving our tests runners to GHA, we can continue to use the Grunt command for now.
- 50401.diff and the PR intentionally contain a small number of PHPCS and PHP compatability issues in order to demonstrate that the workflows are correctly flagging those issues.
#11
@
4 years ago
Replying to desrosj:
- The PHP compatibility ruleset explicitly specifies
./src/
as the only directory to run the checks on. However,lint-action
has.
hard coded when invokingphpcs
. I was able to fix this by specifying/tests/
as an exclude pattern within the ruleset.
I've removed the lint-action
from the workflows. While using maintained actions to setup and run the linting commands is nice, the PHPCS commands are only one line, so it's not unreasonable to hard code those instead of utilizing an action. Hard coding the commands also fixes the issue detailed above where it was impossible to lint just the tests
directory with warnings enabled.
The below issues I mentioned above are also fixed in the latest patch:
- The
PHP_FPM_UID
andPHP_FPM_GID
environment variables need to be made dynamic.- For some reason, failures are not always reported on the correct workflow. For example, if coding standards or compatibility checks fail, they frequently are reported on other random workflows run from the same commit.
The failures not being reported on the correct workflow is due to an issue with how the GitHub checks API works. Checks are attached to commit SHAs, and the failure reports/annotations were being added to the newest workflow run for the commit being tested. By running the commands separate form the lint-action
action, we are able to annotate the commit using a Checkstyle XML-report. The cs2pr
tool can be installed when PHP is set up earlier in the workflow (props @ocean90 for the help figuring that out).
Also in the latest patch:
- NodeJS is now installed before setting up caching for NPM. This ensures the correct version of NPM is cached.
- The events triggering builds have been refined. This will prevent every push on a PR AND the PR itself from running duplicate workflows. I also used expressions to try and scope workflows that should not be run on early branches. For example, the coding standards scans were not added until WP 5.1, and the PHP compatibility scans were not added until WP 5.5.
- I changed the version of PHP that is configured within linting jobs from
latest
to7.4
. This will prevent anything from breaking whenlatest
becomes PHP 8 (just in case our tooling is not ready).
This leaves two items to figure out. They are not blockers for committing these workflows, but they will need to be tackled eventually:
- Slack notifications
- Reading the desired version of NodeJS specified in the
.nvmrc
file.
I played around with reading the .nvmrc
file, but there are some issues. NVM supports aliases such as lts/*
(which is the version specified in newer branches of WP), but the actions/setup-node
does not (and also does not support reading .nvmrc
files). This can be solved later on.
I also did some rough estimating of speeds. It's hard to get 100% accurate comparisons because I have been changing the workflows as I go, but here's how things breakdown:
- E2E tests: almost identical
- PHP compatibility: Roughly 1/2 the time
- JavaScript testing and linting/PHP linting: These are hard to compare because the QUnit tests were split into their own workflow, and the JSHint checks have been merged into a singular coding standards workflow with the PHP checks.
- PHP 8-7.0: Between 3-5 minutes faster.
- PHP 5.6: roughly the same.
4 years ago
#13
Merged into core in https://core.trac.wordpress.org/changeset/49162.
#14
@
4 years ago
- Keywords needs-testing removed
- Milestone changed from Future Release to 5.6
Been monitoring this with @helen the last few hours. There are a few things that we've noticed.
First, the "Welcome" workflow is unable to comment on the PR. It seems this is being caused by some access issues. It seems this can be solved by using pull_request_target
as the event instead of pull_request
. More info can be found on the GitHub Blog.
The second issue is that cancelled workflow runs are considered failures. This is not ideal because it will appear as though the commit associated with the cancelled workflow run introduced a problem to the codebase when that may not be true.
In Travis, builds are placed in a queue among others associated with your account/organization. While there is a queue in GitHub Actions, their usage model is based on minutes, not concurrent jobs. It's unlikely that we'll experience the same bottleneck waiting for jobs to run, so I'm going to remove the "Cancel previous runs of this workflow" steps in the PHPUnit and E2E test workflows.
This ticket was mentioned in PR #620 on WordPress/wordpress-develop by ocean90.
4 years ago
#20
Trac ticket: https://core.trac.wordpress.org/ticket/50401
4 years ago
#21
Thanks! This does look a bit more clear. My only issue is that the memcached job does not include that in the name. So right now it appears 2 identical 7.4 jobs run.
I also have a patch on the Trac ticket that should fix those external HTTP tests. If you could remove the fast fail and add that patch here for testing, that would help verify that.
4 years ago
#22
@desrosj Doesn't seem like 50401.4.diff changes anything but the workflow should be good now.
#27
@
4 years ago
- Type changed from enhancement to task (blessed)
Converting this to a task so we can continue to refine.
This ticket was mentioned in Slack in #core by sergey. View the logs.
4 years ago
#37
@
4 years ago
Thinking about how to limit runs on private mirrors that eat up minutes, I see two options:
- Using an
if: ${{ github.repository == 'WordPress/wordpress-develop' }}
conditional to bail on non-PR triggers - Checking for a secret like
WP_TEST_ALL_EVENTS
(or whatever name) before bailing or proceeding on non-PR triggers
The latter seems easier to manage from a contributor perspective, so forks don't end up having to maintain a diverged version of all the workflow files if they want to run tests on all pushes to branches and instead "just" set the one secret. I don't know if maybe that looks a little strange though, or if we would want to have separate secrets and checks for branch pushes vs. scheduled cron. Also open to other possibilities my 5.6-addled brain has not come up with :)
Apart from that, I think we're probably okay to close out this ticket and open up new ones for issues that come up and for the actual turning off of Travis for core?
#38
@
4 years ago
More on the secret/const idea:
WP_GHA_PR
,WP_GHA_TRUNK
,WP_GHA_BRANCHES
for pull requests, trunk/master, and SVN branches (X.Y) respectively.
I remembered that you can disable individual workflows in the GitHub Actions UI, but I think that does still require an initial run so you then see it in the UI to be able to disable it, which may still be bad for minutes. If we wanted it to be more of an opt-in route, then we could also implement something like:
WP_TEST_ALL
,WP_TEST_PHPUNIT
,WP_TEST_PHPCS
, etc. for test workflows, and decide whether the rest need code level disabling or if they're okay to just disable in the UI.
Most people would then want to set WP_GHA_PR
and WP_TEST_ALL
on their forks. Thoughts?
This ticket was mentioned in PR #761 on WordPress/wordpress-develop by desrosj.
4 years ago
#39
This limits the workflow from running by default on forks, and adds support for 2 secrets per workflow that allows for granular configuration to run workflows as desired:
- One for running the workflow under the same conditions as the mirror
- One for only running on pull requests to the forked repo.
Unfortunately, GitHub Actions does not yet support using secrets
or env
within the context and expression syntax used for if:
conditions, regardless if the env
variables are defined on the workflow, or individual job level.
Trac ticket: https://core.trac.wordpress.org/ticket/50401
This ticket was mentioned in PR #762 on WordPress/wordpress-develop by desrosj.
4 years ago
#40
To prevent unnecessary workflow runs on forks of WordPress/wordpress-develop
, only run workflows if the repository is actually the official mirror repo.
Trac ticket: https://core.trac.wordpress.org/ticket/50401.
#41
@
4 years ago
I've been playing around with this a bit today, and created two PRs.
This one (762) will prevent the workflows from running on any fork of WordPress/wordpress-develop
. We could roll with this temporarily. However, if anyone wishes to run the workflows on their fork, they would have to override the .yml
files and this would introduce the same headache we are trying to avoid.
The second PR (761) is, in my opinion, the ideal solution. It disables the workflows on any repository that is not the official WordPress/wordpress-develop mirror, but also provides the fork owner with two options per workflow.
- Enable all workflows to function on the fork exactly as they would for the mirror. (
WP_GHA_FORK
) - Enable an individual workflow to function exactly on the fork as they would for the mirror. (
WP_*
) - Enable an individual workflow to function for pull requests on the fork as they would for the mirror. (
WP_*_PR
)
These secrets can be defined at the repository or organization level, making it easy to define for just one repo, or throughout the organization (if, for some reason, multiple forks/mirrors of wordpress-develop
are maintained).
It seems secrets
and env
variables are not available to the job specific if:
s within the workflow file. secrets
can be used in steps, but only when provided as an input. While env
context is documented as available to with
and name
keys for a job, and if
conditionals for a step, environment variables defined at the workflow level are not available at the job level.
One workaround could be to add a step to each workflow that happens first, checks for the presence of the secret, and cancels the job. However, this would still result in some workflow minutes being used, as the workflow would have to initialize and proceed to the first step in order to cancel, so that's not 100% ideal.
#42
@
4 years ago
5.6 being branched has also served as a good test scenario for backporting the workflow files. So far, it seems everything is running as expected. The only thing that has yet to be tested is the scheduled cron run for the PHPUnit test workflow, which runs on Sunday evenings at 00:00 UTC.
If we cannot solve the remaining issue by the time 5.6 is released, I am not opposed to spinning that off into its own issue.
#43
@
4 years ago
- Milestone changed from 5.6 to 5.7
Punting this ticket to 5.7 as today is 5.6 RC2.
If any maintainer or committer feels this can be resolved in time, or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.
This ticket was mentioned in Slack in #core by desrosj. View the logs.
4 years ago
#45
@
4 years ago
After giving this more thought, I'm thinking we should proceed by disabling all workflow runs on forks, except those triggered by pull requests. This would only result in the workflows running on the fork if someone opens a pull request to the fork. I sometimes use this workflow to test a PR before submitting it to the official mirror.
Committing directly to a branch of a wordpress-develop fork is not really the best way to go about maintaining a fork for development purposes anyway, unless you truly intend to steer away from the WordPress codebase. In that case, merging updates to the workflow files from the mirror upstream is the least of your problems, as you'll likely experience merge conflicts throughout the branch.
This solves the issue where private mirrors run too many workflows from push events, and still allows collaboration within those private repos.
I've updated https://github.com/WordPress/wordpress-develop/pull/762 with this approach.
4 years ago
#46
@linuxandroid-xyz sorry, I missed your message above previously.
I'm not sure what you are asking or suggesting because there is no context, just an error log dump. Also, I'm not sure how this relates to the code suggestions in this pull request.
4 years ago
#47
Closing in favor of #762. Unfortunately, this level of granularity does not seem like it will work with what's currently available on GitHub Actions.
4 years ago
#49
Merged into WP trunk in https://core.trac.wordpress.org/changeset/49781.
This ticket was mentioned in PR #808 on WordPress/wordpress-develop by helen.
4 years ago
#54
Setting up Slack notifications for GH Actions. Brain dump follows:
- Each workflow is treated separately, not sure how to group all checks per commit the way the GH Slack action does for PRs
(probably need to look at their source)
- Do we still want to only notify on status change? I rather like knowing pass/fail for all commits to trunk and branches but can be convinced otherwise.
See #tmp-notification-testing, didn't want to leave it in a DM so others could collaborate as desired.
Trac ticket: https://core.trac.wordpress.org/ticket/50401
4 years ago
#55
@helen The dist file has a few more replace()
calls. I'm wondering whether the SLACK_WEBHOOK_URL
is actually set since I don't see a masked value in the log.
4 years ago
#56
@ocean90 It was but I deleted and set it again copying it out of my successful curl command and it's still not showing up in the log 🤷🏻♀️
4 years ago
#57
Each workflow is treated separately, not sure how to group all checks per commit the way the GH Slack action does for PRs
This is done by updating an existing message. Each status is basically a new attachment.
I have developed the Slack Messaging action which for example outputs the message_id
so it can be used to update the initial message. This message_id
needs to be available to all workflows which I'm not sure is possible since they may run in parallel. 🤔
This ticket was mentioned in Slack in #core by sergey. View the logs.
4 years ago
#73
@
4 years ago
This is an amazing work, thanks a lot @desrosj and contributors. Chewing several thousands of tests we have in WordPress across several PHP versions is no easy task nor an inexpensive one. Thank you Travis.
This ticket was mentioned in PR #852 on WordPress/wordpress-develop by desrosj.
4 years ago
#82
This PR switches from manually configuring caching for Composer dependencies to using a published action instead. This allows the 3-4 steps used to configure Composer caching to be combined into one.
This also removes the --prefer-dist
(which is now the default) and --no-suggest
(deprecated) flags from Composer commands.
The PHPUnit workflow's Composer caching will remain the same, but the cache key has been adjusted to include the PHP version being used. Currently only the PHP 8.0 job utilizes Composer dependencies, but if other versions run composer install
in the future, it would result in incompatible package versions being loaded as all jobs would currently share a cache key.
Trac ticket: https://core.trac.wordpress.org/ticket/50401.
This ticket was mentioned in Slack in #core by sergey. View the logs.
4 years ago
#136
@
4 years ago
Automated testing has been restored for all branches. Some notes:
- I elected to include all workflow files, even if they are not used in the branch. This made merging the changesets to older branches much easier (it avoids conflicts when the files are missing). The workflows will not run because of the way
branches
are scoped under thepush
trigger event. However, thepull_request
will need to be removed from workflows that do not apply to certain branches if these files will remain. Otherwise, they will run and fail for allpull_request
events. - The PHP 5.2 job is missing from branches that support that version. This is blocked by getting PHPUnit 3.6 to install within the PHPUnit Docker container for PHP 5.2. See https://github.com/WordPress/wpdev-docker-images/pull/46 for more information.
- I decided to test all versions of PHP supported in each branch. My thinking was that there should be a successful test job on record for every combination to prove the transition worked. When PHP 5.2 is added to the matrix, these can also be trimmed appropriately to limit unnecessary builds.
#137
@
4 years ago
One last item: Slack notifications still need to be configured. Once that is configured, it can be backported to all older branches.
This ticket was mentioned in PR #1013 on WordPress/wordpress-develop by desrosj.
4 years ago
#138
This updates the following GHA actions:
actions/setup-node
to version 2.styfle/cancel-workflow-action
to version 0.8.0.
Trac ticket: https://core.trac.wordpress.org/ticket/50401
This ticket was mentioned in Slack in #core by desrosj. View the logs.
4 years ago
4 years ago
#141
Merged into Core in https://core.trac.wordpress.org/changeset/50387.
FWIW I've found GitHub Actions run almost exactly at the same speed as Travis (but the tests for my plugins don't typically get backed up like WordPress core tests do). Definitely worth experimentation.