Split the matrix computation into its own reusable workflow - #277
Open
swissspidy wants to merge 6 commits into
Open
Split the matrix computation into its own reusable workflow#277swissspidy wants to merge 6 commits into
swissspidy wants to merge 6 commits into
Conversation
Prototype. A matrix becomes a collapsible group in the Actions run view only when it sits on a job declared in the workflow the run belongs to. In the current shape the matrix is one level down, inside reusable-testing.yml, so it is not surfaced and all fifty legs appear as one flat list under a single "test" header. That is why the earlier attempt to group them by moving the PHP version into the calling job's name failed: there was no group to move it to. Move the `prepare` job into reusable-prepare-matrix.yml and expose the two matrices as workflow_call outputs. A package can now call that directly and run the legs from its own top-level jobs, which puts the matrix where the run view will group on it. reusable-testing.yml keeps its four inputs and now delegates to the new workflow, so every existing caller is unaffected and adoption is opt-in per package. Verified that both shapes pass identical `with:` blocks to the called workflows, that the public inputs are unchanged, and that the extracted matrix logic produces byte-identical output — 41 functional and 9 unit legs on a pull request, 51 and 12 on the schedule. This repository's own testing.yml adopts the fanned-out shape as the reference implementation, referring to the workflows by local path so that a pull request here exercises its own changes rather than whatever is on main. The cost is documented in the README rather than hidden: the fan-out is about thirty lines in a file that is not synced, so future changes to it have to be repeated per package. Leg names stay self-describing in both shapes, which means the grouped view repeats the PHP version inside the group. Worth revisiting once the grouped run can actually be seen. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jy4dmjymj9VmoTBaqrV4iX
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe workflows now centralize test matrix preparation in a reusable workflow. ChangesTest matrix fan-out
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant testing_yml
participant reusable_prepare_matrix
participant reusable_unit
participant reusable_functional
testing_yml->>reusable_prepare_matrix: Send matrix inputs
reusable_prepare_matrix-->>testing_yml: Return unit and functional matrices
testing_yml->>reusable_unit: Fan out unit matrix entries
testing_yml->>reusable_functional: Fan out functional matrix entries
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Confirmed against wp-cli/wp-cli-tests#352 that the fanned-out shape does group the legs in the run view. What it also showed is that the leg names then repeat their group: a group called "Behat | PHP 8.5" containing "Behat | PHP 8.5 | WP latest | SQLite". wordpress-develop handles this by suppressing the prefix in the called workflow whenever the calling job already states it — its leg name only includes "PHP {0} with" for the test-group and coverage callers, which are exactly the ones whose job name is not "PHP {0}". Do the same, with an explicit `grouped` input rather than inferring it from unrelated inputs. Wrapped callers leave it at its default of false and are unaffected; their names still have to stand on their own because their calling job is not surfaced. The expression is written as `!grouped && <prefix> || ''` rather than the more natural looking `grouped && '' || <prefix>`, because an empty string is falsy and the latter would fall through to the prefix in both cases. That is the same trap that made fetch-depth always evaluate to 1 earlier in this branch; zizmor's unsound-ternary audit is clean on the form used here. Noted in the README that a grouped leg name is only unique within its group, so the back-out is to set `grouped: false` for that caller. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jy4dmjymj9VmoTBaqrV4iX
Verified against wp-cli/wp-cli-tests#352. Grouping Behat by PHP version works — two to eight legs per group — but the unit matrix has exactly one leg per PHP version, so the same treatment produced nine groups containing one job each. Give the unit fan-out a single "Unit" group and let the PHP version distinguish the legs inside it. The grouped form of the unit name therefore keeps the version, rather than collapsing to a bare constant the way the Behat one does; the version is the only thing that tells those legs apart. Behat is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jy4dmjymj9VmoTBaqrV4iX
Reverts the previous commit, which was based on a bad measurement and on a wrong model of how the run view forms groups. The measurement only looked at the default matrix on a pull request, where the macOS and Windows entries are held back for the nightly schedule. That leaves one unit leg per PHP version and made grouping look pointless. On the schedule, and in any package that adds its own OS entries, PHP 8.5 has three unit legs — which is exactly the case worth grouping. The model was wrong in a more useful way. The run view groups legs whose calling job name is identical; a name that does not vary with the matrix does not produce one group holding everything, because GitHub appends the matrix combination to disambiguate it. `name: Unit` therefore produced one group per leg, labelled `Unit (8.5, latest, mysql-8.0)`. The name has to vary by the dimension being grouped on and collide across every other one. Both suites group by PHP version again, and the grouped unit leg name goes back to carrying only its runner and coverage suffixes. That rule is now written down in the README, since it is not obvious and this is the second time it has been guessed wrong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jy4dmjymj9VmoTBaqrV4iX
The 22.04 image is being retired and wordpress-develop has already moved. Only the default matters here; legs that name an explicit runner, and the macOS and Windows entries, are untouched. Also match the ImageMagick policy file by glob instead of a hardcoded ImageMagick-6 path. The guard added earlier keeps `sed -i` from failing on a missing file, but on an image that ships ImageMagick 7 it would have skipped silently and left the PDF coder blocked, which surfaces much later as a confusing media test failure rather than as a clear error. Five of the 51 legs pin MySQL 5.6 or 5.7, and the matrix reaches back to PHP 7.2. Those are the combinations most likely to object to the newer image, and they are the ones to watch on the first run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jy4dmjymj9VmoTBaqrV4iX
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft. Prototype for evaluation — the payoff is presentational and has not been observed working yet. See the caveat at the bottom before merging.
Why
A matrix becomes a collapsible group in the Actions run view only when it sits on a job declared in the workflow the run belongs to. Today the matrix is one level down, inside
reusable-testing.yml, so it is not surfaced: all fifty legs appear as one flat list under a singletestheader.That is also why the attempt in #274 to group the legs by moving the PHP version into the calling job's name failed, and had to be reverted in #276. There was no group to move it to — the version was not moved, it was hidden.
What this does
reusable-prepare-matrix.yml(new) holds thepreparejob, moved verbatim, and exposes the two matrices asworkflow_calloutputs. A package can call it directly and run the legs from its own top-level jobs, which puts the matrix where the run view will group on it.reusable-testing.ymlkeeps its four inputs and delegatesprepareto the new workflow. It drops from 546 lines to 87. Every existing caller is unaffected; adoption of the new shape is opt-in per package.This repository's own
testing.ymladopts the fanned-out shape as the reference implementation. It refers to the workflows by local path rather than@main, so a pull request here exercises its own changes instead of whatever is onmain— which closes a real gap, since until now a pull request against this repository never tested the reusable workflows it was changing.Verified
with:blocks to the called workflows (asserted, not eyeballed).reusable-testing.ymlare unchanged.The cost, documented in the README rather than hidden
The fan-out is about thirty lines in
testing.yml, which is not synced. Every future change to it has to be repeated in each package that adopts the shape. Leg names stay self-describing in both shapes, because the same called workflows serve both, so the grouped view repeats the PHP version inside the group. Worth revisiting once a grouped run can actually be seen.The version that removes this cost is larger: derive
minimum-phpfromcomposer.json'srequire.phpand move the remaining overrides to a per-package config file, at which pointtesting.ymlcarries no per-package configuration and can be synced. Not attempted here.Before merging
The grouping is unverified. It is reasoned from the observation that only the top-level
testjob produced a collapsible header in wp-cli/wp-cli-tests run 31163465254, not from seeing the grouped view work. The intended check is to merge this, convert one package'stesting.yml, and look at the run.If the grouping does not appear, this bought nothing and should be reverted. Nothing else depends on it — the split is otherwise behaviour-neutral.
Generated by Claude Code
Summary by CodeRabbit
New Features
Improvements
Documentation