# Review Topic Test Hygiene

> Review topic: Test hygiene & coverage of changed behavior

- Skill: `roboflow/review-topic-test-hygiene` (Agent Skill)
- Install (CLI): `npx skillmds@latest add roboflow/review-topic-test-hygiene`
- Raw SKILL.md: https://api.skillmd.com/api/skills/roboflow/review-topic-test-hygiene/raw
- Safety review: pending
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: Coding & Dev Tools
- Author: roboflow (https://skillmd.com/u/roboflow)
- Updated: 2026-09-17
- Page: https://skillmd.com/skills/roboflow/review-topic-test-hygiene

---


# Review topic: Test hygiene & coverage of changed behavior

## When this applies
Load and apply this skill whenever the diff shows any of:
- New/modified runtime code (block, model I/O contract, endpoint, SDK/CLI path, execution-engine rule) with **no new/changed test**, or only a test that mocks out the very thing that changed.
- A new/edited dispatch on a runtime **selector**: enum→lambda maps (e.g. `PROPERTIES_EXTRACTORS`), `if kind == ...`, block `type`/`property_name` switches, `get_execution_engine_compatibility`.
- A new `@pytest.mark.skipif`/`skip`/`slow` or env-gated test (`os.getenv("SKIP_*_TESTS", True)`, `RUN_*`).
- New global test state: module-level `monkeypatch`, `sys.modules[...] = stub`, `os.environ[...] =` at import time, or `autouse` fixtures mutating shared singletons/registries.
- A GPU/hardware-only test whose CI runner is unclear.

## Review checklist
Tag each finding BLOCK / FLAG / NIT. See **Standards** below for the one canonical statement of each rule.

- **BLOCK** — Changed runtime behavior ships with no test that actually runs in the PR's x86 CI (Standard 1), OR a bugfix lands with no regression test reproducing the bug.
- **BLOCK** — A new env-gated / GPU test's gate is never flipped `False` by any CI job → dead test giving false confidence (Standard 4).
- **BLOCK** — A new `inference_models/tests/unit_tests` `gpu_only` test lacks a dependency marker selected by `.github/workflows/unit_tests_inference_experimental_gpu.yml`, so no GPU job collects it (Standard 4).
- **BLOCK** — A leaked module-level `monkeypatch`, `sys.modules` stub, or mutated global registry corrupts other tests (Standard 3).
- **FLAG** — A new selector/branch/enum value is exercised only by literals, not by a test driving *that* runtime value (Standard 2).
- **FLAG** — Mock-only test that asserts the mock's own return value rather than the changed output (Standard 6).
- **FLAG** — New marker used in the diff but not declared in `pytest.ini` → silent no-op deselection (Standard 5).
- **NIT** — Brittle exact-value assertion on a numerically unstable op (Standard 7).
- **NIT** — Broken/stale test asset, stub, or a test not updated alongside the API it covers.

### Not blocking
- Do NOT demand a test that duplicates an existing behavior test at a lower layer (e.g. a unit test when the same path is already driven end-to-end by an integration test that runs in CI).
- Do NOT demand exact-float assertions on model predictions — shape/contract/tolerance is the correct bar; over-tight asserts get muted and erode coverage.
- Do NOT block a GPU/hardware test for running only on the self-hosted runner *if* the same behavior also has an x86-CI test; the GPU test is additive.
- Pure refactors with no behavior change need no new test, only that existing tests still pass and were updated to the new signature.

## Standards

1. **Changed behavior is verified by a test that runs in the PR's CI.** Map each behavior in the diff to the covering test AND the CI workflow that executes it:
   - unit workflows → `unit_tests_workflows_x86.yml` (`tests/workflows/unit_tests`)
   - workflow execution → `integration_tests_workflows_x86.yml`
   - `inference_models` I/O → `integration_tests_inference_models.yml` (`tests/inference/models_predictions_tests`)
   - HTTP server → `integration_tests_inference_server_x86.yml` (`tests/inference/integration_tests`)
   - SDK → `unit_tests_inference_sdk_x86.yml` (`tests/inference_sdk/unit_tests`)
   - CLI → `unit_tests_inference_cli_x86.yml` (`tests/inference_cli/unit_tests`)

   A test that is skipped-by-default or lives only on the self-hosted GPU runner (`test.nvidia_t4.yml`, `test.jetson_*.yml`) does NOT protect a change merged through x86 CI.

2. **Selectors, not just literals.** Blocks/UQL dispatch on runtime values (`PROPERTIES_EXTRACTORS` in `inference/core/workflows/core_steps/common/query_language/operations/detections/base.py`). For every branch / enum value / `property_name` / `kind` added, a test must supply *that* runtime value through the block and assert its output. One test per new branch is the bar (PR #1129 added the anchor `property_name`s plus one behavior test each).

3. **Test isolation.** Monkeypatching goes through the function-scoped `monkeypatch` fixture (auto-undone), never module-level assignment. Module stubs belong in a registered `stub_plugins/` package, not `sys.modules` hacks. `autouse` fixtures must restore any shared global they touch.

4. **Gate direction and GPU-unit routing.** For any new `skipif`/env gate, grep the workflows for the var and confirm a job sets it so the test actually runs (e.g. `SKIP_SAM2_TESTS=False` in `test.nvidia_t4.yml`, `SKIP_LMM_TEST` in `integration_tests_inference_server_x86.yml`). A gate that defaults to skip and is never flipped is a dead test. GPU-only tests under `inference_models/tests/unit_tests` must carry `gpu_only` plus the dependency marker for the environment they require (for example, `trt_extras`). Confirm that `.github/workflows/unit_tests_inference_experimental_gpu.yml` installs that dependency group and selects the same marker intersection. The ordinary CPU unit workflow intentionally deselects every `gpu_only` test.

5. **Marker correctness.** Any new marker used in the diff must be declared in the relevant `pytest.ini`. `inference_models/pytest.ini` registers its hardware and dependency markers separately from the repository-root configuration. Unregistered markers silently no-op the intended deselection (PR #1724 had to add `workflows`).

6. **Real coverage over mock theater.** The assertion must observe the changed output, not a mock return value the test itself set. A test whose mocks reproduce the exact code under change asserts nothing about the change.

7. **Assertion stability.** Model-prediction tests assert shape/contract/tolerances, not brittle exact floats on numerically unstable ops (PRs #2026, #1646, #1394).

## Example implementations (point authors here)
- `tests/workflows/unit_tests/core_steps/formatters/test_property_extraction.py` — canonical **selector coverage**: one `test_property_extraction_block_with_<anchor>` per runtime `property_name`, driven through the block, asserting real output (PR #1129).
- `tests/workflows/integration_tests/execution/test_workflow_top_prediction.py` — **behavior-level execution test**: full workflow compiled + run with `$inputs.*`/`$steps.*` selectors, covering non-empty and empty-detections paths.
- `tests/workflows/integration_tests/execution/stub_plugins/` — **isolated module stubs done right**: fake blocks in registered plugin packages loaded per-test, not `sys.modules` monkeypatching.
- `tests/inference/unit_tests/conftest.py` (`_patch_aioresponses_stream_writer`, `autouse=True` + `monkeypatch`) — **scoped, auto-undone patching**; contrast with module-level global assignment.
- `tests/inference/integration_tests/test_sam2.py` (`SKIP_SAM2_TESTS` gate) + `.github/workflows/test.nvidia_t4.yml` (sets `SKIP_SAM2_TESTS=False`) — the **gate-plus-runner pair** for checking any new env-gated test has a CI job that flips it.
- `inference_models/tests/unit_tests/models/rfdetr/test_triton_universal_preprocess_runtime.py` (`gpu_only` + `trt_extras`) + `.github/workflows/unit_tests_inference_experimental_gpu.yml` — canonical GPU-only unit-test routing through a dependency marker.
- The relevant `pytest.ini` — repository-root and `inference_models` suites register different marker sets; add every new marker to the configuration governing its test path.

## Reference PRs
- #1129 — selector coverage established (one behavior test per new `property_name`).
- #1724 — added the `workflows` marker to `pytest.ini`.
- #2526 — fixed a test left on an old `run_workflow(...)` signature after args moved to the `WorkflowRunner` constructor.
- #2168, #1539 — broken/missing test asset and stale stub after a dependency move.
- #2026, #1646, #1394, #1100, #1044, #1033 — loosened brittle exact-value prediction assertions.