fix[notask]: decouple mobile shard validation from desktop integration tests - #4031
Conversation
…n tests `generate-mobile-integration-tests.js` wrote integration.auto.cjs and then asserted that every runner was assigned to a Device Farm shard. The script is chained into `npm run test:integration`, so that assertion could abort desktop integration tests on all seven platforms — which is what happened when #4006 removed the pi05 group: `bare` aborted with exit 134 before a single test ran. Generation had already succeeded at that point. The failure was purely a mobile scheduling policy check running in the desktop path. - Remove validateGroups from the generator; it now only generates. - Move the rules to scripts/lib/validate-test-groups.js — pure, no fs, no process.exit — and call them from validate-mobile-tests.js, which already existed but was wired into nothing. - Add a top-level `deferred` key to test-groups.json recording runners that are intentionally not scheduled. pi05 mobile coverage is deferred pending a project-owned CDN mirror and is gated on-device by `_skipMobilePi05`, so this preserves #4006's outcome while keeping "not scheduled" distinguishable from "forgotten". It must stay top-level: the CI composites read only `.<platform>` and ignore sibling keys, as OCR's `perf_report_filter` already relies on. - Wire `test:mobile:validate` and a new unit suite into `test:unit`, which runs in the ungated ts-checks job — #4006 touched nothing native, so the gated integration job that would have caught it was skipped on its own PR. - Skip the mtime staleness heuristic when CI is set; a fresh clone stamps every file at checkout, so that comparison is meaningless there. Device Farm output is unchanged: same 2 specs, same greps, runPi05Test still absent from the runner list. integration.auto.cjs regenerates byte-identical. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review StatusCurrent Status: ✅ APPROVED |
License compliance — cleanNo new dependency license findings in this PR. Warn-only (shadow) mode — this check does not block merges yet. Updated automatically by the canonical license compliance workflow. NOTICE presence (advisory)Missing NOTICE (advisory, does not block):
|
yingying0906
left a comment
There was a problem hiding this comment.
Ran the branch locally. The diagnosis is right and the split is the right shape, nits inline.
What I checked myself:
ts-checksinon-pr-vla.ymlonly needsauthorizeandfork-approvaland runsnpm run test:unit, and #4006 touchedpackages/vla-ggml/**so it matches thepathsfilter. The check would have caught it.deferredreally is inert to CI.upload-to-devicefarmreads.[$p]only andvalidate-devicesscopes toPLAT_KEY.prettier --checkwith the realprettier-config-holepunchis clean on all changed scripts, andnode scripts/validate-mobile-tests.jsexits 0 with and withoutCI=true.- Dropping the mtime check is fine.
buildFileContentsonly reads filenames and never opens a test file, so the timestamp could never mean anything.
Two things that don't fit on a line:
The PR body is behind the head commit. It says the mtime heuristic is skipped when CI is set and that the new suite has 8 tests, but 7ef057275 removed mtime completely and the suite has 9. The body ends up as the squash message so worth fixing.
llm-llamacpp and ocr-ggml still have the same landmine and main can go red the same way from either. Splitting per release line makes sense, just file the tickets instead of leaving them in the PR body.
No CHANGELOG is fine, sanity-checks is green.
Five review findings on the mobile shard validation split.
1. The unit suite asserted on its own copy of the runner-name regex.
scripts/__tests__/mobile-test-groups.test.js carried a byte-copy of
getGeneratedRunnerNames from validate-mobile-tests.js, so it proved nothing
about the extractor that actually runs in CI. It is now generatedRunnerNames
in lib/validate-test-groups.js, called by both. It takes the file contents
rather than a path, so the lib stays fs-free.
A new test pins it against the real committed integration.auto.cjs: a
template change that renames the declarations can no longer yield zero
runners, which would make every coverage rule below vacuously pass.
2. `deferred` was global, so per-platform deferral was unrepresentable.
The contradiction rule runs inside the platform loop against one flat set, so
"deferred on ios, scheduled on android" always reported a contradiction on
android. Fine for pi05, but it blocks the llm-llamacpp and ocr-ggml ports,
where the platform sets differ. `deferred` now accepts either a flat array
(every platform, what VLA uses) or a `{ <platform>: [runner] }` map.
That is also why platformNames now excludes DEFERRED_KEY by NAME rather than
by shape: an object-form `deferred` is a non-array object, so shape-based
inference would have read it as a platform demanding full coverage. A map
keyed by a non-platform (`deferred: { io: [...] }`) deferred nothing while
reading as a clean file, so that is reported too.
3. isPlatformEntry treated every top-level object as a platform.
llm-llamacpp ships top-level iosWeekly/androidWeekly maps that are schedules,
not platforms, so the same lib there would demand full coverage of them.
validateTestGroups now takes options.platforms to pin the list. Inference
stays the default, so VLA still picks up a newly added platform key
automatically. A pinned platform absent from the file is now reported instead
of silently passing.
4. The reference diff does not cover generator-template changes.
getGeneratedIntegrationRefs compares only the runIntegrationModule paths, so
editing the generator's own template (the __shouldRunTest guard, the header
comments) leaves the committed file stale with both checks green. Wording
softened to say so. Nothing regressed: the removed mtime check compared
test-file timestamps, not the generator's, so it never caught this either.
Not closed here because the generator requires bare-fs, so buildFileContents
is not callable from the node validator.
5. No documented way to run a deferred runner on device.
validate-devices builds its runner allowlist from `.<platform>` too, so a
manual dispatch with `-f tests=runPi05Test` is rejected as an unknown runner.
test/mobile/README.md now documents both halves of the gate: un-deferring the
runner is not enough, because _skipMobilePi05 skips both pi05 cases on mobile
regardless of scheduling, so that alone runs zero tests and passes green.
Verified: 16/16 mobile-test-groups tests pass (up from 9) and 8/8 pre-existing
prestage tests, no regression; validate-mobile-tests.js exits 0 with "4
runner(s), group coverage OK"; prettier --check with the real
prettier-config-holepunch and node --check are clean on every changed file. The
generator is untouched, so integration.auto.cjs is unaffected.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
/review |
🎯 Problem
packages/vla-ggml/scripts/generate-mobile-integration-tests.jswroteintegration.auto.cjsand then asserted that every runner was assigned to a Device Farm shard on every platform. That script is chained intonpm run test:integration:So a mobile scheduling assertion sat directly in the desktop test path, and anything it threw took desktop integration tests down on all seven platforms.
That is what happened when #4006 removed the
pi05group to stop scheduling deferred pi05 mobile tests.pi05.test.jsstayed intest/integration/, sorunPi05Testbecame "unassigned" andbareaborted:Two things make this worse than a one-off:
test-groups.jsonand a workflow comment — no native changes — sointegration-testswasskippedon its PR. The only job that would have failed was the one the gate skipped.mainhas been red for every VLA PR that actually runs integration tests since.📝 How
Split generation from validation, and make the deliberate omission explicit rather than invisible.
The generator only generates.
validateGroupsis removed. Nothing ingenerate-mobile-integration-tests.jscan throw on scheduling state any more.The rules move to
scripts/lib/validate-test-groups.js— pure, dependency-free, nofsand noprocess.exit, so the same rules run from the CLI and from unit tests.scripts/validate-mobile-tests.jscalls it; that script already existed but was wired into nothing.The lib also owns
generatedRunnerNames(content), theasync function run<Name>extractor. It takes file contents rather than a path so the module stays fs-free, and it lives beside the rules that consume its output sovalidate-mobile-tests.jsand the unit suite share one implementation. A test asserting on its own copy of that pattern would prove nothing about the extractor that actually runs.test-groups.jsongains a top-leveldeferredkey:{ "ios": { "smolvla": [...], "groot": [...] }, "android": { "smolvla": [...], "groot": [...] }, "deferred": ["runPi05Test"] }This preserves #4006's outcome — pi05 is still not scheduled — while keeping "deliberately not run" distinguishable from "forgotten". Deleting the runner from the file, as before, made those two states identical.
deferredmust stay top-level. The CI composites consume only.<platform>and ignore sibling keys —upload-to-devicefarm/action.ymlsays so explicitly ("Other top-level keys (e.g. OCR'sperf_report_filter) are ignored"), and OCR already relies on it. Nested underios/androidit would be scheduled as a real shard, so that is reserved and reported, not merely documented: to the coverage rules a nesteddeferredis just another array of runner names, so the file would otherwise validate clean whileupload-to-devicefarmbilled a shard literally named "deferred".deferredalso accepts a{ "<platform>": [runner] }map, for a runner scheduled on one platform and deferred on another. A flat array cannot express that — the scheduled-and-deferred rule fires on the platform that does run it. VLA uses the flat form; the map form is what the llm/ocr ports need, where the platform sets differ. This is whyplatformNamesexcludesdeferredby name rather than by shape: an object-formdeferredis a non-array object, so shape-based inference would read it as a platform demanding full coverage.Platform detection can be pinned.
validateTestGroups(groups, runners, { platforms })overrides shape inference. Inference is right for VLA, whose only top-level maps are platforms, and it means a newly added platform key is covered automatically — butllm-llamacppships top-leveliosWeekly/androidWeeklymaps that are schedules, not platforms, so a caller there pins the list instead. A pinned platform missing from the file is reported rather than silently passing.The check moves to a job that actually runs.
test:unitnow ends withtest:mobile:groups && test:mobile:validate, andtest:unitruns ints-checks, which needs onlyauthorize+fork-approval. A PR like #4006 that touches nothing native now gets the check.New unit suite at
scripts/__tests__/mobile-test-groups.test.js(16 tests), matching themobile-test-groups.test.jspatternasr-ggmlandtts-ggmlalready have and this package lacked. It covers the real committed data plus unassigned runners, group typos, staledeferredentries (flat and per-platform), scheduled-and-deferred contradictions, adeferrednested inside a platform, per-platform deferral, adeferredmap keyed by a non-platform, pinned-vs-inferred platform lists, and non-platform metadata keys.scripts/generate-mobile-integration-tests.jsvalidateGroupsremoved; generation onlyscripts/lib/validate-test-groups.jsscripts/validate-mobile-tests.jsscripts/__tests__/mobile-test-groups.test.jstest/mobile/test-groups.jsondeferred: ["runPi05Test"]test/mobile/README.mdpackage.jsontest:unitOne incidental removal:
validate-mobile-tests.jscompared test-file mtimes againstintegration.auto.cjs.buildFileContentsderives that file from the sorted filenames undertest/integration/and never opens a test file, so editing a test's body cannot make it stale — the check could only produce false positives, and chainingtest:mobile:validateintotest:unitwould have promoted each one into a hard failure telling the author to regenerate a byte-identical file. It is removed rather than CI-gated.The content-based reference diff that replaces it covers the staleness that matters day to day: a test file added, renamed or removed. It does not cover a change to the generator's own template (the
__shouldRunTestguard, the header comments), since it compares only therunIntegrationModulepaths — that still needs a manualnpm run test:mobile:generate. Nothing regressed there: the mtime check compared test-file timestamps, not the generator's, so it never caught that either. Closing the gap properly means callingbuildFileContentsfrom the validator, which is not possible while the generator requiresbare-fs.🧪 Tested
CI runs:
https://github.com/tetherto/qvac/actions/runs/32758510083
https://github.com/tetherto/qvac/actions/runs/32758735695
https://github.com/tetherto/qvac/actions/runs/32756119505
Verified against real
barev1.31.0 andnode --test.The A/B that matters — the generator run against the exact
test-groups.jsonfrommain(nodeferredkey,pi05.test.jspresent), the same data that produced exit 134 in CI:mainbare ./scripts/generate-mobile-integration-tests.jsnode scripts/validate-mobile-tests.jsrunPi05Teston both platformsnode --test scripts/__tests__/mobile-test-groups.test.jsThe same mistake is still caught — twice — but in a two-minute ungated job instead of by aborting seven platforms' integration suites.
With the fix in place:
bare ./scripts/generate-mobile-integration-tests.js→ exit 0.node scripts/validate-mobile-tests.js→ ✅ 4 runners, group coverage OK.node --testacross both script suites → 24/24 pass, including the pre-existinggenerate-prestage-blocktests (no regression).prettier --checkwithprettier-config-holepunch→ clean.node --checkon every changed JS file → clean. Both changed JSON files parse.Two claims verified rather than assumed:
deferredis inert to CI. Ran the composites' ownjqexpressions against the new file.upload-to-devicefarmstill resolves[{smolvla, runAddonTest|runEsmNamedExportsTest}, {groot, runGrootTest}];validate-devicesstill returns 3 runner names and spec-count 2. Identical to before —runPi05Testnever reaches Device Farm.integration.auto.cjsregenerates byte-identical.git statusshows it unmodified after running the generator, confirming only the assertion was removed, not a byte of generated output.Also checked: no
*.test.jsundertest/integration/is gitignored, so the validator cannot fail on a clean checkout; andgenerate-prestage-block.jsderives its runner list fromMODEL_SHARDS, not fromtest-groups.json, so it is unaffected.None. No addon, runtime or native code is touched — only scripts, their tests, the shard map and its README. Device Farm scheduling is byte-identical, and the published package is unaffected (
scripts/is not inpackage.jsonfiles).Two follow-ups worth noting, neither introduced by this PR:
llm-llamacppandocr-ggmlcarry the same landmine. Both chaintest:mobile:generateintotest:integration:generateand have a throwingvalidateGroups, and both ship atest-groups.json. Editing either the way QVAC-23984 fix: stop scheduling deferred pi05 mobile tests #4006 edited VLA's takes down their desktop integration too. They are left out becauserelease-<package>-<x.y.z>is per package, so a three-addon PR cannot be back-ported or reverted per release line — and the three fixes are not mechanical duplicates (llmhasiosWeekly/androidWeeklyfamily logic plus a hardcodedisOverrideOnlylist,ocrhasperf_report_filter). Each needs its own PR; the lib here is already shaped for both (pinned platform list, per-platformdeferred). Tickets still to be filed — noted in review, not yet done.sanity-checksdoes not require one for a scripts-only fix.🔍 Review round 2
Five findings from @yingying0906, all fixed in
0afebd906:generatedRunnerNamesin the lib, plus a test pinning it against the real committedintegration.auto.cjsso a template rename cannot yield zero runners and make every coverage rule vacuously pass.deferredwas global, so per-platform deferral was unrepresentable and the llm/ocr ports were blocked → the map form, andplatformNamesnow excludingdeferredby name rather than by shape.isPlatformEntryread every top-level object as a platform, which breaks on llm'siosWeekly/androidWeekly→options.platforms.validate-devicesbuilds its allowlist from.<platform>too, so-f tests=runPi05Testis rejected → README now documents both halves of the gate, including that_skipMobilePi05skips both pi05 cases on mobile regardless of scheduling, so un-deferring alone runs zero tests and passes green.🤖 Generated with Claude Code