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

Scope pull request workflows to paths #1079

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

paulrobertlloyd
Copy link
Contributor

@paulrobertlloyd paulrobertlloyd commented Nov 21, 2024

Currently, visual regression tests are run when any file changes in a pull request. Should you make a tiny change to any file in the rep, such as package.json or a configuration file, it’ll take ~3.45mins to run visual regression tests. Changes outside of components get needlessly tested, but more importantly, untold resources are consumed while this task works away on a remote server somewhere.

This PR breaks out the previous pull-request.yml workflow into 3 separate workflows:

  • analyse.yml - Perform static analyse if any file has changed
  • lint.yml - Check code style if any file has changed (note that linters configured to only examine certain files within packages/components and /dist/app/components folders, arguably JS outside of packages/components should be linted, too)
  • test.yml - Test JavaScript and perform visual regression tests only if a file has been changed in the packages, app or test folders. This is the change that should hopefully reduce the number of times these tests are run, notably when dependency updates are made to package.json and package-lock.json.

Now uses workflow action to check for changed files that are actually included in respective test tasks before running them.

@paulrobertlloyd paulrobertlloyd changed the title Scope pull-request worflows to paths Scope pull-request workflows to paths Nov 21, 2024
@paulrobertlloyd paulrobertlloyd changed the title Scope pull-request workflows to paths Scope pull request workflows to paths Nov 21, 2024
.github/workflows/analyse.yml Outdated Show resolved Hide resolved
.github/workflows/lint.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
@paulrobertlloyd paulrobertlloyd force-pushed the scope-worflows-to-paths branch 5 times, most recently from 46fffb9 to 9a7aaa7 Compare November 28, 2024 12:19
@paulrobertlloyd
Copy link
Contributor Author

paulrobertlloyd commented Nov 28, 2024

Using tj-actions/changed-files action, we can test for changed files, and only continue to run workflows if a subset of files has changed.

I tested changing the HTML in a component, and the visual regression tests ran and correctly failed (after 3 minutes):

Screenshot 2024-11-28 at 12 13 47

I reverted that change, and that test completed in only 24 seconds:

Screenshot 2024-11-28 at 12 22 35

Using this action, we could revert to having all pull request workflows in one file. @frankieroberto what do you think? One file, or many?

@frankieroberto
Copy link
Contributor

@paulrobertlloyd stick with one file for now (easier to review the diff)?

Given that most of the repo is in scope for possible changes which affect the tests, I slightly wonder useful it’ll be, but it would be nice to have speedier ✅ on PRs which just affect the README or whatever.

@paulrobertlloyd paulrobertlloyd force-pushed the scope-worflows-to-paths branch from 9a7aaa7 to d12eb3b Compare December 2, 2024 11:47
Comment on lines -65 to -66
- name: Run tests
run: npm test
Copy link
Contributor Author

@paulrobertlloyd paulrobertlloyd Dec 2, 2024

Choose a reason for hiding this comment

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

@frankieroberto Removed the (mostly) duplicate test run here, though potentially given concurrent tests, you’d want this workflow to fail early if there’s a failing test before running expensive visual regression tests?

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