Move CI to GitHub Actions #814

dlh01 · 2021-11-17T06:21:15Z

This Pull Request migrates Fieldmanager's continuous integration runner to GitHub Actions. Along the way, it migrates the PHPUnit and PHPCS infrastructure to be Composer-based instead of Grunt-based, and it brings the PHP tests and linting back up to passing.

The highlights:

New minimum versions

The minimum supported versions of PHP and WordPress are now 7.4 and 5.8, respectively. (WordPress 5.9 is due in early 2022, and "supporting" 5.7 for the next couple of months didn't seem worthwhile.)

Migration of dependencies to Composer

Previously, PHPUnit was installed as part of setting up the virtual machine in Travis and called directly, and Grunt was used to initiate PHPCS. Both are now installed and run through composer.json for simplicity, closer alignment with Alley's internal practices, and because the JavaScript unit tests that have kept the Grunt config alive are in such disrepair that they're dragging the PHP checks with them.

QUnit tests are disabled

See previous section. These haven't worked properly for such a long time that keeping them on seems disingenuous. They should be fixed, but someone needs to do that, and the rest of Fieldmanager needn't wait, I think.

New GitHub Actions workflows for running PHPUnit and PHPCS

PHPUnit runs against the latest version of WordPress, in single-site and multisite mode, on PHP 5.6, 7.4, and 8.0, a small reduction in coverage from the Travis workflow, which tested down to PHP 5.3 and WordPress 4.7.6, and up to WordPress trunk. This reduction is meant to reflect the slower pace of Fieldmanager development over the last couple of years, as well as the fact that the minimum PHP version for WordPress is now 5.6.

Migration to Alley Coding Standards

We have been using our own PHPCS ruleset since early 2020. Adopting it here brings consistency for developers internally and ensures that new Fieldmanager code reflects our standards.

A new PHPCS "baseline"

The WordPress coding standards have evolved quite a bit since our last heave to bring Fieldmanager in line as part of versions 1.2 and 1.1.

This PR again brings Fieldmanager into coding standards compliance through the use of a "baseline" provided by https://github.com/isaaceindhoven/php-code-sniffer-baseliner. The baseline provides phpcs:ignore comments for every current PHPCS violation, allowing the current code to pass without disabling files or sniffs for future code.

After running the baseliner, I fixed the cosmetic violations and swapped in preferred functions (gmdate(), wp_json_encode()) where appropriate. All other ignore lines should be either rewritten versions of old-style WPCS: comments or longstanding Fieldmanager code that will be difficult to fix and certainly not in scope here, like loose-equality comparisons.

Done with https://github.com/isaaceindhoven/php-code-sniffer-baseliner

montchr

Good stuff.

I requested one change – marked with ⚠️ – because it looks like the PHP version matrix for the PHPUnit workflow is not being used.

montchr · 2021-12-06T21:11:47Z

.github/workflows/phpcs.yml

+    strategy:
+      fail-fast: true


Suggested change

strategy:

fail-fast: true

fail-fast defaults to true, so this isn't strictly necessary.

https://docs.github.com/en/actions/learn-github-actions/workflow-syntax-for-github-actions#jobsjob_idstrategyfail-fast

.github/workflows/phpcs.yml

.github/workflows/phpunit.yml

fieldmanager.php

.github/workflows/phpunit.yml

montchr · 2021-12-06T21:58:04Z

phpcs.xml

+
+	<!-- Include Alley Rules -->
+	<rule ref="Alley-Interactive">
+		<!-- See #659. -->


I'm not following based on the context provided in that issue. Why is long array syntax still allowed?

If one of the goals of this PR is to follow Alley coding standards, then this seems like a better time than any to enforce Generic.Arrays.DisallowLongArraySyntax and automatically apply the required fixes with phpcbf.

Just seemed out of scope. It probably would wipe out a lot of open PRs, too. But I'm not totally against it if folks think now is the time.

#659 is a citation to show that we have, in fact, required long syntax. Also #662.

tests/php/bootstrap.php

Co-authored-by: Chris Montgomery <[email protected]>

Since there aren't any runtime dependencies that would need to be locked, and since we want CI runners to install the versions of dependencies appropriate to whatever version of PHP they're running, it's not clear what the Composer lockfile would be locked to.

This reverts commit 0a7f9d1.

dlh01 · 2021-12-29T22:53:07Z

After @montchr astutely pointed out my misunderstanding of how GitHub Actions work, I think the tests are now actually running on PHP 5.6 and 8.0, not just 7.4.

This process brought in yet more changes for compatibility with newer versions of PHPUnit and with the forthcoming WordPress 5.9, which can be seen here: 07a23e4...2f7cf34

Most notably, Fieldmanager tests cases are now structured as extensions of the test case classes provided by the PHPUnit Polyfills library, rather than PHPUnit itself. The visible effect of this change is that setUp() and tearDown() have been replaced with set_up() and tear_down(), the same change as has already happened in WordPress core. See the technical dev note on Make/Core.

Under the hood, PHPUnit 5 or 7 is installed by Composer for PHP 5.6 or PHP 7.4+, respectively. (PHPUnit 7 doesn't technically support PHP 8 but can be installed anyway using --ignore-platform-reqs.)

PHPUnit assertions used in the tests that are deprecated or removed in PHPUnit 7+ have been updated, such as migrating assertContains to assertStringContainsString. A few minor changes to the tests themselves were also required, such as fixing a test that unnecessarily relied on a loose type comparison.

We should be in a position to adopt PHPUnit 9 once Fieldmanager's minimum supported version of WordPress is 5.9 or greater — WordPress 5.8 supports only PHPUnit 7. At that time, we'll also be able to remove PHPUnit from our composer.json entirely, and allow PHPUnit Polyfills to install the highest-supported version, as described in the dev note.

renatonascalves

🗡️

Left a few suggestions!

renatonascalves · 2022-01-03T23:57:40Z

.github/workflows/phpcs.yml

+on:
+  pull_request:


You can also set up as such:

Suggested change

on:

pull_request:

on: pull_request

This comes from other Alley projects, and I'm inclined to keep it consistent with those so that it's easier to pick up the Fieldmanager config if you're used to other Alley defaults.

renatonascalves · 2022-01-03T23:59:22Z

.github/workflows/phpcs.yml

+        uses: shivammathur/setup-php@v2
+        with:
+          php-version: '7.4'
+          extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, sqlite, pdo_sqlite, gd


Are we sure we need to add all those extensions?

No, I'm not sure. Going with the flow of defaults used in other Alley projects here as well. I'm happy for someone who knows more about where those came from to make adjustments to this list, but I'm not personally comfortable trying to do that yet.

renatonascalves · 2022-01-04T00:00:20Z

.github/workflows/phpcs.yml

+          max_attempts: 5
+          command: composer install
+
+      - name: Run PHPCS


Recommend validating the composer package with:

- name: Validate Composer run: composer validate --strict

This happens in phpunit.yml, although I suppose it makes more sense here so it runs once instead of on each of the PHPUnit variations.

renatonascalves · 2022-01-04T00:01:30Z

.github/workflows/phpcs.yml

+          access_token: ${{ github.token }}
+
+      - name: Check out code
+        uses: actions/checkout@v2


Another action suggestion:

- name: Check Gitignored files run: if [[ ! -z $(git ls-files -i --exclude-standard) ]]; then exit 1; fi

Is this something that is likely to be an issue? What would a developer have to do to cause it?

renatonascalves · 2022-01-04T00:05:02Z

php/context/class-fieldmanager-context-menuitem.php

@@ -108,6 +108,8 @@ public function add_fields( $item_id ) {

 		// Render the field.
 		$this->render_field();
+
+		return $item_id;


The method could use an update to introduce the new @return value.

…nto fix/github-tests

dlh01 added 4 commits November 17, 2021 01:13

Update PHPUnit configuration file for newer requirements

15d6d49

Run PHPUnit with Composer

1d8ca22

Move PHPUnit run to GitHub Actions

de40c15

Refresh specs in README

0ce6e35

dlh01 assigned dlh01 and unassigned dlh01 Nov 17, 2021

dlh01 added 24 commits November 17, 2021 01:26

Add description and license for Composer validation

6703f11

Fix typo

9e57c14

Remove unused argument

91eda5b

Begin migration to Alley PHPCS standard

78e98bc

Modernize .gitignore

b2553cd

Create a "baseline" for PHPCS compliance

28187ff

Done with https://github.com/isaaceindhoven/php-code-sniffer-baseliner

Use wp_json_encode()

c3bfb36

Use gmdate()

4798367

Fix indent

308d3ca

Use newer ignore syntax

2dee47f

Fix spacing

11ca18a

Fix spacing

69fbf00

Remove provisional comment

7bf07cc

Use strict search

6d2c645

Fix spacing

87e6d5f

Use gmdate()

43bbf9d

Fix spacing

9139d0a

Fix spacing

db4ffba

Fix spacing

ac8221d

Fix spacing

77563ae

Fix docs

accc272

Fix docs

d9a54e8

Fix docs

4d1be92

Fix missing return from filter

0aac52d

dlh01 requested a review from emilyatmobtown November 23, 2021 01:15

montchr suggested changes Dec 6, 2021

View reviewed changes

Fix PHP version

07a23e4

Co-authored-by: Chris Montgomery <[email protected]>

montchr approved these changes Dec 7, 2021

View reviewed changes

dlh01 added 17 commits December 29, 2021 00:14

Let PHPUnit Polyfills library manage PHPUnit installation

f3387d2

Handle strict type check

984ab1f

Migrate phpunit.xml to PHPUnit 9 format

0a7f9d1

Make test cases compatible with PHPUnit 9

f3eaed3

Remove duplicative const

69563e3

Revert "Migrate phpunit.xml to PHPUnit 9 format"

8a0a171

This reverts commit 0a7f9d1.

Move to PHPUnit Polyfills' set_up and tear_down fixtures

4607a4f

Pin to PHPUnit 7.x for continued support of WordPress 5.8

af42fd4

Cache based on PHP version now that lockfile is gone

c9f2740

Allow PHPUnit 5 to be installed for PHP 5.6

556c253

Remove void return type hints for PHP 5.6

db9dd9e

Allow PHPUnit 7 to be installed on PHP 8

d6cee60

Allow PHPUnit 7 to be installed on PHP 8

03a12b5

Use retry that supports 'new_command_on_retry'

b59b2bb

Avoid getMockForAbstractClass() while not yet on PHPUnit 9

2f7cf34

Merge branch 'main' into fix/github-tests

eb1adf5

renatonascalves approved these changes Jan 4, 2022

View reviewed changes

dlh01 added 3 commits January 3, 2022 22:58

Fix docs

cf7c34e

Run Composer validation only once

33a9292

Merge remote-tracking branch 'refs/remotes/origin/fix/github-tests' i…

59cad01

…nto fix/github-tests

dlh01 merged commit dabdd12 into main Jan 6, 2022

dlh01 deleted the fix/github-tests branch January 6, 2022 01:23

dlh01 added this to the 1.4.0 milestone Jan 6, 2022

dlh01 mentioned this pull request Aug 16, 2022

Add initial functionality and tests alleyinteractive/wp-match-blocks#1

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move CI to GitHub Actions #814

Move CI to GitHub Actions #814

dlh01 commented Nov 17, 2021 •

edited

Loading

montchr left a comment

montchr Dec 6, 2021

montchr Dec 6, 2021

dlh01 Dec 6, 2021

dlh01 Dec 6, 2021

dlh01 commented Dec 29, 2021

renatonascalves left a comment

renatonascalves Jan 3, 2022

dlh01 Jan 4, 2022

renatonascalves Jan 3, 2022

dlh01 Jan 4, 2022

renatonascalves Jan 4, 2022

dlh01 Jan 4, 2022

renatonascalves Jan 4, 2022

dlh01 Jan 4, 2022 •

edited

Loading

renatonascalves Jan 4, 2022

Move CI to GitHub Actions #814

Move CI to GitHub Actions #814

Conversation

dlh01 commented Nov 17, 2021 • edited Loading

New minimum versions

Migration of dependencies to Composer

QUnit tests are disabled

New GitHub Actions workflows for running PHPUnit and PHPCS

Migration to Alley Coding Standards

A new PHPCS "baseline"

montchr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlh01 commented Dec 29, 2021

renatonascalves left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlh01 Jan 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlh01 commented Nov 17, 2021 •

edited

Loading

dlh01 Jan 4, 2022 •

edited

Loading