Skip to content

Performance "baselines" in tests/perf/ cannot fail: no assertions, no stored baseline, and disabled under the default -n auto #511

Description

@kgdunn

Summary

The tests in tests/perf/ describe themselves as tracking performance regressions, but they contain no assertions, there is no stored baseline to compare against, and pytest-benchmark disables itself under the repo's default -n auto. They pass unless the function under test raises - they are smoke tests wearing a performance-test label.

Where

Line numbers as of ac52aab (branch claude/repo-error-audit-si62bq, PR #500).

  • tests/perf/test_pca_pls_attrs_perf.py - 0 assert statements
  • tests/perf/test_random_helper_perf.py - 0 assert statements
  • pytest.ini:23 - addopts includes -n auto
  • CONTRIBUTING.md - describes the performance CI job as planned

Evidence

Every test has the shape:

def test_pca_transform_baseline(benchmark) -> None:
    """... These baselines track regressions on those paths ..."""
    benchmark(model.transform, x)

grep -c assert tests/perf/*.py returns 0 for both modules. There is no --benchmark-compare / --benchmark-compare-fail configuration and no committed .benchmarks/ baseline. Under -n auto, pytest-benchmark emits PytestBenchmarkWarning: Benchmarks are automatically disabled because xdist plugin is active and does not time anything.

Why it is wrong

The module docstrings claim these tests "track regressions", and a reader (or a future agent) will believe a performance regression would be caught. Nothing would be. A 100x slowdown in PCA.transform passes green.

Suggested fix

Pick one of three honest options - all are acceptable, silently doing nothing is not:

  1. Make them real. Add a dedicated CI job that runs pytest tests/perf -p no:xdist --benchmark-only against a committed baseline with --benchmark-compare-fail=mean:20%. Requires deciding how to store/refresh baselines and accepting some noise on shared runners.
  2. Assert on complexity, not wall-clock. Replace timing with scaling assertions that are robust on shared CI (e.g. scores_ access after fit performs no re-computation; the lazy frame is built once and cached; transform on 10x rows costs < 30x). This is what the ENG-18 lazy-frame work actually wants protected.
  3. Demote honestly. Keep them as smoke tests, delete the "tracks regressions" language from the docstrings, and drop the benchmark fixture.

Option 2 is the best value here: it catches the regressions that matter (accidental O(n^2), losing a cache) without baseline-management overhead or flakiness.

Acceptance criteria

  • Either the perf tests can fail on a real regression (demonstrate by temporarily introducing one), or their docstrings no longer claim regression tracking.
  • If kept as benchmarks: they run in a job where pytest-benchmark is actually enabled (not under -n auto), and the comparison threshold is documented.
  • CONTRIBUTING.md's performance section matches whatever is implemented.

Scope / non-goals

Do not add a benchmark gate to the main test job - noise on shared runners would make the suite flaky. Keep any timing work in its own job.

References

Found during the repo-wide audit in PR #500. Related: the ENG-18 lazy-frame work these tests were written to protect.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions