-
Notifications
You must be signed in to change notification settings - Fork 846
ci(tests): port to github actions #4919
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
base: develop
Are you sure you want to change the base?
Conversation
| - master | ||
| - develop | ||
|
|
||
| concurrency: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we run on PRs and on pushes to key branches, the concurrency setup will run each push event one at a time in series. So everything will fully finish there. Meanwhile for PRs, if something is rapidly pushed to a PR branch, it will cancel the previous commits run since it is no longer useful to the PR and continue with running the latest commit on those branches.
This helps save resources for the project.
| group: axe-core Tests ${{ github.ref_name }} | ||
| cancel-in-progress: ${{ github.event_name != 'push' }} | ||
|
|
||
| permissions: {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nifty trick for disabling all permissions in one sweep. So we refine permissions needed (if any) at the job level.
|
|
||
| if (process.env.FIREFOX_BIN) { | ||
| options.setBinaryPath(process.env.FIREFOX_BIN); | ||
| options.setBinary(process.env.FIREFOX_BIN); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got confused somewhere and had the wrong method in here. It just wasn't hit in the nightly testing where I was setting this env change up since it used a different variable.
Why does Webdriver not have uniform method naming for doing the same action between browsers? No clue. But we also can't use // @ts-check in this file since so many other things are broken. That will need a bigger effort to go fix on its own to help avoid these issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the CI workflow configuration and fixes the Firefox WebDriver binary path configuration method. The workflow has been restructured to separate concerns into individual jobs with improved security practices and updated action versions.
- Changed Firefox WebDriver configuration from
setBinaryPathtosetBinary - Restructured GitHub Actions workflow into separate jobs for linting, formatting, building, and testing
- Added workflow concurrency control and the
always()condition to nightly tests
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| test/integration/full/test-webdriver.js | Updates Firefox options to use setBinary() method instead of setBinaryPath() |
| .github/workflows/test.yml | Restructures CI workflow into separate jobs with pinned action versions, YAML anchors, and Node 24 support |
| .github/workflows/nightly-tests.yml | Adds always() condition to Chrome Beta tests to ensure they run even if previous tests fail |
Comments suppressed due to low confidence (1)
test/integration/full/test-webdriver.js:137
- Inconsistent API usage between Chrome and Firefox options. Chrome uses
setBinaryPath()while Firefox usessetBinary(). For consistency and correctness, Chrome should also usesetBinary()if both selenium-webdriver Options classes support this method, or there should be a comment explaining why different methods are used.
options.setBinaryPath(process.env.CHROME_BIN);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All LGTM, but I would like @dbjorge or @WilcoFiers approval so we don't miss anything.
I did notice we didn't port over the test_rule_help_version step which should be fine as that should only run on a pr to master but we were running it on develop, which I don't think we need to do since a pr to develop will never increase the version number before it goes to master.
| - uses: actions/checkout@v5 | ||
| - uses: actions/setup-node@v6 | ||
| - *checkout | ||
| - &install-deps-with-xvfb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if I understand everything correctly, calling &install-deps-with-xvfb will reinstall the node modules for every step and not cache it, whereas in circle we found a way to cache the build and the node modules so it only happened once for the entire run. This isn't so much a problem for now as as you said for the build we can worry about optimizing after. Looking at the runs, each of the tests take ~2mins longer than their circleCI counterpart, but since it all runs in parallel it doesn't add too much time in the long run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That feels like a step back. I don't care if we do that in this PR or a separate one but I don't think we should call this done until we've sorted this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, cacheing of node modules is handled by actions/setup-node. It caches the local cache of node_modules and not the modules in the repo after installation. Reason being, just restoring the cache in the repo may miss running install scripts that make things work. (This is probably why some of the tests broke after the move.)
The build time is like 30 seconds of effort when we do need to re-run it. Not enough time to run up a bill too much.
Ideally, we'd ultimately end up building docker images we run against that have all the OS deps pre-installed (aside from nightly stuff which of course we need to install at runtime.) This would reduce the bulk of that extra ~2m time. Since most of it is spent downloading and installing the same OS packages on every runner.
No one that I'm aware of in the org has done Docker Images for CI yet in GHA. This "install deps" shared action is how every other project has done this that I've seen. So it's a trade-off the org has accepted for the initial conversion of these projects to GHA. If we want to prioritize setting up the infrastructure for making docker images and refreshing them regularly for CI to run against, I'm all for it. But that's a bigger task that will need to be planned out with PO buy-in so it has accounted time to work on it and get it done.
|
On |
This patch ports the main test workflow over to GHA. This is not removing Circle CI configuration as we need to convert required checks over. As well as only removing that after the publishing is ported over to GitHub.
One key change in the patterns from the CircleCI system, is the browser tests and the examples have to do the build internally instead of using the build artifact. I have not had the time to fully investigate what is going on with the differences. I believe it fails in re-use now because of some slight difference in the way the caches were done before vs how they are done in GitHub Actions.
As an aside, I realized in the previous nightly PR I did mess up one aspect. Since the browsers are individual steps now, if the first running fails it was blocking the second from proceeding. For that situation, I added an instruction to always run the second browser. This is good enough for now and we can further optimize later when we setup better failure reporting.
Refs: #4912