Skip to content

Commit 5483917

Browse files
jeremymanningclaude
andcommitted
Issue #130: Ignore test-generated artifacts; add session notes
Running the full suite while diagnosing the pre_push_check hang showed the benchmark tests in tests/real_world/ write performance_test_results/ into the repo root, which was not ignored. coverage_detailed_report.txt was untracked for the same reason. Notes record the measured before/after, the two things the handoff plan did not anticipate, and what was deliberately left open (#133, #110, #117). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012gTBDPK16HUZ3kHQ2QyjuU
1 parent 14222d0 commit 5483917

2 files changed

Lines changed: 115 additions & 0 deletions

File tree

.gitignore

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,11 @@ test-reports/
2626
.coverage
2727
.coverage.*
2828
coverage.json
29+
coverage_detailed_report.txt
2930
htmlcov/
3031
.tox/
32+
# Written by the benchmark tests in tests/real_world/ at run time.
33+
performance_test_results/
3134

3235
# Personal configuration files
3336
clustrix.yml
Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
# Session log: executing the #130 plan
2+
3+
**Date:** 2026-08-17 · **Branch:** `fix/130-pytest-config` · **Base:** `master` @ `a9393b7`
4+
**Plan followed:** `notes/handoff_130_pytest_config.md`
5+
6+
Every command below was run in a throwaway venv, since the repo has no usable
7+
one by default (#110):
8+
9+
```bash
10+
python3 -m venv /tmp/v130 && /tmp/v130/bin/pip install -e ".[dev]"
11+
```
12+
13+
`scripts/pre_push_check.py` shells out to bare `pytest`/`mypy`/`flake8` from
14+
`PATH`, so it must be run as
15+
`PATH="/tmp/v130/bin:$PATH" /tmp/v130/bin/python scripts/pre_push_check.py`.
16+
Running it with the venv interpreter alone is not enough and fails with
17+
`ModuleNotFoundError: paramiko`.
18+
19+
## What landed, in order
20+
21+
| Commit | Step | What |
22+
|-|-|-|
23+
| `9c18cb3` | 0.5 | Cleared 6 collection errors blocking Step 4 |
24+
| `176d87e` | 5 (first) | Regression tests, TDD red |
25+
| `9c85296` | 1 | Guard reads argv, not resolved testpaths |
26+
| `d1d6090` | 2 | Deleted pytest.ini; `testpaths = ["tests"]` |
27+
| `6d39bd4` | 3 | Registered all 7 markers; dropped `cleanup` |
28+
| `d93f933` | 4 | Enabled `--strict-markers` |
29+
| `43e7bf3` | 6 | MIGRATION.md + prime.md |
30+
| `14222d0` | 6 | pre_push_check runs CI's test command |
31+
32+
## Things the plan did not anticipate
33+
34+
**Step 0 baseline had 6 collection errors.** The plan said to record the
35+
number; it did not say the number came with errors attached. They had to be
36+
fixed before Step 4 could be verified at all, because Step 4's gate is
37+
"0 errors under `--strict-markers`".
38+
39+
- `tests/real_world/test_container_registry_comprehensive.py:571` had a
40+
backslash inside an f-string expression — a `SyntaxError` on every Python
41+
before 3.12, while `requires-python` says `>=3.8`. An AST parse over all of
42+
`tests/` and `clustrix/` confirmed it was the only one in the tree.
43+
- `numpy` and `pandas` were undeclared but imported at module scope by five
44+
modules. Added to the `[dev]` extra.
45+
46+
Result: 1214 collected + 6 errors → 1275 collected + 0 errors.
47+
48+
**The plan's own regression test needed correcting.** It proposed asserting
49+
`pytestconfig.inipath.name == "pyproject.toml"`, which is right, but the
50+
docstring I first wrote for the bare-pytest guard test overclaimed. Measured:
51+
52+
| guard reads | testpaths | bare pytest |
53+
|-|-|-|
54+
| `invocation_params.args` | `["tests"]` | ok |
55+
| `invocation_params.args` | `["tests/unit","tests/integration"]` | ok |
56+
| `config.args` | `["tests"]` | ok |
57+
| `config.args` | `["tests/unit","tests/integration"]` | **REFUSED** |
58+
59+
So the two fixes are each independently sufficient, and the test guards the
60+
*combination*. Both are kept as belt and braces, and the table is recorded in
61+
the test's docstring.
62+
63+
**`--strict-markers` needs care with `-o addopts=`.** It reaches pytest through
64+
`addopts`, so `test_strict_markers_is_active` skips when
65+
`config.option.override_ini` contains an `addopts=` entry. Verified against
66+
pytest 8.4.2: that attribute holds `['addopts=']`.
67+
68+
**`pre_push_check.py` ran a bare `pytest`.** Harmless only while no config was
69+
effective. Once `testpaths` went live it resolved to all 1670 tests including
70+
the 224 network-bound ones in `tests/real_world/`, and stopped terminating. It
71+
also left artifacts behind: it modified checked-in files under
72+
`tests/real_world/screenshots/` and created an untracked
73+
`performance_test_results/`. Now runs `pytest tests/unit/ -m "not real_world"`,
74+
matching `.github/workflows/tests.yml`; the generated paths are gitignored.
75+
76+
## Definition of done — measured
77+
78+
```
79+
configfile pyproject.toml
80+
shadowing config files in repo 0
81+
bare pytest 1670 collected, no abort
82+
pytest tests/integration/<file> refused (#109 holds)
83+
tests/ -m "not real_world" integration node IDs 0
84+
markers registered 7 / 7
85+
collection errors under --strict-markers 0
86+
tests/unit/ 75 passed
87+
fresh venv, [dev] only, pytest tests/ --co 1670 collected, 0 errors
88+
```
89+
90+
## Left open deliberately
91+
92+
**#133 (filed).** `scripts/pre_push_check.py` still cannot exit 0: flake8
93+
reports 92 findings on `master` (91 on this branch). CI runs the identical
94+
flake8 command with `--exit-zero`, so it has never been green anywhere. 75 are
95+
`E402` from deliberate `sys.path` setup. The rest are not stylistic — the
96+
notable one is `tests/integration/test_direct_gpu_detection.py:42`, which
97+
builds a remote Python program inside an f-string without escaping its braces,
98+
so `{torch.__version__}`, `{i}`, `{props.name}`, `{e}` are interpolated in the
99+
*local* scope and the module raises `NameError` before the subprocess starts.
100+
It survives because the #109 guard keeps `tests/integration/` out of CI.
101+
102+
Not folded into this PR: different concern, gated directory, and it would bury
103+
the #130 change.
104+
105+
**#110 corrected**, not closed. Its `addopts`/xdist premise quoted the epic
106+
branch, not `master`, and its `sklearn` collection-error claim is wrong (every
107+
`sklearn` import under `tests/` is inside a function body). Two of its
108+
acceptance criteria are now met. Comment:
109+
https://github.com/ContextLab/clustrix/issues/110#issuecomment-5317453905
110+
111+
**#117** untouched, as instructed — `tests/real_world/conftest.py:223` still
112+
calls `is_dartmouth_network()` at collection time.

0 commit comments

Comments
 (0)