Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
33 changes: 32 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,38 @@ in; the remaining function tiers and the evaluation budget are not.
scan now covers them too.

### Changed
- **A bare `pytest` is 33% faster**, 58.8 s to 39.6 s measured on one machine, and CI still runs
everything. Two tests were **17.9 s of a 58.8 s suite**: `test_the_table` and `test_the_json`
each run `scripts/limits.py --quick` as a subprocess, and the script does real timing work,
which is the point of it.

They are marked `slow` and deselected from the inner loop. `scripts/lanes.py` passes
`--runslow` in the `fast`, `compat` and `sdist` lanes, so **nothing stops running where a green
result is meant to mean the suite passed**, and `test_regression_limits_the_script_still_runs_in_ci`
asserts both ends of that: the lane's command carries the flag and CI invokes the lane. A
marker that quietly stopped running in CI would be strictly worse than the ten seconds it saved.

A slow suite changes engineering behaviour long before it fails a gate: people stop running it,
batches get larger, and feedback time becomes the real constraint. `pytest --runslow` runs the
lot, and `pytest -m slow` runs only these two.

Two cheaper-looking fixes were rejected. Adding a `--both` mode to `scripts/limits.py` so one
invocation serves both tests halves the cost and means changing a published script's command
line so a test can run faster, which is the wrong way round: the script is documented in
[Performance](docs/performance.md) as the thing a reader runs to reproduce the table on their
own machine, and its interface belongs to them. **Reducing what `--quick` measures is ruled
out**: cutting sample counts is how a measurement becomes a flake, this repository has already
fixed one flake in exactly that file for exactly that reason, and
`test_regression_limits_the_samples_were_not_quietly_reduced` now reads the script and asserts
it still takes the minimum of five rather than a median of three.

- **The suite now reports its own wall time and refuses a step change in it.** Printed on every
full run, with a 240 s tripwire beside it, which is roughly four times the number it protects.
A ceiling near the measurement fails on a busy machine and gets deleted rather than
investigated; what this catches is somebody adding a test that takes a minute. It arms only for
the default selection, because a subset is faster by construction and timing one against a
whole-suite ceiling would be measuring nothing.

- **An evaluator compiles each source once instead of on every call**, which is **13 to 14x** on a
rule that does not touch a collection. `evaluate()` parsed, rewrote and validated the source on
every call and threw the result away; all of that depends only on `(source, registry)`, and the
Expand Down Expand Up @@ -341,7 +373,6 @@ in; the remaining function tiers and the evaluation budget are not.
accepts expression text from an untrusted source would otherwise hold an unbounded allocation
keyed by that text.

### Changed
- **The benchmark suite runs in CI, against a threshold measured on the runner.** It had been in
the repository since the tier work, with seven saved baselines and a documented 10% gate, and
**none of it ran anywhere**: `measure` is not a default dependency group, so `uv sync --frozen`
Expand Down
10 changes: 10 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -297,4 +297,14 @@ testpaths = ["tests"]
norecursedirs = ["*.egg", ".*", "build", "dist", "_tmp", "node_modules"]
addopts = "-ra --strict-markers --strict-config"
xfail_strict = true
# `--strict-markers` is on, so every marker this suite applies has to be declared. `benchmark` and
# `limit_memory` come from their plugins; `slow` is ours.
#
# **`slow` is deselected by default and run in full by CI.** `tests/conftest.py` carries the
# argument; the short version is that two tests running a real measurement script as a subprocess
# were a third of the suite, and a slow suite changes engineering behaviour long before it fails a
# gate. `scripts/lanes.py` passes `--runslow` in the lanes CI runs.
markers = [
"slow: runs a real measurement as a subprocess; deselected unless --runslow (CI passes it)",
]
filterwarnings = ["error"]
8 changes: 7 additions & 1 deletion scripts/check_sdist.py
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,14 @@ def main() -> int:
print("==> running the shipped suite from the unpacked sdist")
# From the unpacked directory, so `testpaths` and every path a test resolves relative to
# its own location point inside the distribution rather than back at the checkout.
#
# `--runslow` for the same reason the `fast` lane passes it: two tests are deselected from a
# developer's inner loop and must run wherever a green result is meant to mean the suite
# passed. This lane is also the one that would notice `tests/conftest.py` or `scripts/`
# falling out of the distribution, since the flag is defined in the first and the tests it
# re-selects run the second.
_run(
[str(python), "-m", "pytest", "-q", "--no-header"],
[str(python), "-m", "pytest", "-q", "--no-header", "--runslow"],
cwd=unpacked,
quiet=False,
diagnosis=(
Expand Down
23 changes: 18 additions & 5 deletions scripts/lanes.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,19 @@ class Lane:
),
Lane(
name="fast",
checks="the unit suite on the development interpreter",
needs="uv sync --frozen",
command=("pytest",),
checks=(
"the unit suite on the development interpreter, including the tests a bare `pytest` "
"leaves out"
),
needs=(
"uv sync --frozen. **`--runslow` is not optional here and that is the point of it.** "
"Two tests run `scripts/limits.py` as a subprocess and were a third of the suite's "
"wall time, so they are deselected from a developer's inner loop and run in full "
"wherever a green result has to mean something. A marker that quietly stopped running "
"in CI would be strictly worse than the ten seconds it saved, and "
"`tests/test_limits.py` asserts this flag is here"
),
command=("pytest", "--runslow"),
),
Lane(
name="corpus",
Expand Down Expand Up @@ -130,9 +140,12 @@ class Lane:
),
needs=(
"an environment built for the matrix row's interpreter, which is what `_resolve` "
"below looks for beside `sys.executable` before it falls back to `.venv/`"
"below looks for beside `sys.executable` before it falls back to `.venv/`. "
"`--runslow` for the same reason as `fast`: the limits the script measures are "
"interpreter behaviour, so the matrix is exactly where those two tests are worth "
"their wall time"
),
command=("pytest",),
command=("pytest", "--runslow"),
),
)

Expand Down
118 changes: 118 additions & 0 deletions tests/conftest.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
"""What the default selection is, and how long it is allowed to take.

**Two tests were a third of the suite.** `test_the_table` and `test_the_json` each run
`scripts/limits.py --quick` as a subprocess, and the script does real timing work, which is the
point of it. Together they were 17.9 s of a 58.8 s run. They are not slow by accident and they are
not wasteful: `--quick` already skips the 100,000-item runs, and cutting the sample counts is how a
measurement becomes a flake, which this repository has already fixed once in exactly this file.

So they are **deselected from the inner loop and run in full in CI**, which puts the cost where it
belongs. `scripts/lanes.py` passes `--runslow` in the `fast` and `compat` lanes, and
`tests/test_limits.py::test_regression_limits_the_script_still_runs_in_ci` asserts that it does. A
marker that quietly stopped running in CI would be strictly worse than the ten seconds it saved.

Three options were weighed and this is the second of them. Adding a `--both` mode to
`scripts/limits.py` so one invocation serves both tests would halve the cost, and it means changing
a published script's command line so that a test can run faster, which is the wrong way round: the
script is documented in `docs/performance.md` as the thing a reader runs to reproduce the table on
their own machine, and its interface belongs to them. **Reducing what `--quick` measures is ruled
out**, and the reason is recorded so it is not re-proposed as the obvious cheap one next quarter.

The other 2,407 tests run in about 41 s, which is 17 ms each, and there is nothing wrong with them.
The three next-slowest are load-bearing and they stay: a denial-of-service regression test that has
to do real work to prove anything, and two differential tests that generate expressions across the
whole allowlist.
"""

from __future__ import annotations

import time
from typing import Any

import pytest

# **A step change, not drift.** The default selection measures about 41 s and the full one about
# 59 s, both on the machine this was written on and both with benchmarks disabled; with them
# enabled the full run is about 90 s, and under `--cov` about 103 s. The ceiling has to sit above
# all of those and above a hosted runner, which is slower again.
#
# 240 s is roughly four times the number this is protecting. That is deliberate: a tripwire close
# to the measurement fails on a busy machine, and a check that fails for a reason nobody can act on
# gets deleted rather than investigated. What this catches is somebody adding a test that takes a
# minute, which is the thing that actually happens.
MAX_SUITE_SECONDS = 240.0

_STARTED = "safeexpr_suite_started"


def pytest_addoption(parser: pytest.Parser) -> None:
"""Add `--runslow`, which re-selects what the inner loop leaves out."""
parser.addoption(
"--runslow",
action="store_true",
default=False,
help="also run tests marked `slow`; CI passes this, the inner loop does not",
)


def pytest_collection_modifyitems(config: pytest.Config, items: list[pytest.Item]) -> None:
"""Deselect `slow` unless asked for.

Deselected rather than skipped, deliberately. A skip is a line of output per test claiming
something was considered and not done; a deselection is one number in the summary saying the
selection was smaller, which is what actually happened. It also means `-m slow` still works to
run only these, without `--runslow`, because an explicit marker expression is a request.
"""
if config.getoption("--runslow") or config.option.markexpr:
return
kept, dropped = [], []
for item in items:
(dropped if item.get_closest_marker("slow") else kept).append(item)
if dropped:
config.hook.pytest_deselected(items=dropped)
items[:] = kept


def pytest_configure(config: pytest.Config) -> None:
"""Start the clock the tripwire reads."""
setattr(config, _STARTED, time.perf_counter())


def _is_the_whole_suite(config: pytest.Config) -> bool:
"""Whether this run was the default selection rather than somebody's subset.

A subset is faster by construction, so timing one against a whole-suite ceiling would be
measuring nothing. `-k`, `-m` and a path argument are the three ways to ask for one.
"""
testpaths = list(config.getini("testpaths"))
return (
not config.option.keyword and not config.option.markexpr and list(config.args) == testpaths
)


def pytest_terminal_summary(terminalreporter: Any, exitstatus: int, config: pytest.Config) -> None:
"""Report the run's wall time, and refuse a step change in it.

Printed on every run rather than only when it fails, because a ceiling nobody sees the distance
to is one that gets crossed in a single commit.
"""
started = getattr(config, _STARTED, None)
if started is None: # pragma: no cover - configure always runs first
return
seconds = time.perf_counter() - started
if not _is_the_whole_suite(config):
return
terminalreporter.write_line(
f"suite wall time: {seconds:.1f}s (tripwire at {MAX_SUITE_SECONDS:.0f}s)"
)
if seconds > MAX_SUITE_SECONDS:
terminalreporter.write_line(
f"SUITE TOO SLOW: {seconds:.1f}s against a {MAX_SUITE_SECONDS:.0f}s ceiling. This is "
f"four times the number it protects, so it is a step change rather than drift: run "
f"with --durations=10 and look for something new. If the suite has genuinely grown, "
f"move the ceiling deliberately and say what grew.",
red=True,
)
terminalreporter._session.exitstatus = pytest.ExitCode.TESTS_FAILED # noqa: SLF001
if exitstatus == 0:
terminalreporter._session.testsfailed += 1 # noqa: SLF001
Loading