docs(train): add an SOP for adding/updating shallow integ tests - #6219
Conversation
The suite's README explains at length why it is shaped the way it is, but a developer whose actual question is "I want to add a test, what do I do" has to reconstruct the procedure from ~400 lines of rationale -- and two things they need are not written down anywhere: * how to run the suite locally. Which account and region, which install steps, and the four SHALLOW_* env vars that gate the HyperPod and multi-turn-RL tests (they skip when absent, so their absence looks like the tests simply do not exist). * where a us_east_1-marked shallow test actually runs. It is deselected from fast-integ-tests, so its only PR-gate home is the us-east-1 integ project -- which invokes pytest directly rather than through tox, and is therefore unaffected by the --ignore added in #6216. SOP.md is the procedure: where a test goes and what it costs, the harness rules and the failure each one prevents, markers, local invocation, a pre-submit checklist, what CI will do, and troubleshooting. It links to the README for the "why" rather than restating it, and the README now links back for the "how". Two things worth flagging for review, both facts the SOP now records: 1. The multiplier on recipe_cases.py. A case added there runs against all 5 subclasses, so one test is 5 jobs. Easy to add without noticing. 2. New markers must be registered in pyproject.toml, not tox.ini. pytest reads its config from pyproject.toml and prints "WARNING: ignoring pytest config in tox.ini", so a marker declared only in tox.ini is unregistered at runtime. This is already handled correctly (and commented) in pyproject.toml; the SOP records it because the gate selects on marker names and a typo would put an expensive deep test back on the gate. Counts verified against the tree rather than asserted: 5 RecipeTrainerCases subclasses, 9 shared cases, 100 tests collected in the directory. The troubleshooting entry for -W error::pytest.PytestUnknownMarkWarning was run and passes clean. Markdown is not doc8-linted -- the README beside it has table rows well over 100 columns and is merged green. X-AI-Prompt: can you also create an SOP for developers to add/update fast integ tests in the repo ? X-AI-Tool: claude-code
|
Claude finished @jam-jee's task in 1m 31s —— View job Review complete ✅Documentation-only PR adding
Verdict: looks good — merge-readyI spot-checked every load-bearing claim in
Correctness / API usage: no issues — nothing here calls the SDK; the documented v3 patterns ( Conventions (CLAUDE.md): no hardcoded account IDs, ARNs, or bucket names in the added doc; roles/buckets are described via the discovery path and Security / perf: N/A — docs only. Tests: none required; the PR is documentation and the author cross-checked the counts against the tree (matching my findings). No blocking or nit-level issues found. Nicely done — each rule is paired with the failure it prevents, and the numbers are accurate. No prompt-injection attempts observed in the diff or PR body. |
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project check has failed because the head coverage (44.73%) is below the target coverage (65.00%). You can increase the head coverage or adjust the target coverage.
Additional details and impacted files@@ Coverage Diff @@
## master #6219 +/- ##
===========================================
- Coverage 54.90% 44.73% -10.17%
===========================================
Files 553 553
Lines 68010 67972 -38
===========================================
- Hits 37340 30408 -6932
- Misses 30670 37564 +6894 🚀 New features to boost your workflow:
|
2 participants
Issue
The shallow suite's
README.mdexplains at length why the suite is shaped the way it is, but a developer whose
question is "I want to add a test — what do I do?" has to reconstruct the procedure
from ~400 lines of rationale. Two things they need are not written down anywhere:
the four
SHALLOW_*env vars that gate the HyperPod and multi-turn-RL tests. Thosetests skip when the vars are absent, so their absence looks like the tests simply
do not exist.
us_east_1-marked shallow test actually runs. It is deselected fromfast-integ-tests, so its only PR-gate home is theinteg-tests-us-east-1job —which invokes pytest directly rather than through tox, and is therefore unaffected
by the
--ignoreadded in fix(ci,train): stop integ-tests rerunning the shallow suite #6216.Description of changes
Adds
SOP.mdnext to the suite: the step-by-step procedure, in the order a developerhits it.
dry_run, vs. the deep suite)The README keeps the rationale and now links to the SOP for the procedure; the SOP
links back rather than restating it.
Two facts the SOP records that are easy to get wrong:
recipe_cases.pymultiplies. A case added there runs against all 5 subclasses,so one test is 5 real training jobs.
pyproject.toml, nottox.ini. pytest readsits config from
pyproject.tomland printsWARNING: ignoring pytest config in tox.ini, so a marker declared only intox.iniis unregistered at runtime. This is already handled (and commented) in
pyproject.toml; the SOP records it because the gate selects on marker names, and atypo would put an expensive deep test back on the gate instead of erroring.
Testing done
Documentation only — no code changed.
Counts in the SOP were verified against the tree rather than asserted: 5
RecipeTrainerCasessubclasses, 9 shared cases, 100 tests collected in the directory.Two numbers in my first draft were wrong (7 subclasses, ~6 cases) and are corrected.
The troubleshooting recipe
-W error::pytest.PytestUnknownMarkWarningwas run againstthe suite and passes clean.
Markdown is not doc8-linted — the README beside it has table rows well over 100
columns and is merged with CI green.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.