-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Unit Tests Workflow: Enable for documentation-only PRs #28696
Conversation
Size Change: +1.38 kB (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
I think it was @mkaz who added those checks: while this is fine by me, I'm also a newbie when it comes to GitHub actions so I was hoping to get him interested in reviewing this PR. |
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.
Makes sense. My only minor suggestion is to remove the word "now" from the comment since that won't make sense in 10 years when someone reads this comment 😛
Good point, thanks! I'll make that change and 🚢 |
on: | ||
pull_request: | ||
paths-ignore: | ||
- '**.md' |
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 think we should explore ways to allow this kind of config using if
statements in the jobs. I think some reusable github actions already exist for this use-case.
It's also very useful if we decide one day to consolidate in a single workflow.
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.
See #28669 for an exploration and some discussion 🙂
(cherry picked from commit 8cfbfb0)
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).
I was originally trying in #28669 to be smart and enable the workflow, but keep skipping unit tests for documentation-only PRs. Unfortunately, this has turned out harder than expected (see this comment).
(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.)