diff --git a/.agents/skills/pr-checklist/SKILL.md b/.agents/skills/pr-checklist/SKILL.md index 7daca797831..213d02ddad7 100644 --- a/.agents/skills/pr-checklist/SKILL.md +++ b/.agents/skills/pr-checklist/SKILL.md @@ -104,6 +104,8 @@ When adding or modifying tests, consider both the coverage and the effectiveness Assess whether the included test cases are meaningful, concise, and maintainable. Testing should not be exhaustive: local validation is typically sufficient to ensure a baseline correctness of the change. Contributed tests should focus exclusively on areas that are deemed prone to errors, regressions, or complex interactions within the codebase. Recommend deletion of redundant, trivial, or otherwise unnecessary tests: this is one of the most common complaints raised during code reviews. +Check added and changed tests against [`tests/AGENTS.md`](../../../tests/AGENTS.md). For reproducible bugs, the PR must show the new test failing without the fix and passing with it. + Verify that testing actually exercises the new path, rather than passing through an unchanged path or fallback. Ensure added speculative decoding correctness tests assert nonzero acceptance rate, selection tests identify the desired backend, etc. Aim to write tests that are meaningful, targeted, and maintainable. Avoid redundant or trivial tests that do not contribute to the overall reliability of the test suite. Avoid using mock objects, especially when highly coupled with internal logic. diff --git a/AGENTS.md b/AGENTS.md index f2e116ae072..9b1e26a987e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -88,7 +88,9 @@ When adding tests: - **Design before you write.** Answer four questions first: what is the module for, what is its I/O contract, what failure am I guarding against, and what is - the cheapest level that catches it (unit over integration over e2e)? + the cheapest level that actually reproduces it? Fixes to user-visible + behavior need a test through a real entry point (`LLM`, OpenAI server); see + [`tests/AGENTS.md`](tests/AGENTS.md). - **Reuse before create.** Extend existing test files, `conftest.py` fixtures, and helpers; add a new file only when no nearby suite fits. - **Test behavior with intent.** Assert observable outcomes through public APIs; diff --git a/tests/AGENTS.md b/tests/AGENTS.md new file mode 100644 index 00000000000..5cd967bb7f3 --- /dev/null +++ b/tests/AGENTS.md @@ -0,0 +1,36 @@ +# tests + +- **Test real objects on the lightest real path.** Construct vLLM objects with + their real constructors; do not stand them in with `SimpleNamespace` or + `MagicMock`, set private attributes, or patch methods to reach a state. Mock + only external services. Pick the cheapest setup that exercises the change: a + layer or kernel test does not need `vllm_runner`, and single-rank parallel + state is enough when parallelism is irrelevant. A fix to user-visible behavior + needs at least one regression test through `LLM` (`vllm_runner`) or + `RemoteOpenAIServer`, with real weights when loading is involved. A test that + only asserts `assert_called*` or "no exception" guards nothing. +- **Show the failure before the fix.** When the bug is reproducible, run the new + test on the base commit (rebuild if the fix touches `csrc/`) and confirm it + fails for the reported reason, then passes with the fix. Paste both commands + and the relevant output in the PR; "fails before" without output is not + evidence. +- **Never buy a green CI.** Do not add `flaky`/reruns, retry loops, `skip`, + `xfail`, lowered thresholds, wider `atol`/`rtol`, or deletions to make a test + pass. Find out whether the failure is caused by your change or also happens + on main (`.buildkite/scripts/rerun-test.sh`, + `docs/contributing/ci/failures.md`); the only exception is + `xfail(strict=True)` linking an open issue for a bug on main. +- **Deterministic and exact by construction.** Use greedy or seeded sampling. + Assert equality where the result is exact (a change that must not alter + output is compared against the code it replaces, not another reference), and + keep tolerances as tight as the math allows. vLLM is not batch invariant by default: batch size, chunked + prefill, prefix caching, backend, and TP size can flip greedy tokens, so + compare against a reference with `check_logprobs_close` / + `check_embeddings_close`, or require `VLLM_BATCH_INVARIANT=1` for exact + equality. Never synchronize with a fixed `sleep`; poll an explicit condition + with a timeout, and force race orderings with events. +- **Make sure CI runs it.** A test no step in `.buildkite/test_areas/*.yaml` + selects never runs, which looks like passing. Check that the step's + `commands` and pytest markers (`core_model`, `slow_test`, `distributed`) + select the test and its `source_file_dependencies` cover the changed files; + update the step if not.