ci_select: fix the membrowse test's env dependence, and stop HIL unit tests taking the rig - #3846
Merged
Merged
Conversation
The new divergence test called get_family_boards without ci=True, so it pinned whichever set the developer's machine produced. The CI skip lists are off by default locally and change the one-first pick on three families, so the assertion held here and went red on its first CI run: rp2040 is only diverging locally, samd2x_l2x and samd5x_e5x only on the runner. Pass ci=True unconditionally, as _prune_buildable already does for the same reason - this models the step being described, which always runs on CI - and pin the runner's twelve.
Size Difference ReportBecause TinyUSB code size varies by port and configuration, the metrics below represent the averaged totals across all example builds. Note: If there is no change, only one value is shown. Changes >1% in sizeNo entries. Changes <1% in sizeNo entries. No changes
|
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped to a test fix, uses the existing ci override supported by get_family_boards, and aligns the assertion with runner behavior to prevent CI-only failures.
Pull request overview
This PR fixes a CI-vs-local discrepancy in the HIL CI-metrics test suite by ensuring the “membrowse upload board divergence” test queries board selection as CI would (using CI skip/preferred lists), so the pinned family set is stable and matches runner behavior.
Changes:
- Pass
ci=Truetobuild.get_family_boards()intest_the_upload_board_can_diverge_from_the_built_boardso the test reflects runner board selection rather than a developer machine’s default. - Update the pinned expected “diverging families” set to the runner-derived 12-family set.
- Expand the test docstring to document why CI behavior must be forced in this test.
File summaries
| File | Description |
|---|---|
| test/hil/test/test_ci_metrics.py | Forces CI-mode board selection in the membrowse divergence test and updates the expected pinned family set accordingly. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
Rule 2 is a bare `test/hil/` prefix, so test/hil/test/** was booking the full
27-board rig - roughly 11 minutes of exclusive hardware - for diffs that
cannot reach it. This PR did it to itself.
Nothing on the rig runs those files: pre-commit does, and build.yml runs
test_ci_select.py as the gate before trusting a selection at all. Carve them
out to rule 1b, where test/{fuzz,unit-test}/** already sits for the same
reason. test/hil/ itself is untouched and still forces the whole rig - the
harness and the rosters decide what the rig does, so they cannot be trusted
to narrow their own blast radius.
The carve-out is a claim about that directory's contents rather than about
any one file, so a test pins its file list: anything added there that the rig
does read fails the suite instead of silently skipping hardware. Two more
pin the behaviour in both directions.
Rule table updated in the spec and in the module docstring, which is a
cell-for-cell carbon of it.
hathach
changed the title
|
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.
Follow-up to #3845, which merged while its last CI run was still red:
pre-commitfailson master right now.
test_the_upload_board_can_diverge_from_the_built_boardis the test that pins how manyfamilies the membrowse
$EX_ARGSremoval gave up data on. It calledget_family_boardswithoutci=True, so it pinned whichever set the developer's machineproduced — and the CI skip lists are off by default locally, which moves the
--one-firstpick on three families:
imxrt lpc11 lpc18 lpc54 mcx rp2040 rx samd11 stm32l0 stm32l4 tm4cimxrt lpc11 lpc18 lpc54 mcx rx samd11 samd2x_l2x samd5x_e5x stm32l0 stm32l4 tm4crp2040diverges only locally;samd2x_l2xandsamd5x_e5xonly on the runner. So theassertion held on my machine and went red on its first CI run.
Pass
ci=Trueunconditionally, exactly asci_select._prune_buildablealready does andfor the same reason — the question is what CI will do, so it must not change with
GITHUB_ACTIONSbeing set — and pin the runner's twelve.The measured trade in #3845 is unaffected: it is twelve families rather than eleven, and
rp2040was never one of them.Verified green both bare and under
GITHUB_ACTIONS=true CI=true;pre-commit run --all-filesgreen.Also: the harness's own tests stop taking the rig
Noticed on this PR's own CI run — a one-line test fix booked the full 27-board rig.
Rule 2 is a bare
test/hil/prefix, sotest/hil/test/**matches it. But that directoryholds the harness's unit tests, not the harness. Nothing on the rig runs them:
pre-commitdoes (.pre-commit-config.yaml:73,88), andbuild.yml:88,779runstest_ci_select.pyas the gate before trusting a selection at all.Carved out to rule 1b, where
test/{fuzz,unit-test}/**already sits for the same reason.test/hil/test/test_ci_*.pytest/hil/hil_test.pytest/hil/tinyusb.jsontest/hil/itself is untouched: the harness and the rosters decide what the rig does, sothey cannot be trusted to narrow their own blast radius.
The carve-out is a claim about that directory's contents rather than about any one file,
so a test pins its file list — anything added there that the rig does read fails the
suite instead of silently skipping hardware. Both directions are pinned, and I verified
each new guard actually fires by breaking it.
Rule table updated in the spec and in the module docstring, which is a cell-for-cell
carbon of it (that guard fires too — checked by editing the spec alone).