CI: switch to GitHub Actions

[jrfnl]

Important

This PR can't get merged until the required statuses have been adjusted, but as there is another open PR from earlier, changing the required statuses at this time would make that PR unmergable.

Please do review this PR though. Once there are two approvals, I will update the required statuses and merge this PR.
If the other PR hasn't been merged yet by that time, I will rebase it to make it mergable again.

Testing

No testing is needed. Extensive testing has already been done and can be verified by checking the (deliberately) failed builds on the Actions page.

Commit details

CI: switch to GitHub Actions - step 1: sniff stage

This commit:

• Adds a GH Actions workflow for the CI checks which were previously run on Travis in the sniff stage.
  While these aren't 100% CS (= code style) checks, for the workflow display, CS still seemed the most descriptive name.
  Note: includes moving the composer validate check to this workflow.
• Removes these parts of the .travis.yml configuration.

Notes:

1. Builds will run on all pushes and on pull requests, with the exception of those just modifying files which are irrelevant to this workflow.

CI: switch to GitHub Actions - step 2: quick test stage

This commit:

• Adds a GH Actions workflow for the CI checks which were previously run on Travis in the quicktest stage.
• Removes that part of the .travis.yml configuration.

Notes:

1. Previously, this "stage" would run on all push events, except when merging to master or develop.
   The current set-up still does so, with one exception: pushes to develop will now use this quicktest instead of the full test.
   develop is a protected branch anyhow, so all merges will already have had the full test run in the pull request.
   For merges to master, the full Test will still be used as that will generally mean we're getting ready to tag a release and some extra safety checking before a release is not a bad thing.
2. This also updates the "high" PHP version for the "quicktest" against PHPCS dev-master from PHP 7.4 to PHP 8.0 by using latest which translates to "latest stable PHP release".

CI: switch to GitHub Actions - step 3: test stage

This commit:

• Adds a GH Actions workflow for the CI checks which were previously run on Travis in the test stage.
• Removes the, now redundant, .travis.yml configuration.
• Updates the .gitattributes file.

Notes:

1. Previously, this "stage" would run on all pull requests events.
   The current set-up still does so, with one addition: pushes to master (merges) will now also use this workflow instead of the quicktest.
   This replaces the full run on tagging a release, meaning that things will essentially be the same.
2. As there are a couple of jobs which are "allowed to fail" (experimental = true), the build status may unfortunately show as "failed", even though all non-experimental jobs have succeeded.
   This is a known issue in GHA: Please support something like "allow-failure" for a given job actions/runner#2347
   For now, two of three experimental builds have (temporarily) been disabled
   • The WPCS build against dev-develop is disabled as the next WPCS will be a major release containing breaking changes, so that build won't be able to pass until YoastCS has been updated for this and updating YoastCS for this only makes sense once WPCS 3.0.0 has stabilized.
   • The build against PHPCS 4.x is disabled as there are a couple of bugs in it, aside from the expectation that it won't be released for another year or so, so we may as well wait with testing against it until it has stabilized more.

[jrfnl]

Note: the current build failure against PHP 8.1 was expected - this is an upstream issue with the PHP Parallel Lint package and I've already pulled a fix there: php-parallel-lint/PHP-Parallel-Lint#64

[moorscode]

Looking great, perhaps I'm more used to Travis configs.. these GitHub Action files are a bit harder to read through.

Just a couple of questions and suggestions.

[moorscode]

Same as before, could php be php_version?

[moorscode]

If a lint variable is added to the matrix, this condition would be a lot smaller and simpler.

[jrfnl]

True, but it would mean you'd have to add the lint key to every build in the matrix and make sure not to forget to toggle it to the right value and keep it in sync with the condition now used, when adding new builds, while now things are handled automatically.

In other words: yes, the condition would be cleaner, but the matrix would not be and the script would become more prone to human error if someone less versed in GH Actions would edit the script.

[jrfnl]

Note: this may not seem like a big thing for the limited matrix currently used by this repo, but the matrices for PHPCS repos can get quite complicated - see https://github.com/PHPCSStandards/PHPCSUtils/blob/develop/.github/workflows/test.yml#L41-L124 for an example.

[jrfnl]

Another remark against adding a lint key: it would also force us to pull the matrix in the test script apart.

The matrix in the test script currently looks like this:

        php_version: ['5.4', '5.5', '5.6', '7.0', '7.1', '7.2', '7.3', '7.4', '8.0']
        phpcs_version: ['3.6.0', 'dev-master']
        wpcs_version: ['2.3.0', 'dev-master']
        experimental: [false]

This is automatically expanded to give use four builds against every PHP version.

If I were to add a lint key, the default would have to be false as we only want it to be applied to the dev-master (PHPCS + WPCS) builds so it will only run once for every PHP version.

In effect this means that I'd have to add the complete set of builds to which that key should be applied (true) in the include section making the matrix much more wordy and less intuitive to read and manage as I'd be changing builds added in the matrix via include, instead of adding new builds via include (both are possible in GH Actions).

In that case, what I'd much rather propose would be to remove the linting from this build and add a separate script just to do the linting.

[moorscode]

Do we want to wait with merging until the linting issue is resolved in the php-parallel-lint package?

[jrfnl]

Do we want to wait with merging until the linting issue is resolved in the php-parallel-lint package?

I'd recommend not to wait on PHP Parallel Lint. The issue only affects PHP 8.1, which hasn't been released yet, won't be a required status until it has been released and is marked as an "experimental" build.

There is a downside to merging it with the error still happening though:
Travis would report one result for each complete run and that result would take "allowed to fail" into account, so would show 🟢 green when there were only failures in "experimental" builds.

GH Actions - as can be seen in the box at the bottom of this PR - reports back on every individual build.
This means that the status on the PR overview may show as ❌ "failed", even though the only failure is on a build which is currently not required and set to "allowed to fail" (experimental). So, reviewers will always need to check the status block at the bottom of a PR to verify whether the build which failed was an "allowed to fail" build.

Also see my remark about this in the "Step 3" commit message and the link to the open issue about this with GH Actions.

[jrfnl]

@moorscode I've added a few commits based on your feedback. Left those as separate commits for easier review. I propose I squash them into the original commits once you've had a chance to look at the updates, so as to keep the commit history clean.

Re: the linting - have a look my feedback above and let me know what you'd prefer. I think splitting the linting off from the testing (at least for the test script, not so much the quicktest) would be a good option.

[moorscode]

Looking good, you have convinced me the lint variable isn't a realistic option.

[diedexx]

Looks good to me 😄
While it takes some getting used to to read the GH actions config, I really like how easy it is to do things like cs2pr and job matrixes with it.

@jrfnl you mentioned having to update the required statuses. I think that can be done now 🙂

[jrfnl]

Thanks @diedexx ! I've squashed the extra commits I made based on the review back into the original commits now and pushed the branch again. That should now allow me to get the status checks setup. Will merge once the build has passed.

Once this PR has been merged, I will rebase all other open PRs as due to the change in the required statuses, they will no longer be mergable unless rebased.