Skip to content

Conversation

@ravichdev
Copy link
Contributor

@ravichdev ravichdev commented Mar 24, 2021

Summary

  • PHP unit tests use a modified script to download WP and WP unit tests. We could look into moving this file to wp-dev-lib
  • The PHP tests use the base ubuntu-* image to run PHP tests, using docker to run these tests will increase the time.
  • The build is pretty fast now except for the E2E tests which use docker images and npm run env:start to setup local docker env and this is resulting in the job taking ~4 minutes. This could also be improved if we use apache or nginx bundled in the base ubuntu-* image instead of docker.

Test PR actions - https://github.com/ravichdev/wp-foo-bar/actions/runs/684218426

Fixes #

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 Contributing Guidelines (updates are often made to the guidelines, check it out periodically).

@ravichdev
Copy link
Contributor Author

ravichdev commented Mar 24, 2021

@derekherman @kasparsd This is ready for review.

@@ -0,0 +1,162 @@
name: Coding Standards and Tests

Choose a reason for hiding this comment

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

I think it is cleaner to have this as different workflows in different files.

A good example of this would this project - https://github.com/google/web-stories-wp/tree/main/.github/workflows

I would break into following files.

  • test-php.yml
  • test-js.yml
  • lint-js-css.yml
  • lint-php.yml
  • test-e2e.yml

on:
push:
branches:
- master

Choose a reason for hiding this comment

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

Should this be main or include main

run: npm ci

- name: Run coding standards check
run: npm run lint

Choose a reason for hiding this comment

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

You can pipe lint results into cs2pr. This will highlight the line errors. Here is an example.

https://github.com/google/web-stories-wp/blob/458499e8178d5e911d18504829666ce29d7ae85a/.github/workflows/lint-php.yml#L67


env:
NODE_ENV: e2e
WP_VERSION: 5.7

Choose a reason for hiding this comment

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

Why is this hardcoded to 5.7? Travis just uses latest
https://github.com/xwp/wp-foo-bar/blob/develop/.travis.yml#L71


- name: Install dependencies
run: npm ci

Choose a reason for hiding this comment

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

Dont we need to composer install as well here?

strategy:
fail-fast: false
matrix:
php_versions: [7.4, 7.3, 7.2, 7.1]

Choose a reason for hiding this comment

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

Should we add PHP 8 here.

Comment on lines +142 to +147
- name: Install and Run tests
if: ${{ matrix.php_versions == '7.0' || matrix.php_versions == '5.6.20' }}
run: |
wget -O bin/phpunit https://phar.phpunit.de/phpunit-5.phar
chmod +x bin/phpunit
source bin/php-tests.sh wordpress_test root root localhost false bin/phpunit

Choose a reason for hiding this comment

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

You can install via composer.

@spacedmonkey
Copy link

@ravichdev I have posted some thoughts here. I have a lot of experience with github actions. We can copy and paste lots of work done elsewhere, like in web stories.

Also, can we enable github actions, see can these running.

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.

2 participants