feat(tests): stamp testtype__/model__ JUnit tags so tier coverage is recorded - #510
Open
ashokponkumar wants to merge 2 commits into
Open
feat(tests): stamp testtype__/model__ JUnit tags so tier coverage is recorded#510ashokponkumar wants to merge 2 commits into
ashokponkumar wants to merge 2 commits into
Conversation
…recorded hf-adapters emitted no JUnit result tags at all -- `record_property` appeared in no file in the repo -- so the CI/CD warehouse has zero tier-coverage data for it (0 rows carrying a testtype__ tag, against 3,311 for spyre-inference). Anything asking "has this artifact already been tested at tier X" can only answer "no" here, so no run can ever be reused. Each test case now carries `testtype__<tier>` for every tier its suite belongs to, plus `model__<id>` from the parametrization. Emitted as `<property name="tag" value="namespace__value"/>`, which the ingest already on main parses with no change -- verified: it writes one hf_run_properties row per tag. The tier cannot be recovered at test time: CI calls the per-suite make targets directly (never `make tests`), and a suite target does not know which tier invoked it. So the suite is declared with a new `--suite <key>` option -- matching this repo's existing idiom of pytest options (--run-slow, --model-path) rather than env vars, which no conftest here reads -- and tests/_tier_tags.py maps it to the tier set. MEMBERSHIP IS DECLARED, NEVER INFERRED FROM A LADDER. 11 of the gated suites are [regression, trunk, unit] with NO integration, so expanding "in unit => in everything above" would claim integration coverage for suites that never ran under it and silently skip real tests. A test asserts a ladder would still over-claim these suites, so the shortcut cannot creep back in. Source of truth is each suite job's `if:` gate in _test_matrix.yaml, because that is what actually decides whether a suite runs. Worth flagging for review: the Makefile's `tests` target carries a SECOND hand-maintained copy of the mapping and the two have ALREADY drifted -- the Makefile omits clip from every suites= list, has no multicard_smoke target, and puts model_components outside integration even though Makefile:8 documents it as an integration suite. A test pins that known divergence so a NEW one fails rather than being absorbed silently; reconciling them changes which suites `make tests` runs and belongs in its own change. Five suites are deliberately untagged, each for a checked reason: perf (scaffold, echoes an empty JUnit with no testcases), edge_cases (gated on edge_cases_only, no tier), multicard_smoke (torchrun script, emits no JUnit), adapter_coverage (runs --noconftest, so the fixture cannot bind and --suite would be a hard argument error) and model_module (delegates to oot_framework run_test.sh, which does its own marker-based tagging). Verified against the REAL conftest in a synced venv: 18 cases each carry the four token_compare tiers; an embed_compare run carries three (no integration) plus the model tag; omitting --suite emits zero tags rather than failing. A mutation closing the ladder fails the tests. ruff 0.15.6, black 26.3.1 and the repo's pre-commit hooks all pass. Signed-off-by: Ashok Pon Kumar <ashokponkumar@gmail.com>
ashokponkumar
requested review from
BenjSz,
arielge and
kiszk
as code owners
September 10, 2026 18:40
The tier tags are REPORTING, so nothing about them should be able to fail a suite. As first written they could: `result_tags` was imported at conftest MODULE scope, where any import error aborts collection for the whole suite. Worse, pytest then reports the first failing import in the conftest chain, so the message names an unrelated dependency and sends you looking in the wrong place -- which is exactly what happened while diagnosing this PR's red pod suites. Now the import happens inside the fixture, and both the import and the tagging are wrapped: a failure warns and the tests run untagged. Also returns early when --suite is absent, so the default path does no work at all. Verified by hiding tests/_tier_tags.py: collection previously aborted, now 13 cases run and pass with zero tags. The normal path still emits all four token_compare tiers. This does NOT yet explain the CI failures on this branch -- CI's exact invocation (pytest tests/spyre/... --suite token_compare) collects 23 tests cleanly locally in a synced venv, so the cause is still open. This change means that whatever it is, it can no longer take a suite down with it. Signed-off-by: Ashok Pon Kumar <ashokponkumar@gmail.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.
Problem
hf-adapters emits no JUnit result tags at all —
record_propertyappears in no file in the repo. So the CI/CD warehouse has zero tier-coverage data for it: 0 rows carrying atesttype__tag, against 3,311 for spyre-inference. Anything asking "has this artifact already been tested at tier X?" can only answer "no" here, so no hf run can ever be reused.Change
Each test case now carries
testtype__<tier>for every tier its suite belongs to, plusmodel__<id>from the parametrization. Emitted as<property name="tag" value="namespace__value"/>— the shape the ingest already on main parses, with no ingest change needed (verified: it writes onehf_run_propertiesrow per tag).The tier can't be recovered at test time — CI calls the per-suite make targets directly (never
make tests), and a suite target doesn't know which tier invoked it. So the suite is declared with a new--suite <key>option, andtests/_tier_tags.pymaps it to the tier set.--suiterather than an env var because that's this repo's existing idiom (--run-slow,--model-path); no conftest here reads env vars for configuration.Membership is declared, never inferred from a ladder
11 of the gated suites are
[regression, trunk, unit]with nointegration. Expanding "in unit ⇒ in everything above" would claim integration coverage for suites that never ran under it and silently skip real tests. A test asserts a ladder would still over-claim these suites, so the shortcut can't creep back in.Worth reviewer attention: the mapping already exists twice, and the copies disagree
Source of truth here is each suite job's
if:gate in_test_matrix.yaml— that's what actually decides whether a suite runs. But the Makefile'steststarget carries a second, hand-maintained copy, and the two have already drifted (pre-existing, not from this PR):if:caseclipsuites=listmulticard_smokeadapter_coveragemodel_componentsmodel_componentsis the interesting one:Makefile:8documents it as an integration suite and CI agrees — the Makefile's owncaseblock is the stale copy.A test pins this known divergence, so a new one fails rather than being absorbed silently. I deliberately did not reconcile them: that changes which suites
make testsruns, and belongs in its own change.Five suites are deliberately untagged
Each for a checked reason —
tier_tags()returns[], so they still get a model tag:perf— scaffold; echoes an empty JUnit with no<testcase>elementsedge_cases— gated oninputs.edge_cases_only, never a tiermulticard_smoke— runs a torchrun script, emits no JUnitadapter_coverage— runs--noconftest, so the fixture can't bind; passing--suitethere is a hard argument error (confirmed)model_module— delegates tooot_framework/run_test.sh, which does its own marker-based taggingVerification
Against the real conftest in a synced venv (not a stub):
token_comparetiersembed_comparerun carries three (correctly nointegration) plusmodel__…--suiteemits zero tags rather than failingScope
Producer only — no change to which tests run, only to what the JUnit XML records. Pairs with torch-spyre/spyre-inference#847, which fixes the same class of gap on that side.