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.