Unit tests workflow: Run always, but bail early if only docs have changed #28669
Conversation
ockham
added
the
[Type] Automated Testing
|
Size Change: 0 B Total Size: 1.37 MB ℹ️ View Unchanged
|
ockham
changed the title
ockham
force-pushed
the
update/unit-tests-to-run-on-doc-changes
branch
from
0e33fee
to
6e4ffe2
Compare
ockham
force-pushed
the
update/unit-tests-to-run-on-doc-changes
branch
from
6e4ffe2
to
46a9242
Compare
|
I see that tests are green in this PR, so I presume changes are fine. Not sure how to go about testing this, if others feel confident about shipping, I'll defer to them, as I'm not very knowledgeable about GitHub actions. However, to offer some thoughts: would you think it'd be useful to create a few test PRs based on this one to test the following use cases:
They don't need to make sense, only should make tests fail. |
Thanks @nosolosw! I'm unfortunately less optimistic about this PR 😅 Tests are indeed running and passing, but the mechanism to suppress them for doc changes isn't working yet. The problem is that GitHub Actions don't support the concept of exiting early. (That feature has been requested e.g. at actions/runner#662). The workaround would be to add the exiting logic (or an So at this point, I'm thinking to file a separate PR to simply enable the unit tests workflow for all PRs (including doc changes). (Given the choice between saving some time by not running them on doc fixes, and making them mandatory for all PRs, I have a clear preference for the latter.) |
Ah, so the issue was that they were required AND skipped for PRs that had only changes to markdown files (hence they'll block the PR). Is that correct? It sounds that making them required and that they run for all PRs is a fine tradeoff. I also prefer that people wait a bit than not letting them merge PRs unless they have admin permissions. |
Yeah, exactly. Apologies if that was lost in technical details 😅
👍 |
|
BTW testing GitHub Actions can be fairly easy, and it's really a great tool, so here are some notes on that: First, a very small glossary: A workflow (e.g. Unit Tests) can consist of individual jobs (e.g. PHP Unit Tests) which in turn consist of individual steps (e.g. check out code, or run an For this PR, you can simply click on the 'Details' links next to the "Unit Tests / PHP" and "Unit Tests / JavaScript" GitHub Actions workflows at the bottom of the page.
Here's e.g. the JavaScript unit test job (on Node 12):
In the left sidebar, you can navigate to other jobs in the same workflow, or even to other workflows. Note the new job that I added to the Unit Tests workflow in this PR ("Make a list of files that were changed"):
You can expand individual steps in the job, which allows some closer inspection of what's going on:
E.g. here I compared the download size from curl to when I run it locally (and successfully), and they matched (so the download is probably successul -- I can rule out a 404 etc). Let's go back to the JS Unit test workflow now, and expand the step that I added in this workflow:
We're not finding any md files, so the subsequent steps are also executed, as they should. Let's compare this to a doc-files only PR I filed against my personal fork (with the changes from this PR applied to
Hmm. Only |
|
Closing in favor of #28696. |
|
Thanks for the step-through :) The terminology is now clearer for me. |
I think it's a good solution, I think a job can define some "output" variables that can be used in if statements. So the https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#jobsjob_idoutputs |
I'm just wary of the clutter it would add, TBH. We'd have to add that It might be worth revisiting if actions/runner#662 gets addressed. |
Description
I've recently made the PHP and JS unit tests required -- meaning they have to pass before a PR can be merged.
(This is in the wake of a recent issue that left PHP unit tests in a broken state -- also for subsequent PRs.)
@nosolosw then discovered that this affected documentation-only PRs (such as #28639): Since the unit test workflow had a condition that exempted it from running on such PRs, it never passed -- and the 'required' check criterion was never fulfilled.
This PR removes that condition from the workflow (so that the unit tests workflow always runs). Instead, it adds new steps to each unit test job that will cause them to bail early, if it the changed files are really only documentation files. (This could later be refined to e.g. only run the PHP unit tests if PHP files have been modified.)
This is based on this answer (with a small modification to use
git difflocally -- rather than the GH REST API -- in order to get the list of files that have been changed by the PR).How has this been tested?
Check the unit test GH workflows at the bottom of this PR. We'll also need a documentation-only PR to verify that it skips those -- TBD 🤔