Commit a565574
authored
Skip absent test dependencies, and stop unit runs deleting the integration cache (#27)
Running the full test suite against a Babel release that actually
publishes the DuckDB Parquet files produced **4 failures and 4 errors**,
and not one of them was a defect in the code under test. Chasing those
turned up a fifth problem that had been corrupting the runs themselves:
any `pytest` invocation deleted the integration cache, including the `-m
"not integration"` loop you run constantly while editing.
This PR makes the suite report absent dependencies honestly and stop
sabotaging itself, so that a red run means a real problem again. It is
**tests and documentation only** — no change to `src/`.
Closes #19.
## What's here
**A file being published is settled per file, not per session.** A
release can publish `Concord.parquet` without `Identifiers.parquet` —
the current one does exactly that, serving a 4.6 GB Concord with no
Identifiers beside it — so "does this release have the Parquet files?"
is not one question. Every DuckDB file now goes through
`_download_or_skip()`, which skips on the `MissingBabelFileError` the
downloader already raises for a 404 on a `duckdb/` path.
`shared_downloader` is left answering only what is genuinely
session-wide — is the server reachable — and probes the release root
rather than a named file, so it cannot be misread as a publication check
that forgot to check anything.
**A unit-test run no longer deletes the integration cache.**
`pytest_sessionfinish` removed `data/test` unconditionally, and that
path is fixed rather than per-run, so *any* session did it. A `pytest -m
"not integration"` in one terminal silently destroyed a multi-gigabyte
download in progress in another, and the integration run simply started
over — which reads as a downloader defect rather than an unrelated
`pytest` two windows away. The cleanup now runs only when the session
actually selected integration tests. The unit suite never creates or
reads that directory, so it has no business removing it.
**An unreachable NodeNorm skips instead of failing.** The `nodenorm`
fixture only constructed a client, so a connection error surfaced inside
each test body and failed it. CI runs `integration-test` on every push
to `main` and on a weekly cron, so a RENCI outage turned the build red
for reasons unrelated to any code change — the exact failure the
Babel-side skip was written to avoid. The fixture now probes `status`
once and skips on a `RequestException`.
**`NCIT:C55060` is removed from the test CURIEs.** It has no rows at all
in `Concord.parquet` as of Babel `2026jul22`, so the four tests
parametrized over it asserted `len(results) > 0` against an empty list.
NodeNorm still resolves it — as *"Hypertension, CTCAE"*, a
`biolink:PhenotypicFeature` with a clique of 2, from Babel `2025sep1` —
so this is not a typo or a retired identifier, and something changed
between those two releases. That question is #29, not this PR's: whether
Babel was right to drop the cross-references is a question about Babel's
data, and if a good one went for a bad reason it belongs upstream.
`valid_curies.txt` keeps a two-line pointer so the short list explains
itself.
**`Identifiers.parquet` is checked for being a Parquet file, not for
being large.** The assertion was `size > 2 GB`, a figure picked against
a release long since superseded. It would fail a legitimately smaller
future release while passing a 2.5 GB *truncated* download. It now
checks the `PAR1` marker at both ends, which tests what actually goes
wrong: a download cut short, or an error page saved under a `.parquet`
name.
## What it produces
Against a release that publishes the Parquet files, the full suite is
**299 passed, 3 skipped, 0 failures, 0 errors** (16m31s, dominated by
the 4.6 GB download). It was 296 passed / 4 failed / 4 errors / 28
skipped before. The 25 tests that turned from skips into passes are the
Parquet-dependent ones that had never actually run — they now exercise
real data, including the DuckDB query paths and the recursive expansion.
Against a release without the files, the suite still skips cleanly, as
documented.
Nine new unit tests cover the three guards, because this whole class of
bug is otherwise invisible: these code paths only execute during an
integration run, and CI's integration job skips, so nothing would notice
a guard being dropped. It is the same blind spot that hid ten stale
`get_curie_xref.cache_clear()` calls until a review found them by
reading rather than running. Each guard was confirmed non-vacuous by
removing it and watching the test fail.
Four things that cost real time are now in `CLAUDE.md`: that a release
can publish one DuckDB file without the other; that
`pytest_sessionfinish` deletes `data/test` so a full run re-pays the
download; that two concurrent *integration* runs still clash, with how
to detect, avoid and recover; and that `addopts` carries `-n auto`, so a
serial run needs `-n0` rather than `-p no:xdist`.
## What it deliberately does not do
- **Diagnose `NCIT:C55060`.** Tracked in #29, which covers both
outcomes: picking a replacement CURIE if Babel was right to drop it, or
taking it upstream if a genuinely good cross-reference went for a bad
reason.
- **Make two concurrent integration runs safe.** Unavoidable while
`data/test` is a shared fixed path. Documented instead, with the symptom
spelled out, because it presents as a downloader bug.
- **Cover an outage that begins mid-run.** Both probes are single
up-front checks. A service that dies between the probe and the call
still fails the test, which is correct — the probes exist for the case
that recurs.
- **Give CI a release that publishes the Parquet files.** Filed as #28,
to be decided alongside #18's sizing question, since `Concord.parquet`
alone is a 4.6 GB download per run.
- **Clean up DuckDB spill files.** Filed as #26.
## Nothing is blocking this merge
Everything outstanding is tracked in #18, #26, #28 and #29, and none of
it makes what ships here wrong. #29 is the one worth reading before
merging: it records that the test CURIE list is now two entries, both
`MONDO:`, so the integration tests exercise no other namespace until a
third is restored. That is reduced coverage, not a defect in this
change.
<details>
<summary><b>How this landed</b> — two things reached their final shape
by correction rather than design. Kept for anyone tracing why a line
looks the way it does; the durable conclusions are in the code comments
and CLAUDE.md above.</summary>
**The per-file skip arrived in two steps.** The `Identifiers.parquet`
skip was first added inline in the `downloaded_identifiers` fixture,
duplicating the `MissingBabelFileError` handling that
`shared_downloader` was doing with a HEAD probe. That left two
mechanisms for one rule, which prompted collapsing both onto
`_download_or_skip()` and reducing `shared_downloader` to a reachability
check — and then re-pointing that check at the release root, since a
probe naming `Concord.parquet` still looked like the publication check
it had stopped being.
**The cache deletion was misdiagnosed twice before the cause turned
up.** The symptom was a full run whose `data/test` shrank instead of
growing and that never finished. It was blamed first on the new "a
resume requires a validator" rule in the downloader — wrong, the server
sends both an ETag and a Last-Modified, so that rule never fired — and
then on two concurrent full suites, which was real but not the cause.
The actual culprit was the unit suite: every `pytest -m "not
integration"` run during the investigation deleted the download it was
investigating. That is why `pytest_sessionfinish` now carries an
explicit "do not remove this guard" comment naming the misdiagnosis, and
why CLAUDE.md describes the symptom rather than just the rule — the
failure mode is much easier to recognise than to reason out.
</details>5 files changed
Lines changed: 282 additions & 23 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
184 | 184 | | |
185 | 185 | | |
186 | 186 | | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
187 | 190 | | |
188 | 191 | | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
189 | 196 | | |
190 | 197 | | |
191 | 198 | | |
| |||
309 | 316 | | |
310 | 317 | | |
311 | 318 | | |
| 319 | + | |
| 320 | + | |
| 321 | + | |
| 322 | + | |
| 323 | + | |
| 324 | + | |
| 325 | + | |
| 326 | + | |
| 327 | + | |
| 328 | + | |
| 329 | + | |
| 330 | + | |
| 331 | + | |
| 332 | + | |
| 333 | + | |
| 334 | + | |
| 335 | + | |
| 336 | + | |
| 337 | + | |
| 338 | + | |
| 339 | + | |
| 340 | + | |
| 341 | + | |
| 342 | + | |
| 343 | + | |
| 344 | + | |
| 345 | + | |
| 346 | + | |
| 347 | + | |
| 348 | + | |
| 349 | + | |
| 350 | + | |
| 351 | + | |
| 352 | + | |
| 353 | + | |
312 | 354 | | |
313 | 355 | | |
314 | | - | |
| 356 | + | |
315 | 357 | | |
316 | 358 | | |
317 | 359 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
13 | 13 | | |
14 | 14 | | |
15 | 15 | | |
16 | | - | |
| 16 | + | |
17 | 17 | | |
18 | 18 | | |
19 | 19 | | |
| |||
38 | 38 | | |
39 | 39 | | |
40 | 40 | | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
41 | 46 | | |
42 | 47 | | |
43 | 48 | | |
| |||
50 | 55 | | |
51 | 56 | | |
52 | 57 | | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
53 | 66 | | |
54 | 67 | | |
55 | 68 | | |
| 69 | + | |
| 70 | + | |
56 | 71 | | |
57 | 72 | | |
58 | 73 | | |
| |||
68 | 83 | | |
69 | 84 | | |
70 | 85 | | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
71 | 104 | | |
72 | 105 | | |
73 | 106 | | |
74 | 107 | | |
75 | | - | |
76 | | - | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
77 | 118 | | |
78 | | - | |
79 | 119 | | |
80 | | - | |
| 120 | + | |
81 | 121 | | |
82 | | - | |
83 | | - | |
84 | | - | |
| 122 | + | |
85 | 123 | | |
86 | 124 | | |
87 | 125 | | |
| |||
92 | 130 | | |
93 | 131 | | |
94 | 132 | | |
95 | | - | |
96 | | - | |
97 | | - | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
98 | 136 | | |
99 | 137 | | |
100 | 138 | | |
101 | 139 | | |
102 | 140 | | |
103 | | - | |
104 | | - | |
105 | | - | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
106 | 144 | | |
107 | 145 | | |
108 | 146 | | |
| |||
119 | 157 | | |
120 | 158 | | |
121 | 159 | | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
122 | 163 | | |
123 | | - | |
124 | | - | |
125 | | - | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
126 | 167 | | |
127 | 168 | | |
128 | 169 | | |
129 | 170 | | |
130 | | - | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
131 | 184 | | |
132 | 185 | | |
133 | 186 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | 1 | | |
2 | 2 | | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
3 | 7 | | |
4 | 8 | | |
5 | | - | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
13 | 13 | | |
14 | 14 | | |
15 | 15 | | |
| 16 | + | |
16 | 17 | | |
17 | 18 | | |
18 | 19 | | |
| |||
22 | 23 | | |
23 | 24 | | |
24 | 25 | | |
| 26 | + | |
25 | 27 | | |
26 | 28 | | |
27 | 29 | | |
| |||
1279 | 1281 | | |
1280 | 1282 | | |
1281 | 1283 | | |
1282 | | - | |
| 1284 | + | |
| 1285 | + | |
| 1286 | + | |
| 1287 | + | |
| 1288 | + | |
| 1289 | + | |
| 1290 | + | |
| 1291 | + | |
1283 | 1292 | | |
1284 | | - | |
1285 | | - | |
| 1293 | + | |
| 1294 | + | |
| 1295 | + | |
| 1296 | + | |
| 1297 | + | |
| 1298 | + | |
| 1299 | + | |
| 1300 | + | |
| 1301 | + | |
| 1302 | + | |
| 1303 | + | |
| 1304 | + | |
| 1305 | + | |
| 1306 | + | |
| 1307 | + | |
| 1308 | + | |
| 1309 | + | |
| 1310 | + | |
| 1311 | + | |
| 1312 | + | |
| 1313 | + | |
| 1314 | + | |
| 1315 | + | |
| 1316 | + | |
| 1317 | + | |
| 1318 | + | |
| 1319 | + | |
| 1320 | + | |
| 1321 | + | |
| 1322 | + | |
| 1323 | + | |
| 1324 | + | |
| 1325 | + | |
| 1326 | + | |
| 1327 | + | |
| 1328 | + | |
| 1329 | + | |
| 1330 | + | |
| 1331 | + | |
| 1332 | + | |
| 1333 | + | |
| 1334 | + | |
| 1335 | + | |
| 1336 | + | |
| 1337 | + | |
| 1338 | + | |
| 1339 | + | |
| 1340 | + | |
| 1341 | + | |
| 1342 | + | |
| 1343 | + | |
| 1344 | + | |
| 1345 | + | |
| 1346 | + | |
| 1347 | + | |
| 1348 | + | |
| 1349 | + | |
| 1350 | + | |
| 1351 | + | |
| 1352 | + | |
| 1353 | + | |
| 1354 | + | |
| 1355 | + | |
| 1356 | + | |
| 1357 | + | |
| 1358 | + | |
| 1359 | + | |
| 1360 | + | |
| 1361 | + | |
| 1362 | + | |
| 1363 | + | |
| 1364 | + | |
| 1365 | + | |
| 1366 | + | |
| 1367 | + | |
| 1368 | + | |
| 1369 | + | |
| 1370 | + | |
| 1371 | + | |
| 1372 | + | |
| 1373 | + | |
| 1374 | + | |
| 1375 | + | |
| 1376 | + | |
| 1377 | + | |
| 1378 | + | |
| 1379 | + | |
| 1380 | + | |
| 1381 | + | |
| 1382 | + | |
| 1383 | + | |
| 1384 | + | |
| 1385 | + | |
| 1386 | + | |
| 1387 | + | |
| 1388 | + | |
| 1389 | + | |
| 1390 | + | |
| 1391 | + | |
| 1392 | + | |
| 1393 | + | |
| 1394 | + | |
| 1395 | + | |
| 1396 | + | |
| 1397 | + | |
| 1398 | + | |
| 1399 | + | |
| 1400 | + | |
| 1401 | + | |
| 1402 | + | |
| 1403 | + | |
| 1404 | + | |
0 commit comments