Run tests via GitHub Actions by pierlon · Pull Request #5201 · ampproject/amp-wp

pierlon · 2020-08-09T22:09:20Z

Summary

Migrates all lint and test jobs from Travis to GHA.

Checklist

My pull request is addressing an open issue (please create one otherwise).
My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

github-actions · 2020-08-11T17:04:44Z

Plugin builds for 9568f67 are ready 🛎️!

Download development build
Download production build

CLAassistant · 2020-10-08T22:07:59Z

All committers have signed the CLA.

johnwatkins0

@pierlon This is very impressive. My review mainly consists of questions rather than requests for changes. It looks like it's all working, which is awesome.

.github/workflows/build-test-measure.yml

johnwatkins0 · 2020-10-26T19:03:50Z

.github/workflows/build-test-measure.yml

+        run: echo "::add-matcher::${{ runner.tool_cache }}/phpunit.json"
+
+      - name: Set up tests
+        # TODO: Add step to install plugins instead of relying on .dev-lib


Correct me if I'm wrong, but couldn't we do this now by moving the code from .dev-lib into install-wp-tests.sh? It seems like this PR gets us 90% of the way toward removing the wp-dev-lib dependency. Can we get to 100%?

Good question. The original intention was to remove the wp-dev-lib dependency but I realized it is being used for generating the readme (@westonruter any other uses I'm missing?) which also relies on the .dev-lib file, so I didn't spend much time looking into migrating the installation of the plugins just yet. I suppose that still can be done and then we could leave the .dev-lib file in place until all use cases for it are migrated.

bin/install-wp-tests.sh

johnwatkins0 · 2020-10-26T19:08:25Z

.github/workflows/test-external-libs.yml

@@ -0,0 +1,64 @@
+name: External Libraries


Curious, why are these tests in a separate configuration? Shouldn't these also depend on the preceding linting/testing?

Technically these libs aren't apart of the plugin so these tests shouldn't be impeded by the plugin's lint/tests jobs (unless I'm missing something here).

/cc @schlessera

I'm currently working on extracting these libraries out (https://github.com/ampproject/amp-toolbox-php), so we shouldn't waste too much thought on these. I will rebuild GHA testing on that other repo.

The tests are already running on the other repository now:

So there's no need to do any work on them here anymore.

Awesome! WIll remove the workflow then.

pierlon · 2020-10-27T00:06:20Z

The current approach here is to run lint jobs first, then if those succeed it moves on to running the tests, then it builds the plugin for deployment to GCS and finally comments on the PR with links for downloading those builds. One major drawback with this approach is that if any of these jobs fail for whatever reason, the whole workflow has to be re-run (i.e. if a test job fails, all lint jobs have to be re-run irregardless of their outcome).

The only alternative I see so far is to split the relevant jobs into separate workflows. That allows for easier re-run of jobs if they fail, but it also means that all jobs would be running in parallel as there is currently no way for separate workflows to depend on one another (i.e. all workflows would be running in parallel).

Any thoughts on this? Should we keep the linear approach and have one workflow with all the jobs, or split the relevant jobs in different workflows and have all the jobs run asynchronously to allow for easier re-run of failed jobs?

johnwatkins0 · 2020-10-28T16:18:08Z

if any of these jobs fail for whatever reason, the whole workflow has to be re-run

@pierlon Do you mean in the case that some technical issue in the action itself causes the build to stop? I don't think we would expect that case to happen frequently, right? In that case, it might be annoying to have to restart the entire process when the failure was extrinsic to the code change, but it would also indicate to us that the action workflow might not be stable enough and may need attention, which is useful.

To me it makes sense to have all the jobs that depend on each other run in sequence, with the entire sequence needing to start over if linting or tests fail.

pierlon · 2020-10-28T21:30:23Z

Do you mean in the case that some technical issue in the action itself causes the build to stop?

@johnwatkins0 Yep that's what I meant. That would also include flaky tests we sometimes encounter, but yes those should be happening quite infrequently from now on.

I also prefer having the jobs run in sequence, but just wanted to make it known that the inability to re-run a separate job is currently not a feature on GHA.

This reverts commit 2240e1b

pierlon · 2020-11-03T23:24:16Z

⚠️ Rebasing with latest changes from develop.

westonruter

Amazing work. You are a GitHub Action whisperer. Just a couple questions:

westonruter · 2020-11-07T19:17:56Z

.github/workflows/build-test-measure.yml

+            ${{ runner.os }}-npm-
+
+      - name: Install Node dependencies
+        run: npm ci


TIL npm ci: https://docs.npmjs.com/cli/v6/commands/npm-ci

westonruter · 2020-11-07T19:58:42Z

.github/workflows/build-test-measure.yml

+    needs:
+      - lint-js


Why do JS unit tests depend on linting?

This can be removed as well.

westonruter · 2020-11-07T19:58:55Z

.github/workflows/build-test-measure.yml

+    needs:
+      - lint-js


Why do E2E tests depend on linting?

No definitive reason, this can be removed.

westonruter · 2020-11-07T19:59:24Z

.github/workflows/build-test-measure.yml

+    needs:
+      - lint-php
+      - static-analysis-php


Why does PHPUnit depend on linting and static analysis?

This can be removed as well.

pierlon

Thanks @westonruter. See 9568f67 for the amendments made.

pierlon · 2020-11-07T20:17:14Z

.github/workflows/build-test-measure.yml

+    needs:
+      - lint-js


No definitive reason, this can be removed.

pierlon · 2020-11-07T20:17:34Z

.github/workflows/build-test-measure.yml

+    needs:
+      - lint-php
+      - static-analysis-php


This can be removed as well.

pierlon · 2020-11-07T20:17:43Z

.github/workflows/build-test-measure.yml

+    needs:
+      - lint-js


This can be removed as well.

westonruter

I can't claim to certify all the GH Action code, but it looks good to me!

google-cla bot added the cla: yes Signed the Google CLA label Aug 9, 2020

pierlon changed the title ~~Run tests via GitHub Actions~~ Aug 9, 2020

pierlon force-pushed the add/test-workflow branch 2 times, most recently from b5e757e to 6714969 Compare August 11, 2020 16:54

pierlon added the WS:Core Work stream for Plugin core label Aug 12, 2020

pierlon marked this pull request as draft August 13, 2020 07:49

pierlon changed the title ~~[WIP] Run tests via GitHub Actions~~ Aug 13, 2020

pierlon force-pushed the add/test-workflow branch 4 times, most recently from cddb1c6 to 2df4fe7 Compare October 25, 2020 08:28

pierlon requested review from johnwatkins0, schlessera and westonruter October 25, 2020 08:58

pierlon added Testing Issues related with Unit, E2E, Smoke, and other testing requirements/needs Infrastructure Changes impacting testing infrastructure or build tooling labels Oct 25, 2020

pierlon self-assigned this Oct 25, 2020

pierlon added this to the v2.1 milestone Oct 25, 2020

pierlon marked this pull request as ready for review October 25, 2020 08:59

pierlon mentioned this pull request Oct 26, 2020

Migrate lint jobs from Travis to GHA #5225

Closed

3 tasks

johnwatkins0 reviewed Oct 26, 2020

View reviewed changes

pierlon force-pushed the add/test-workflow branch from ba4cc07 to f040121 Compare October 29, 2020 16:49

pierlon added 3 commits November 3, 2020 18:20

Add lint jobs

a115094

Fix workflow syntax

9ef4eba

Add reporting for static analysis

2407314

pierlon added 11 commits November 3, 2020 18:21

Simply condition for running 'prod zip' job

e0f4ede

Simplify build jobs

15f2969

Update link in comment

9ed843e

Remove workflow to test external libs

ca60ff2

Move post WP install function to its own file

519ab4f

Remove xwp/wp-dev-lib dependency

f9cba6f

Move phpstan step to its own job

3a25922

Only require interdepent jobs to run job

ea587ac

Revert "Remove xwp/wp-dev-lib dependency"

fe94c35

This reverts commit 2240e1b

Re-add .dev-lib

e5cc08c

Lock ergebnis/composer-normalize to v2.9.0

8ccb12c

pierlon force-pushed the add/test-workflow branch from 70a7736 to 8ccb12c Compare November 3, 2020 23:24

pierlon added 3 commits November 3, 2020 18:30

Update composer.lock

53e461b

Update PHPUnit patch for PHP 8

f6269b9

Remove custom logo override set by Gutenberg

3f4b81b

pierlon force-pushed the add/test-workflow branch from 85f6cb1 to 3f4b81b Compare November 7, 2020 11:33

Set WP_TESTS_DIR var

4b62121

pierlon requested a review from johnwatkins0 November 7, 2020 11:40

westonruter reviewed Nov 7, 2020

View reviewed changes

Remove dependencies for E2E, PHP & JS unit tests

9568f67

pierlon commented Nov 7, 2020

View reviewed changes

westonruter approved these changes Nov 7, 2020

View reviewed changes

westonruter merged commit a5d221b into develop Nov 7, 2020

westonruter deleted the add/test-workflow branch November 7, 2020 20:28

pierlon mentioned this pull request Nov 11, 2020

Update Codecov configuration #5341

Merged

3 tasks

westonruter mentioned this pull request Dec 6, 2020

Eliminate Travis in favor of GitHub Actions for linting and testing GoogleChromeLabs/pwa-wp#354

Merged

jwold unassigned pierlon Apr 23, 2021

westonruter mentioned this pull request Apr 30, 2022

Add wp-env and utilize for running PHPUnit locally GoogleChromeLabs/pwa-wp#778

Merged

westonruter mentioned this pull request Oct 26, 2022

PHP 8.1 / 8.2 Compatibility #7225

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run tests via GitHub Actions#5201

Run tests via GitHub Actions#5201
westonruter merged 60 commits intodevelopfrom
add/test-workflow

pierlon commented Aug 9, 2020 •

edited

Loading

github-actions bot commented Aug 11, 2020 •

edited

Loading

CLAassistant commented Oct 8, 2020 •

edited

Loading

johnwatkins0 left a comment

Uh oh!

Uh oh!

johnwatkins0 Oct 26, 2020

pierlon Oct 26, 2020

Uh oh!

johnwatkins0 Oct 26, 2020

pierlon Oct 27, 2020

schlessera Oct 29, 2020

schlessera Oct 29, 2020

pierlon Oct 29, 2020

pierlon commented Oct 27, 2020

johnwatkins0 commented Oct 28, 2020 •

edited

Loading

pierlon commented Oct 28, 2020

pierlon commented Nov 3, 2020

westonruter left a comment

westonruter Nov 7, 2020

westonruter Nov 7, 2020

pierlon Nov 7, 2020

westonruter Nov 7, 2020

pierlon Nov 7, 2020

westonruter Nov 7, 2020

pierlon Nov 7, 2020

pierlon left a comment

pierlon Nov 7, 2020

pierlon Nov 7, 2020

pierlon Nov 7, 2020

westonruter left a comment

Labels

5 participants

Conversation

pierlon commented Aug 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

github-actions bot commented Aug 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CLAassistant commented Oct 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

johnwatkins0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

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

pierlon commented Oct 27, 2020

johnwatkins0 commented Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

pierlon commented Oct 28, 2020

pierlon commented Nov 3, 2020

westonruter 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

pierlon 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

westonruter left a comment

Choose a reason for hiding this comment

Labels

5 participants

pierlon commented Aug 9, 2020 •

edited

Loading

github-actions bot commented Aug 11, 2020 •

edited

Loading

CLAassistant commented Oct 8, 2020 •

edited

Loading

johnwatkins0 commented Oct 28, 2020 •

edited

Loading