From c5c9eca88958eb2595c9b9d776c2973deb13a363 Mon Sep 17 00:00:00 2001 From: aoshen02 <281812371+aoshen02@users.noreply.github.com> Date: Tue, 20 Oct 2026 10:46:46 +0800 Subject: [PATCH 1/3] [Doc] Raise the bar for agent-written tests Add tests/AGENTS.md with concrete rules for tests: fail-before/pass-after evidence for reproducible bug fixes, at least one real-entry-point test for user-visible fixes, determinism and batch invariance, isolation, specific assertions, no weakening of existing tests, and CI step wiring. Point the root AGENTS.md and the pr-checklist skill at it instead of repeating rules. Co-Authored-By: Claude Opus 5.5 Signed-off-by: aoshen02 <281812371+aoshen02@users.noreply.github.com> --- .agents/skills/pr-checklist/SKILL.md | 2 + AGENTS.md | 15 +++-- tests/AGENTS.md | 97 ++++++++++++++++++++++++++++ 3 files changed, 109 insertions(+), 5 deletions(-) create mode 100644 tests/AGENTS.md diff --git a/.agents/skills/pr-checklist/SKILL.md b/.agents/skills/pr-checklist/SKILL.md index ae3099b6a076..f46c7c25cef5 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 a31d156b4b63..64da0f719b6d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -81,16 +81,19 @@ uv pip install -r requirements/test/cuda.in .venv/bin/python -m pytest tests/path/to/test_file.py -v ``` -When adding tests: +When adding tests, follow [`tests/AGENTS.md`](tests/AGENTS.md): +- **Prove bug fixes.** When the bug is reproducible, the new test must fail + without the fix and pass with it; show both runs in the PR. - **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? User-visible fixes need at + least one test through a real entry point (`LLM`, OpenAI server). - **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; - state why in the name or docstring. Skip trivial wiring; flaky tests are worse - than no tests. +- **Test behavior with intent.** Assert specific observable outcomes through + public APIs; state why in the name or docstring. Skip trivial wiring; flaky + tests are worse than no tests. - **Keep it minimal.** One behavior per test and the smallest setup that triggers it; if the test diff dwarfs the code change, cut scope. - **No one-off kernel benchmarks in `tests/`.** Put kernel perf work in @@ -151,6 +154,8 @@ Security reviewers should start with [`SECURITY.md`](SECURITY.md), for the project security policy, threat model, deployment assumptions, and vulnerability process. +- **Writing or changing tests**: [`tests/AGENTS.md`](tests/AGENTS.md) + — Bug-fix evidence, test level, determinism, isolation, and CI wiring. - **Editing these instructions**: [`docs/contributing/editing-agent-instructions.md`](docs/contributing/editing-agent-instructions.md) — Rules for modifying AGENTS.md or any domain-specific guide it references. diff --git a/tests/AGENTS.md b/tests/AGENTS.md new file mode 100644 index 000000000000..05c8929038f5 --- /dev/null +++ b/tests/AGENTS.md @@ -0,0 +1,97 @@ +# tests + +Read this before adding or changing tests. A test is worth merging only if it +fails when the behavior it guards breaks and passes every time otherwise. + +## Bug fixes: show the test catches the bug + +When the bug is reproducible, run the new test on the base commit (without the +fix) and confirm it fails for the reported reason: read the assertion or +traceback, not just the exit code. Then confirm it passes with the fix. If the +fix touches `csrc/`, rebuild before each run. Put both commands and the relevant +output in the PR's Validation section; "tests pass" alone is not evidence. + +If no automated test can reproduce the bug (hardware, checkpoint size, timing), +say so and give the manual reproducer with before/after output. + +## Pick the level + +- Use the cheapest level that actually reproduces the problem. +- A fix to user-visible behavior needs at least one test through a real entry + point: `LLM` via the `vllm_runner` fixture, or the OpenAI server via + `RemoteOpenAIServer` (`tests/utils.py`). Testing an extracted helper does not + show that a scheduler, model runner, or sampler fix works for users. +- Kernel tests compare against a reference implementation with explicit + tolerances; custom ops also go through `opcheck` (`tests/kernels/utils.py`). +- Mock only boundary dependencies (external services, absent hardware), never + the code under test. A test that only asserts `assert_called*` or that no + exception was raised is not accepted. +- Keep end-to-end tests cheap: the smallest model already used in that area, + short prompts, small `max_tokens` and `max_model_len`, and `enforce_eager=True` + unless the behavior involves CUDA graphs or `torch.compile`. +- If you cannot name a code change that would turn the test red, it guards + nothing; drop it. + +## Determinism + +- Use greedy sampling or a fixed `SamplingParams(seed=...)`; seed random inputs + with `set_random_seed`. Pin settings that change numerics (dtype, attention + backend, chunked prefill, prefix caching) instead of relying on defaults. +- Compare against a reference with explicit tolerances: HF via `hf_runner` and + `check_logprobs_close` (`tests/models/utils.py`), or + `torch.testing.assert_close` against a PyTorch reference. vLLM is not batch + invariant by default: batch size, chunked prefill, prefix-cache hits, backend, + and TP size change kernel shapes and can flip greedy tokens. Assert exact + tokens or bitwise equality across such changes only with + `VLLM_BATCH_INVARIANT=1` (see `docs/features/batch_invariance.md`). +- Do not synchronize with fixed `sleep`s; poll an explicit condition (health + endpoint, future, event) with a timeout. Reproduce concurrency bugs by forcing + the ordering (events, barriers), not by hoping for it. Let + `RemoteOpenAIServer` pick its port. Serve URL inputs from the + `local_asset_server` fixture rather than the internet. +- Do not hide flakiness with new `@pytest.mark.flaky(reruns=...)` markers, retry + loops, or wider tolerances; find the cause. For tests involving timing, async + code, or multiple processes, run them repeatedly before submitting + (`.buildkite/scripts/rerun-test.sh`, see `docs/contributing/ci/failures.md`) + and report the number of passing runs in the PR. + +## Isolation + +- Create engines in context managers (`with vllm_runner(...) as llm:`, + `with RemoteOpenAIServer(...) as server:`) so GPU memory and child processes + are released before the next test. +- Use `@create_new_process_for_each_test()` (`tests/utils.py`) when a test + initializes distributed state or other process-global state. +- Pass server environment variables through `RemoteOpenAIServer(..., + env_dict=...)` (works in module-scoped fixtures, unlike `monkeypatch`). In + process, prefer `monkeypatch.setenv` or `patch.dict(os.environ, ...)` so + changes are undone; plain assignment is fine only in a child process's entry + point. +- Declare hardware needs with `@multi_gpu_test(num_gpus=...)` and + `@large_gpu_test(min_gb=...)`. + +## Assertions + +- Assert the expected result: tokens or text, field values, status code and + error message, not `len(out) > 0` or `out is not None`. +- Prove the new path ran rather than a fallback: the selected backend, a log + line (`caplog_vllm`), or a nonzero spec-decode acceptance rate. +- Include inputs and both values in failure messages, or use a helper that + prints a diff. + +## Do not weaken existing tests + +- Do not delete, loosen, or skip an existing test to make a PR pass. If the + expected behavior changes on purpose, explain why in the PR. +- Use `skip` only for unsupported environments. Mark a known bug with `xfail` + linking the issue, so the test reports when it starts passing. + +## Placement and CI + +- Put tests in the `tests/` subdirectory that mirrors the code under test and + extend an existing file when one fits. +- CI runs only what a step in `.buildkite/test_areas/*.yaml` selects. Check + that a step's `commands` run your test and its `source_file_dependencies` + cover the changed source paths; update the step if not. A test no step + selects never runs, which looks the same as passing. If CI hardware cannot + run the test, say so in the PR and show a local run. From fada50038579b9e7722a5dc128d294e2f0e6964d Mon Sep 17 00:00:00 2001 From: aoshen02 <281812371+aoshen02@users.noreply.github.com> Date: Tue, 20 Oct 2026 11:05:23 +0800 Subject: [PATCH 2/3] [Doc] Trim test guide to rules that change agent behavior Keep only the rules that differed in agent dry runs: real-path regression tests for user-visible fixes, before/after evidence, no reruns or loosened assertions, determinism without batch invariance, and CI step selection. Co-Authored-By: Claude Opus 5.5 Signed-off-by: aoshen02 <281812371+aoshen02@users.noreply.github.com> --- AGENTS.md | 17 +++---- tests/AGENTS.md | 124 +++++++++++------------------------------------- 2 files changed, 36 insertions(+), 105 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 64da0f719b6d..e711a28b3ca4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -81,19 +81,18 @@ uv pip install -r requirements/test/cuda.in .venv/bin/python -m pytest tests/path/to/test_file.py -v ``` -When adding tests, follow [`tests/AGENTS.md`](tests/AGENTS.md): +When adding tests: -- **Prove bug fixes.** When the bug is reproducible, the new test must fail - without the fix and pass with it; show both runs in the PR. - **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 actually reproduces it? User-visible fixes need at - least one test through a real entry point (`LLM`, OpenAI server). + 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 specific observable outcomes through - public APIs; state why in the name or docstring. Skip trivial wiring; flaky - tests are worse than no tests. +- **Test behavior with intent.** Assert observable outcomes through public APIs; + state why in the name or docstring. Skip trivial wiring; flaky tests are worse + than no tests. - **Keep it minimal.** One behavior per test and the smallest setup that triggers it; if the test diff dwarfs the code change, cut scope. - **No one-off kernel benchmarks in `tests/`.** Put kernel perf work in @@ -154,8 +153,6 @@ Security reviewers should start with [`SECURITY.md`](SECURITY.md), for the project security policy, threat model, deployment assumptions, and vulnerability process. -- **Writing or changing tests**: [`tests/AGENTS.md`](tests/AGENTS.md) - — Bug-fix evidence, test level, determinism, isolation, and CI wiring. - **Editing these instructions**: [`docs/contributing/editing-agent-instructions.md`](docs/contributing/editing-agent-instructions.md) — Rules for modifying AGENTS.md or any domain-specific guide it references. diff --git a/tests/AGENTS.md b/tests/AGENTS.md index 05c8929038f5..ab9842d29a7d 100644 --- a/tests/AGENTS.md +++ b/tests/AGENTS.md @@ -1,97 +1,31 @@ # tests -Read this before adding or changing tests. A test is worth merging only if it -fails when the behavior it guards breaks and passes every time otherwise. - -## Bug fixes: show the test catches the bug - -When the bug is reproducible, run the new test on the base commit (without the -fix) and confirm it fails for the reported reason: read the assertion or -traceback, not just the exit code. Then confirm it passes with the fix. If the -fix touches `csrc/`, rebuild before each run. Put both commands and the relevant -output in the PR's Validation section; "tests pass" alone is not evidence. - -If no automated test can reproduce the bug (hardware, checkpoint size, timing), -say so and give the manual reproducer with before/after output. - -## Pick the level - -- Use the cheapest level that actually reproduces the problem. -- A fix to user-visible behavior needs at least one test through a real entry - point: `LLM` via the `vllm_runner` fixture, or the OpenAI server via - `RemoteOpenAIServer` (`tests/utils.py`). Testing an extracted helper does not - show that a scheduler, model runner, or sampler fix works for users. -- Kernel tests compare against a reference implementation with explicit - tolerances; custom ops also go through `opcheck` (`tests/kernels/utils.py`). -- Mock only boundary dependencies (external services, absent hardware), never - the code under test. A test that only asserts `assert_called*` or that no - exception was raised is not accepted. -- Keep end-to-end tests cheap: the smallest model already used in that area, - short prompts, small `max_tokens` and `max_model_len`, and `enforce_eager=True` - unless the behavior involves CUDA graphs or `torch.compile`. -- If you cannot name a code change that would turn the test red, it guards - nothing; drop it. - -## Determinism - -- Use greedy sampling or a fixed `SamplingParams(seed=...)`; seed random inputs - with `set_random_seed`. Pin settings that change numerics (dtype, attention - backend, chunked prefill, prefix caching) instead of relying on defaults. -- Compare against a reference with explicit tolerances: HF via `hf_runner` and - `check_logprobs_close` (`tests/models/utils.py`), or - `torch.testing.assert_close` against a PyTorch reference. vLLM is not batch - invariant by default: batch size, chunked prefill, prefix-cache hits, backend, - and TP size change kernel shapes and can flip greedy tokens. Assert exact - tokens or bitwise equality across such changes only with - `VLLM_BATCH_INVARIANT=1` (see `docs/features/batch_invariance.md`). -- Do not synchronize with fixed `sleep`s; poll an explicit condition (health - endpoint, future, event) with a timeout. Reproduce concurrency bugs by forcing - the ordering (events, barriers), not by hoping for it. Let - `RemoteOpenAIServer` pick its port. Serve URL inputs from the - `local_asset_server` fixture rather than the internet. -- Do not hide flakiness with new `@pytest.mark.flaky(reruns=...)` markers, retry - loops, or wider tolerances; find the cause. For tests involving timing, async - code, or multiple processes, run them repeatedly before submitting - (`.buildkite/scripts/rerun-test.sh`, see `docs/contributing/ci/failures.md`) - and report the number of passing runs in the PR. - -## Isolation - -- Create engines in context managers (`with vllm_runner(...) as llm:`, - `with RemoteOpenAIServer(...) as server:`) so GPU memory and child processes - are released before the next test. -- Use `@create_new_process_for_each_test()` (`tests/utils.py`) when a test - initializes distributed state or other process-global state. -- Pass server environment variables through `RemoteOpenAIServer(..., - env_dict=...)` (works in module-scoped fixtures, unlike `monkeypatch`). In - process, prefer `monkeypatch.setenv` or `patch.dict(os.environ, ...)` so - changes are undone; plain assignment is fine only in a child process's entry - point. -- Declare hardware needs with `@multi_gpu_test(num_gpus=...)` and - `@large_gpu_test(min_gb=...)`. - -## Assertions - -- Assert the expected result: tokens or text, field values, status code and - error message, not `len(out) > 0` or `out is not None`. -- Prove the new path ran rather than a fallback: the selected backend, a log - line (`caplog_vllm`), or a nonzero spec-decode acceptance rate. -- Include inputs and both values in failure messages, or use a helper that - prints a diff. - -## Do not weaken existing tests - -- Do not delete, loosen, or skip an existing test to make a PR pass. If the - expected behavior changes on purpose, explain why in the PR. -- Use `skip` only for unsupported environments. Mark a known bug with `xfail` - linking the issue, so the test reports when it starts passing. - -## Placement and CI - -- Put tests in the `tests/` subdirectory that mirrors the code under test and - extend an existing file when one fits. -- CI runs only what a step in `.buildkite/test_areas/*.yaml` selects. Check - that a step's `commands` run your test and its `source_file_dependencies` - cover the changed source paths; update the step if not. A test no step - selects never runs, which looks the same as passing. If CI hardware cannot - run the test, say so in the PR and show a local run. +- **User-visible fixes need a real-path test.** Use the cheapest level that + reproduces the problem, but a fix to user-visible behavior needs at least one + regression test through `LLM` (`vllm_runner`) or the OpenAI server + (`RemoteOpenAIServer`), not only a unit test on hand-built inputs. Mock only + external dependencies; 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 `@pytest.mark.flaky(reruns=...)`, retry + loops, wider tolerances, lowered thresholds, `skip`, 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`). Mark a known bug with `xfail` linking the + issue, not `skip`. +- **Deterministic by construction.** Use greedy or seeded sampling. 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 tolerances (`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. From 05636b5e9fc5e3e4960819e35120f022ee391391 Mon Sep 17 00:00:00 2001 From: aoshen02 <281812371+aoshen02@users.noreply.github.com> Date: Wed, 21 Oct 2026 10:56:14 +0800 Subject: [PATCH 3/3] [Doc] Require real objects and exact references in agent tests Address review: no SimpleNamespace/MagicMock stand-ins for vLLM objects, use the lightest real path instead of always vllm_runner, forbid xfail and wider atol/rtol to pass, and compare output-preserving changes against the code they replace. Co-Authored-By: Claude Opus 5.5 Signed-off-by: aoshen02 <281812371+aoshen02@users.noreply.github.com> --- tests/AGENTS.md | 43 ++++++++++++++++++++++++------------------- 1 file changed, 24 insertions(+), 19 deletions(-) diff --git a/tests/AGENTS.md b/tests/AGENTS.md index ab9842d29a7d..5cd967bb7f33 100644 --- a/tests/AGENTS.md +++ b/tests/AGENTS.md @@ -1,29 +1,34 @@ # tests -- **User-visible fixes need a real-path test.** Use the cheapest level that - reproduces the problem, but a fix to user-visible behavior needs at least one - regression test through `LLM` (`vllm_runner`) or the OpenAI server - (`RemoteOpenAIServer`), not only a unit test on hand-built inputs. Mock only - external dependencies; a test that only asserts `assert_called*` or "no - exception" guards nothing. +- **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 `@pytest.mark.flaky(reruns=...)`, retry - loops, wider tolerances, lowered thresholds, `skip`, 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`). Mark a known bug with `xfail` linking the - issue, not `skip`. -- **Deterministic by construction.** Use greedy or seeded sampling. 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 tolerances (`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. +- **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`)