Skip to content
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

Migrate Travis CI tasks to GitHub Actions #1040

Merged
merged 13 commits into from Oct 8, 2021
Merged

Conversation

eason9487
Copy link
Member

@eason9487 eason9487 commented Oct 6, 2021

Changes proposed in this Pull Request:

Somehow seems all repos under Woo were stopped CI tasks on Travis CI. This PR migrates CI tasks to GitHub Actions:

  • Run PHP unit tests
  • Run JavaScript unit tests
  • Check PHP coding standards
  • Lint JavaScript and CSS
  • Check bundle size
  • Add status badges to README

Additional notes

Since the WC_VERSION in install-wp-tests.sh doesn't take from a command param, the WC_VERSION of matrix values in .travis.yml have never been used when running PHP unit tests on Travis CI. Therefore, I didn't migrate the matrix of specified WC_VERSION such as WC_VERSION=5.2.

- WC_LATEST=trunk
matrix:
- WP_VERSION="${WP_LATEST}" WC_VERSION="${WC_LATEST}"
- WP_VERSION="${WP_LATEST}" WC_VERSION=5.2
- WP_VERSION=5.7 WC_VERSION=5.2
- WP_VERSION=5.6 WC_VERSION=5.2

Screenshots:

image

Detailed test instructions:

Go to the Checks tab for details

Changelog entry

@eason9487 eason9487 self-assigned this Oct 6, 2021
@eason9487 eason9487 changed the title Migrate Travis CI tasks to GitHub ActionsAdd/GitHub actions Migrate Travis CI tasks to GitHub ActionsAdd Oct 6, 2021
@eason9487 eason9487 changed the title Migrate Travis CI tasks to GitHub ActionsAdd Migrate Travis CI tasks to GitHub Actions Oct 6, 2021
@tomalec
Copy link
Member

tomalec commented Oct 6, 2021

WDYT of using the node version from .nvmrc? So we will not hardcode it in so many places, and have to manually change on every update?
actions/setup-node#32

@eason9487 eason9487 requested a review from a team October 6, 2021 09:46
@eason9487 eason9487 marked this pull request as ready for review October 6, 2021 09:46
@eason9487
Copy link
Member Author

WDYT of using the node version from .nvmrc? So we will not hardcode it in so many places, and have to manually change on every update?
actions/setup-node#32

Currently, there should be only one place that uses a hardcode node version:

- name: Set up Node
uses: actions/setup-node@v2
with:
node-version: "12.21.0"
cache: "npm"

The reason is that I recalled several discussions about the node version used for building a bundle, and from slack I found that node 12.21.0 was mentioned a few times, so I specified it in the bundle-size action:

  • p1622120832186300-slack-C01E9LB4X61
  • p1622194602214700-slack-C01E9LB4X61

If that's not an issue anymore, then we could remove the actions/setup-node step in the bundle-size action, because in the runs-on: ubuntu-latest environment, the built-in node is already the LTS version.

@tomalec
Copy link
Member

tomalec commented Oct 6, 2021

I'd rather make sure it's set to the exact same version as https://github.com/woocommerce/google-listings-and-ads/blob/develop/.nvmrc, which is the version that was used on Travis before, but more importantly, that's the version we use locally to build (if we use NVM). Which has the benefit of keeping all the parties at the same version, and a single point to set it up.

If the issue with building with lts would pop up, we should rather fix the issue, not downgrade, not to makes lag too far behind.

@tomalec
Copy link
Member

tomalec commented Oct 6, 2021

Since the WC_VERSION in install-wp-tests.sh doesn't take from a command param, the WC_VERSION of matrix values in .travis.yml have never been used when running PHP unit tests on Travis CI. Therefore, I didn't migrate the matrix of specified WC_VERSION such as WC_VERSION=5.2.

I recently learned (naturally from reading the code, not from the docs 😬), that WC_VERSION is the only way to set the WC version for E2E test, so I think we would need them once we will run E2E against a range of versions. (For example to mitigate Dependency Extraction Plugin related bugs)

Edit: but adding E2E tests to CI could be a separate PR.

Comment on lines 23 to 30
- name: Get npm cache directory
uses: actions/cache@v2
with:
path: ~/.npm
key: ${{ runner.os }}-node-${{ hashFiles('package-lock.json') }}
restore-keys: |
${{ runner.os }}-node-

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💅 I wonder If we could somehow DRY it, as this bit repeats in many other workflows.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing a search about this led me to this issue as well as Composite Actions. I didn't delve deeply enough to see how that might play out with our actions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adjusted by 148dd5f, b86b5fe, and e7467f7. And it needs some workarounds due to lack of if syntax in composite actions. (ref: actions/runner#834)

@@ -23,7 +23,8 @@
"dealerdirect/phpcodesniffer-composer-installer": "^v0.7",
"phpunit/phpunit": "^7.5",
"wp-cli/i18n-command": "^2.2",
"wp-coding-standards/wpcs": "^2.3"
"wp-coding-standards/wpcs": "^2.3",
"yoast/phpunit-polyfills": "^1.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


Is this a dependency needed for GH Actions, or something that was missing before?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the problem stuck me for the longest before. I got error when running composer test-unit on my local and GHA. Not sure why it can run successfully on Travis CI.

> ./vendor/bin/phpunit
Error: The PHPUnit Polyfills library is a requirement for running the WP test suite.
If you are trying to run plugin/theme integration tests, make sure the PHPUnit Polyfills library (https://github.com/Yoast/PHPUnit-Polyfills) is available and either load the autoload file of this library in your own test bootstrap before calling the WP Core test bootstrap file; or set the absolute path to the PHPUnit Polyfills library in a "WP_TESTS_PHPUNIT_POLYFILLS_PATH" constant to allow the WP Core bootstrap to load the Polyfills.

If you are trying to run the WP Core tests, make sure to set the "WP_RUN_CORE_TESTS" constant to 1 and run `composer install` before running the tests.
Once the dependencies are installed, you can run the tests using the Composer-installed version of PHPUnit or using a PHPUnit phar file, but the dependencies do need to be installed whichever way the tests are run.
Script ./vendor/bin/phpunit handling the test-unit event returned with error code 1

Finally, I ended up this problem by adding the yoast/phpunit-polyfills dependency.

@eason9487
Copy link
Member Author

I'd rather make sure it's set to the exact same version as https://github.com/woocommerce/google-listings-and-ads/blob/develop/.nvmrc, which is the version that was used on Travis before, but more importantly, that's the version we use locally to build (if we use NVM). Which has the benefit of keeping all the parties at the same version, and a single point to set it up.

If the issue with building with lts would pop up, we should rather fix the issue, not downgrade, not to makes lag too far behind.

Tested build bundle with LTS version and it works well. I removed the specified node version 12.21.0.

And since the built-in node version of runs-on: ubuntu-latest is the same as the lts/* version (in .nvmrc), I think we could just use it without installing the same version via shivammathur/setup-php@v2 again. If still have concern about it, we can easily add it later through the prepare-node action then.

@eason9487 eason9487 requested review from tomalec and JPry October 7, 2021 12:03
@eason9487
Copy link
Member Author

I forgot to update the status badges within README before. Added by 9137422.

Copy link
Member

@tomalec tomalec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :)

@eason9487 eason9487 merged commit df4fbb3 into develop Oct 8, 2021
@eason9487 eason9487 deleted the add/github-actions branch October 8, 2021 02:59
@jconroy jconroy mentioned this pull request Oct 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants