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

Move visual regression testing to GitHub Actions #1865

Closed
aaemnnosttv opened this issue Aug 3, 2020 · 5 comments
Closed

Move visual regression testing to GitHub Actions #1865

aaemnnosttv opened this issue Aug 3, 2020 · 5 comments
Labels
Good First Issue Good first issue for new engineers P2 Low priority QA: Eng Requires specialized QA by an engineer Type: Enhancement Improvement of an existing feature
Milestone

Comments

@aaemnnosttv
Copy link
Collaborator

aaemnnosttv commented Aug 3, 2020

Feature Description

As a stepping stone towards moving our CI jobs to GitHub Actions (see #1779), a good first candidate to move is the VRT job. One benefit of moving this job in particular is that we can upload the generated HTML report that backstop generates as an artifact of the job for downloading/inspection later in the event of a failure where this is not readily available with Travis (possible but requires you bring your own storage and authentication).


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • The "Visual Regression" test job is removed from .travis.yml
  • All references to the $VRT environment variable are removed from .travis.yml
  • VRT should run on GitHub Actions as a check for all open PRs with a base branch of develop or master branches and must pass for a PR to be merged
  • The generated VRT report HTML should be saved as an artifact if the job fails

Implementation Brief

Travis Config

  • Remove job in Travis

    site-kit-wp/.travis.yml

    Lines 115 to 116 in 6eb4c52

    - name: Visual Regression
    env: VRT=1
  • Remove condition in before_install
    if [[ "$JS" == "1" ]] || [[ "$VRT" == "1" ]] || [[ "$E2E" == "1" ]] || [[ "$SNIFF" == "1" ]]; then
  • Remove conditional in before_script block

    site-kit-wp/.travis.yml

    Lines 50 to 52 in 6eb4c52

    if [[ "$VRT" == "1" ]]; then
    docker run --rm -v "$PWD:/app" -v "$HOME/.cache/composer:/tmp/cache" composer install --no-scripts
    fi
  • Remove conditional in script block

    site-kit-wp/.travis.yml

    Lines 80 to 82 in 6eb4c52

    if [[ "$VRT" == "1" ]]; then
    travis_retry npm run test:visualtest || exit 1
    fi

GitHub Actions

  • Create a new .github/workflows/tests.yml workflow file
    See zips.yml for reference as to when the workflow should run (although we only need pull_request)
  • Create a visual-regression job with steps to run
    Most of this can be borrowed from storybook.yml since VRT starts Storybook as the target to use when it runs
    • Checkout the repo
    • Install composer dependencies (use the same command as we had on Travis, with updated path for where the cache is located locally - although it should be the same)
    docker run --rm -v "$PWD:/app" -v "$HOME/.cache/composer:/tmp/cache" composer install --no-scripts
    
    Note --no-scripts is important as we don't need to do any dependency scoping which hooks in here (this is really only needed for installing WP for sourcing CSS used by core) - we should change this but not sure that this is the right issue to do so
    • Cache composer packages after install
    • Set up node with the correct version using nvm
    • Run npm ci
    • Cache node modules after install
    • Run npm run test:visualtest to start Storybook and run Backstop in docker as usual
      • As a workaround to deal with the lack of travis_retry, this can be called with a fallback to itself (npm run test:visualtest || npm run test:visualtest) to retry it once if it fails the first time (which still happens, although rarely) since Actions do not provide away to re-run specific jobs.
    • Add a subsequent step for [uploading the tests/backstop/html_report as an artifact (example)
      • Set the job.<job-id>.steps.if to if: failure() so that it only runs if the job fails
        If it fails before the tests run for some reason, the artifact would be empty or not uploaded which is fine

QA Brief

  • VRT no longer runs in Travis builds (for jobs running on branches that include the latest develop)
  • VRT runs in GitHub actions for PRs against develop or master (won't take effect until following release)
  • The behavior of VRT in GHA has already been verified before merging as this takes effect as soon as it's merged
    • See a job where VRT failed and the report was uploaded as an artifact of the job here
    • See a successful VRT run here

Changelog entry

  • N/A
@aaemnnosttv aaemnnosttv added P2 Low priority Type: Enhancement Improvement of an existing feature labels Aug 3, 2020
@tofumatt
Copy link
Collaborator

tofumatt commented Aug 6, 2020

We should make sure to include the cache action so our composer (and other dependencies) are cached when they don't change. See: https://github.com/google/site-kit-wp/blob/develop/.github/workflows/storybook.yml#L22 for reference.

Let's make that clear in the IBs so we don't see a slowdown in action speed when converting these over from Travis 😄

@tofumatt tofumatt assigned aaemnnosttv and unassigned tofumatt and felixarntz Aug 6, 2020
@aaemnnosttv aaemnnosttv assigned tofumatt and unassigned aaemnnosttv Aug 6, 2020
@felixarntz
Copy link
Member

IB ✅

@eclarke1 eclarke1 modified the milestone: Sprint 30 Aug 11, 2020
@eclarke1 eclarke1 added this to the Sprint 31 milestone Aug 25, 2020
@eclarke1 eclarke1 removed this from the Sprint 31 milestone Aug 27, 2020
@eclarke1 eclarke1 modified the milestone: Sprint 32 Sep 8, 2020
@eclarke1 eclarke1 added this to the Sprint 33 milestone Sep 22, 2020
@tofumatt tofumatt added the Good First Issue Good first issue for new engineers label Sep 22, 2020
@eugene-manuilov eugene-manuilov self-assigned this Sep 28, 2020
@eugene-manuilov eugene-manuilov removed their assignment Sep 28, 2020
@eclarke1 eclarke1 removed the Next Up label Sep 28, 2020
@aaemnnosttv
Copy link
Collaborator Author

@aaemnnosttv aaemnnosttv added the QA: Eng Requires specialized QA by an engineer label Sep 29, 2020
@aaemnnosttv aaemnnosttv removed their assignment Sep 29, 2020
@felixarntz
Copy link
Member

Putting this back to Execution just for a follow-up PR to fix the names as discussed before.

@tofumatt
Copy link
Collaborator

tofumatt commented Oct 1, 2020

QA ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue Good first issue for new engineers P2 Low priority QA: Eng Requires specialized QA by an engineer Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

5 participants