Skip to content

Commit 717d1ea

Browse files
fix(test): harden benchmark gate and tracemalloc fixtures (PR review)
1 parent d4dee52 commit 717d1ea

10 files changed

Lines changed: 139 additions & 56 deletions

Makefile

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,19 @@
1-
.PHONY: update-baselines check-benchmarks clean-benchmark-artifacts
1+
.PHONY: seed-baselines-local update-baselines check-benchmarks clean-benchmark-artifacts
22

3-
update-baselines:
4-
pytest tests/benchmarks/ --benchmark-only --benchmark-json=benchmarks/_raw.json -o addopts=
5-
python scripts/reduce_baselines.py benchmarks/_raw.json benchmarks/baselines.json
3+
# WARNING: captures timings on THIS machine. Production baselines must match ubuntu-latest CI.
4+
# Prefer downloading benchmark-results.json from a CI artifact, then:
5+
# python scripts/reduce_baselines.py benchmark-results.json benchmarks/baselines.json --slack 1.5
6+
seed-baselines-local:
7+
@echo "WARNING: seed-baselines-local uses this host's timings; CI gates on ubuntu-latest." >&2
8+
PYTHONPATH=. pytest tests/benchmarks/ --benchmark-only --benchmark-json=benchmarks/_raw.json -o addopts=
9+
PYTHONPATH=. python scripts/reduce_baselines.py benchmarks/_raw.json benchmarks/baselines.json
10+
11+
# Deprecated alias — kept for muscle memory; see seed-baselines-local warning above.
12+
update-baselines: seed-baselines-local
613

714
check-benchmarks:
8-
pytest tests/benchmarks/ --benchmark-only --benchmark-json=benchmark-results.json -o addopts=
9-
python scripts/check_benchmark_regression.py benchmark-results.json benchmarks/baselines.json
15+
PYTHONPATH=. pytest tests/benchmarks/ --benchmark-only --benchmark-json=benchmark-results.json -o addopts=
16+
PYTHONPATH=. python scripts/check_benchmark_regression.py benchmark-results.json benchmarks/baselines.json
1017

1118
clean-benchmark-artifacts:
1219
rm -f benchmarks/_raw.json benchmark-results.json

benchmarks/README.md

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -40,21 +40,31 @@ The `benchmarks` job on **ubuntu-latest** runs pytest-benchmark (`--benchmark-js
4040

4141
**Gated:** parse medium/large + large peak memory; export 10/50/100 session latency + ZIP peak memory.
4242

43-
**Not gated (informational only):** `test_parse_session_small`, `test_search_full_corpus` (sub-ms CI noise), and the `cache` group. Benchmarks without a baseline entry print a warning and do not fail the gate.
43+
**Not gated (informational only):** `test_parse_session_small`, `test_search_full_corpus` (sub-ms CI noise), and the `cache` group. These names are omitted from `baselines.json` when using `reduce_baselines.py`. Benchmarks without a baseline entry print a warning and do not fail the gate.
44+
45+
Missing gated benchmarks (renamed or removed tests still listed in `baselines.json`) fail the gate.
4446

4547
## Refresh baselines
4648

47-
After intentional performance work, capture on **ubuntu-latest** (same OS as the gated CI job):
49+
After intentional performance work, capture on **ubuntu-latest** (same OS as the gated CI job). Download `benchmark-results.json` from a CI artifact when possible:
50+
51+
```bash
52+
python scripts/reduce_baselines.py benchmark-results.json benchmarks/baselines.json --slack 1.5
53+
```
54+
55+
For a quick local snapshot only (may not match CI timings):
4856

4957
```bash
50-
make update-baselines
58+
make seed-baselines-local
5159
```
5260

61+
`make update-baselines` is a deprecated alias for `seed-baselines-local` and prints a warning. Do not commit baselines from macOS/Windows unless you accept cross-OS gate skew.
62+
5363
Or manually:
5464

5565
```bash
56-
pytest tests/benchmarks/ --benchmark-only --benchmark-json=benchmarks/_raw.json -o addopts=
57-
python scripts/reduce_baselines.py benchmarks/_raw.json benchmarks/baselines.json
66+
PYTHONPATH=. pytest tests/benchmarks/ --benchmark-only --benchmark-json=benchmarks/_raw.json -o addopts=
67+
PYTHONPATH=. python scripts/reduce_baselines.py benchmarks/_raw.json benchmarks/baselines.json
5868
```
5969

60-
Baselines must be captured on **ubuntu-latest** to match the gated CI runner. Cross-OS variance causes spurious failures. Download `benchmark-results.json` from a CI artifact to seed baselines if needed.
70+
Baselines must be captured on **ubuntu-latest** to match the gated CI runner. Cross-OS variance causes spurious failures.

benchmarks/baselines.json

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
{
2-
"_note": "Gated means from ubuntu-latest CI benchmark-results.json (PR memory-path benchmarks). Excluded from gate: test_parse_session_small, test_search_full_corpus (CI noise). Memory benchmarks use extra_info.peak_bytes (bytes); latency uses stats.mean (seconds).",
2+
"_note": "Gated means from ubuntu-latest CI benchmark-results.json (PR memory-path benchmarks). Values multiplied by 1.5× slack at seeding time. Excluded from gate (not in this file): test_parse_session_small, test_search_full_corpus (CI noise). Memory benchmarks use extra_info.peak_bytes (bytes); latency uses stats.mean (seconds).",
33
"updated": "2026-06-25T12:00:00Z",
44
"machine": "Linux",
55
"groups": {
@@ -15,9 +15,6 @@
1515
"test_bulk_export_zip_peak_memory[sessions-10]": 350546.0,
1616
"test_bulk_export_zip_peak_memory[sessions-50]": 503331.0,
1717
"test_bulk_export_zip_peak_memory[sessions-100]": 686874.0
18-
},
19-
"search": {
20-
"test_search_full_corpus": 0.001091
2118
}
2219
}
2320
}

scripts/check_benchmark_regression.py

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,8 +39,16 @@ def benchmark_entry_mean(entry: dict[str, object]) -> float:
3939
"""Return gated metric: peak_bytes from extra_info when present, else stats.mean."""
4040
if entry_uses_peak_bytes(entry):
4141
extra = entry["extra_info"]
42-
assert isinstance(extra, dict)
43-
return float(extra["peak_bytes"])
42+
if not isinstance(extra, dict):
43+
raise BenchmarkDataError(
44+
f"extra_info for {entry.get('name')!r} is not a dict"
45+
)
46+
try:
47+
return float(extra["peak_bytes"])
48+
except (KeyError, TypeError, ValueError) as exc:
49+
raise BenchmarkDataError(
50+
f"benchmark {entry.get('name')!r} missing 'stats.mean' or extra_info.peak_bytes"
51+
) from exc
4452
try:
4553
stats = entry["stats"]
4654
return float(stats["mean"]) # type: ignore[index]
@@ -75,6 +83,8 @@ def load_results(
7583
try:
7684
name = entry["name"]
7785
mean = benchmark_entry_mean(entry)
86+
except BenchmarkDataError:
87+
raise
7888
except (KeyError, TypeError, ValueError) as exc:
7989
raise BenchmarkDataError(
8090
f"{path} benchmarks[{index}] missing 'name' or measurable value"
@@ -132,12 +142,14 @@ def check_regression(
132142
baseline_means = load_baseline_means(baselines_path)
133143

134144
failures: list[str] = []
145+
missing: list[str] = []
135146
for name, base in baseline_means.items():
136147
if name in EXCLUDED_FROM_GATE:
137148
continue
138149
cur = flat.get(name)
139150
if cur is None:
140-
print(f"WARN: no current result for baseline {name!r}; skipping")
151+
print(f"FAIL: no current result for gated baseline {name!r}")
152+
missing.append(name)
141153
continue
142154
if base == 0:
143155
print(f"WARN: baseline for {name!r} is zero; skipping ratio check")
@@ -160,6 +172,9 @@ def check_regression(
160172

161173
if failures:
162174
print(f"\nREGRESSION: {len(failures)} benchmark(s) exceeded {threshold:.0%}")
175+
if missing:
176+
print(f"\nMISSING: {len(missing)} gated benchmark(s) absent from current results")
177+
if failures or missing:
163178
return 1
164179
return 0
165180

scripts/reduce_baselines.py

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,11 @@
88
from datetime import UTC, datetime
99
from pathlib import Path
1010

11-
try:
12-
from scripts.check_benchmark_regression import BenchmarkDataError, benchmark_entry_mean
13-
except ModuleNotFoundError:
14-
from check_benchmark_regression import BenchmarkDataError, benchmark_entry_mean
11+
from scripts.check_benchmark_regression import (
12+
EXCLUDED_FROM_GATE,
13+
BenchmarkDataError,
14+
benchmark_entry_mean,
15+
)
1516

1617
GATED_GROUPS = ("parse", "export", "search")
1718

@@ -51,21 +52,32 @@ def reduce_baselines(
5152
try:
5253
name = entry["name"]
5354
mean = benchmark_entry_mean(entry)
55+
except BenchmarkDataError:
56+
raise
5457
except (KeyError, TypeError, ValueError) as exc:
5558
raise BenchmarkDataError(
5659
f"{path} benchmarks[{index}] missing 'name' or measurable value"
5760
) from exc
61+
bench_name = str(name)
62+
if bench_name in EXCLUDED_FROM_GATE:
63+
continue
5864
group = entry.get("group")
5965
if group not in GATED_GROUPS:
6066
continue
61-
groups[group][str(name)] = mean * slack
67+
groups[group][bench_name] = mean * slack
68+
69+
# Drop empty groups (e.g. search when only excluded benchmarks ran).
70+
groups = {name: values for name, values in groups.items() if values}
6271

72+
slack_note = f" Values multiplied by {slack}× slack at generation time." if slack != 1.0 else ""
6373
machine_info = raw.get("machine_info")
6474
machine = machine_info.get("system") if isinstance(machine_info, dict) else None
6575
output: dict[str, object] = {
6676
"_note": (
67-
"Gated means from ubuntu-latest CI benchmark-results.json. "
68-
"Excluded from gate: test_parse_session_small, test_search_full_corpus (CI noise). "
77+
"Gated means from ubuntu-latest CI benchmark-results.json."
78+
f"{slack_note} "
79+
"Excluded from gate (not written here): test_parse_session_small, "
80+
"test_search_full_corpus (CI noise). "
6981
"Memory benchmarks use extra_info.peak_bytes (bytes); "
7082
"latency uses stats.mean (seconds)."
7183
),

tests/benchmarks/conftest.py

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,28 +23,33 @@ class TracemallocPeak:
2323
"""Measure peak Python heap bytes for one callable invocation."""
2424

2525
def measure(self, func: Callable[..., T], /, *args: Any, **kwargs: Any) -> tuple[T, int]:
26+
was_tracing = tracemalloc.is_tracing()
2627
tracemalloc.start()
2728
tracemalloc.clear_traces()
2829
try:
2930
result = func(*args, **kwargs)
3031
_, peak = tracemalloc.get_traced_memory()
3132
return result, peak
3233
finally:
33-
tracemalloc.stop()
34+
if not was_tracing:
35+
tracemalloc.stop()
3436

3537

3638
@pytest.fixture
3739
def tracemalloc_peak() -> TracemallocPeak:
3840
return TracemallocPeak()
3941

4042

41-
def write_jsonl(path: Path, line_count: int) -> Path:
43+
def write_jsonl(path: Path, line_count: int, *, first_timestamp: str | None = None) -> Path:
4244
"""Write a JSONL session file with *line_count* rows derived from the template fixture."""
4345
template = json.loads(TEMPLATE_LINE)
4446
with path.open("w", encoding="utf-8") as f:
4547
for i in range(line_count):
4648
entry = deepcopy(template)
47-
entry["timestamp"] = f"2026-06-12T10:{i % 60:02d}:00Z"
49+
if i == 0 and first_timestamp is not None:
50+
entry["timestamp"] = first_timestamp
51+
else:
52+
entry["timestamp"] = f"2026-06-12T10:{i % 60:02d}:00Z"
4853
if i % 3 == 1:
4954
msg = entry.setdefault("message", {})
5055
if isinstance(msg, dict) and "content" in msg:
@@ -96,7 +101,9 @@ def export_corpus(tmp_path: Path, request: pytest.FixtureRequest) -> Path:
96101
project = tmp_path / "bench-project"
97102
project.mkdir()
98103
for i in range(count):
99-
write_jsonl(project / f"session_{i:04d}.jsonl", 20)
104+
# Unique first_timestamp per session so export filenames do not collide in ZIP benches.
105+
first_ts = f"2026-06-12T{i % 24:02d}:{i % 60:02d}:00Z"
106+
write_jsonl(project / f"session_{i:04d}.jsonl", 20, first_timestamp=first_ts)
100107
return project
101108

102109

tests/benchmarks/test_export_bench.py

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88

99
import pytest
1010

11-
from tests.benchmarks.conftest import TracemallocPeak
1211
from utils.export_engine import BulkExportResult, NoopSink, ZipSink, run_bulk_export
1312

1413

@@ -56,10 +55,11 @@ def _run() -> object:
5655
def test_bulk_export_zip_peak_memory(
5756
benchmark,
5857
export_corpus: Path,
59-
tracemalloc_peak: TracemallocPeak,
58+
tracemalloc_peak,
6059
) -> None:
6160
projects = _bench_projects(export_corpus)
6261
peaks: list[int] = []
62+
results: list[BulkExportResult] = []
6363

6464
def _run() -> None:
6565
def _export() -> BulkExportResult:
@@ -78,9 +78,10 @@ def _export() -> BulkExportResult:
7878
)
7979

8080
result, peak = tracemalloc_peak.measure(_export)
81-
assert result.exported_session_count > 0
81+
results.append(result)
8282
peaks.append(peak)
8383

8484
benchmark(_run)
85+
assert results and results[-1].exported_session_count > 0
8586
assert peaks, "benchmark produced no peak memory samples"
8687
benchmark.extra_info["peak_bytes"] = int(sum(peaks) / len(peaks))

tests/benchmarks/test_parse_memory.py

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2,40 +2,34 @@
22

33
from __future__ import annotations
44

5-
import tracemalloc
65
from pathlib import Path
76

87
import pytest
98

10-
from tests.benchmarks.conftest import TracemallocPeak
119
from utils.jsonl_parser import parse_session
1210

1311

14-
def test_large_parse_peak_memory_under_ceiling(parse_large_file: Path) -> None:
12+
def test_large_parse_peak_memory_under_ceiling(
13+
parse_large_file: Path,
14+
tracemalloc_peak,
15+
) -> None:
1516
path = parse_large_file
1617
file_bytes = path.stat().st_size
1718
# Issue #7 ceiling: Python heap peak (tracemalloc) vs on-disk JSONL size. Parsed
1819
# dict/str objects often exceed raw bytes; 10x is a generous v1 guard — relax with
1920
# a comment here if the parser legitimately grows.
2021
ceiling = file_bytes * 10
2122

22-
tracemalloc.start()
23-
tracemalloc.clear_traces()
24-
try:
25-
result = parse_session(str(path))
26-
assert len(result["messages"]) > 0, "parse_session returned no messages"
27-
_, peak = tracemalloc.get_traced_memory()
28-
finally:
29-
tracemalloc.stop()
30-
23+
result, peak = tracemalloc_peak.measure(parse_session, str(path))
24+
assert len(result["messages"]) > 0, "parse_session returned no messages"
3125
assert peak < ceiling, f"peak {peak} bytes exceeds 10x file size {file_bytes}"
3226

3327

3428
@pytest.mark.benchmark(group="parse")
3529
def test_parse_large_peak_memory(
3630
benchmark,
3731
parse_large_file: Path,
38-
tracemalloc_peak: TracemallocPeak,
32+
tracemalloc_peak,
3933
) -> None:
4034
path = str(parse_large_file)
4135
peaks: list[int] = []

tests/test_check_benchmark_regression.py

Lines changed: 32 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -39,12 +39,12 @@ def test_missing_baseline_warns_without_failing(
3939
results,
4040
[
4141
{"name": "test_new_bench", "stats": {"mean": 0.01}},
42-
{"name": "test_parse_session_small", "stats": {"mean": 0.0001}},
42+
{"name": GATED_BENCH, "stats": {"mean": 0.002}},
4343
],
4444
)
4545
_write_baselines(
4646
baselines,
47-
{"parse": {"test_parse_session_small": 0.0001}},
47+
{"parse": {GATED_BENCH: 0.002}},
4848
)
4949

5050
assert check_regression(results, baselines) == 0
@@ -152,9 +152,7 @@ def test_excluded_benchmark_in_baselines_is_not_gated(
152152
assert "REGRESSION" not in capsys.readouterr().out
153153

154154

155-
def test_missing_current_result_warns_without_failing(
156-
tmp_path, capsys: pytest.CaptureFixture[str]
157-
) -> None:
155+
def test_missing_current_result_fails(tmp_path, capsys: pytest.CaptureFixture[str]) -> None:
158156
results = tmp_path / "results.json"
159157
baselines = tmp_path / "baselines.json"
160158
_write_results(results, [])
@@ -163,8 +161,10 @@ def test_missing_current_result_warns_without_failing(
163161
{"parse": {GATED_BENCH: 0.002}},
164162
)
165163

166-
assert check_regression(results, baselines) == 0
167-
assert "no current result for baseline" in capsys.readouterr().out
164+
assert check_regression(results, baselines) == 1
165+
out = capsys.readouterr().out
166+
assert "MISSING" in out
167+
assert "no current result for gated baseline" in out
168168

169169

170170
def test_main_reports_benchmark_data_error(tmp_path, capsys: pytest.CaptureFixture[str]) -> None:
@@ -199,11 +199,11 @@ def test_metric_is_bytes_uses_extra_info_without_name_hint() -> None:
199199
from scripts.check_benchmark_regression import metric_is_bytes
200200

201201
entry = {
202-
"name": "test_custom_memory",
202+
"name": "test_export_latency",
203203
"stats": {"mean": 0.05},
204204
"extra_info": {"peak_bytes": 1_000_000},
205205
}
206-
assert metric_is_bytes("test_custom_memory", entry)
206+
assert metric_is_bytes("test_export_latency", entry)
207207

208208

209209
def test_memory_metric_regression_uses_bytes(tmp_path, capsys: pytest.CaptureFixture[str]) -> None:
@@ -230,6 +230,29 @@ def test_memory_metric_regression_uses_bytes(tmp_path, capsys: pytest.CaptureFix
230230
assert "REGRESSION" in out
231231

232232

233+
def test_benchmark_entry_mean_rejects_non_dict_extra_info() -> None:
234+
from scripts.check_benchmark_regression import benchmark_entry_mean
235+
236+
with pytest.raises(BenchmarkDataError, match="extra_info"):
237+
benchmark_entry_mean(
238+
{
239+
"name": "test_parse_large_peak_memory",
240+
"extra_info": "not-a-dict",
241+
}
242+
)
243+
244+
245+
def test_load_results_preserves_benchmark_data_error_message(tmp_path) -> None:
246+
path = tmp_path / "results.json"
247+
_write_results(
248+
path,
249+
[{"name": "test_parse_large_peak_memory", "extra_info": {"peak_bytes": "bad"}}],
250+
)
251+
252+
with pytest.raises(BenchmarkDataError, match="extra_info.peak_bytes"):
253+
load_results(path)
254+
255+
233256
def test_duplicate_baseline_name_raises(tmp_path) -> None:
234257
baselines = tmp_path / "baselines.json"
235258
_write_baselines(

0 commit comments

Comments
 (0)