Skip to content

[Doc] Raise the bar for agent-written tests #62601

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aoshen02 merged 4 commits into from
Oct 21, 2026
+41 −1
Merged

[Doc] Raise the bar for agent-written tests #62601

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c5c9eca
[Doc] Raise the bar for agent-written tests
aoshen02 Oct 20, 2026
fada500
[Doc] Trim test guide to rules that change agent behavior
aoshen02 Oct 20, 2026
05636b5
[Doc] Require real objects and exact references in agent tests
aoshen02 Oct 21, 2026
2997617
Merge branch 'main' into docs/agents-test-quality
aoshen02 Oct 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .agents/skills/pr-checklist/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
4 changes: 3 additions & 1 deletion AGENTS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,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;
Expand Down
31 changes: 31 additions & 0 deletions tests/AGENTS.md
View file
Open in desktop

@tomasruizt tomasruizt Oct 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be general rules to follow and preferences, but not strict enforcements. Otherwise, agents will get stuck trying to fulfill requirements. I'm thinking concretely of:

  • Objects that are really hard to instantiate atm, so MagicMock might be needed in special cases.
  • Statistical tests that check a p-value, confidence intervals, code paths that check temp != 0. These seem to clash with the requirement to always run greedy/deterministic tests.

Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
# 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.

@fxmarty-amd fxmarty-amd Oct 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have mixed opinion on this. vllm_runner can be quite slow in the CI (#53291) and is not always necessary.

Something like:

vllm/tests/quantization/test_online.py

Lines 1002 to 1008 in a6c898e

# This is faster than `dist_init` arg.
monkeypatch.setattr(
"vllm.model_executor.parameter.get_tensor_model_parallel_rank", lambda: 0
)
monkeypatch.setattr(
"vllm.model_executor.parameter.get_tensor_model_parallel_world_size", lambda: 1
)

vllm/tests/quantization/test_online.py

Line 1804 in a6c898e

model, vllm_config = load_model_without_vllm_runner(

& generally (not on vllm-project/vllm) yet:

@pytest.fixture
def single_rank_parallel_state(monkeypatch: pytest.MonkeyPatch):
    """Single rank parallel groups without initializing a process group.

    This is faster than the `dist_init` fixture.
    """
    for name in ("_WORLD", "_TP", "_ETP", "_DCP", "_PP", "_DP", "_EP", "_PCP"):
        monkeypatch.setattr(
            f"vllm.distributed.parallel_state.{name}", _SingleRankGroup()
        )
    monkeypatch.setattr(
        "vllm.model_executor.parameter.get_tensor_model_parallel_rank", lambda: 0
    )
    monkeypatch.setattr(
        "vllm.model_executor.parameter.get_tensor_model_parallel_world_size", lambda: 1
    )

that is faster than dist_init fixture.

Instead of vllm_runner in cases tests do not read parallelism to be meaningful.

However, there are tests that need real weight loading and not only DummyModelLoader.

- **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`,

@fxmarty-amd fxmarty-amd Oct 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xfail as well

`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.

@fxmarty-amd fxmarty-amd Oct 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also personally prompt not to arbitrarily increase atol/rtol to make tests pass, when they are expected to have strict equality or small atol/rtol. (https://github.com/vllm-project/vllm/pull/59766)

@aoshen02 aoshen02 Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

- **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.
Loading