Skip to content

Commit ab13a9e

Browse files
gauravgaurav
authored
Run BabelTest assertions embedded in GitHub issues as pytest tests (#103)
Test cases for Babel currently live in a Google Sheet, which is fine for bulk regression data and poor for anything tied to a specific bug: the sheet has no idea which issue a row came from, so nothing tells us when a fixed issue regresses or when an open one has quietly started working. This PR lets a test case live in the GitHub issue that motivated it, and runs those cases as pytest tests. An issue body carries assertions in either of two syntaxes — a wiki-style `{{BabelTest|Resolves|CHEBI:15365}}` marker, or a fenced YAML block whose top-level key is `babel_tests:` — and the harness turns each issue into one pytest item, with each assertion a subtest. Open issues are expected to fail, so an open issue whose assertions all pass is reported as a strict XPASS: the tool is telling you the issue looks closeable. Closed issues are expected to pass, so one that starts failing is telling you to reopen it. **Stack 3 of 4** splitting #67. #101 and #102 have merged, so this now bases on `main`; #104 is the remaining piece. This leaves #67 open, which stays open for #104. ## What's here **Parsing — `src/babel_validation/sources/github/`.** `GitHubIssuesTestCases` finds assertion blocks in issue bodies and resolves each to a handler from the assertions framework that landed in #102. Issue discovery uses the GitHub search API rather than paginating every issue, and reads `body` and `html_url` straight off the search result, so scanning the five configured repositories costs two search requests per repo and no core requests at all. **Harness — `tests/github_issues/`.** One pytest item per issue, parametrized by issue ID, across the repositories listed under `Repositories` in `targets.ini`'s `[DEFAULT]` section. `--issue` targets a specific issue by `org/repo#N`, `repo#N` or `N`. Issue IDs and hydrated issues are cached and shared across `pytest-xdist` workers behind a `FileLock`. **Input validation.** Issue bodies are untrusted: anyone with a GitHub account can write one, nothing reviews it, and we parse it and turn it into live NodeNorm and NameRes calls. That is tolerable while a human watches the run and can hit Ctrl-C; it is not tolerable for the unattended daily runs this is meant to enable. Two of the gaps were reproducible denial of service rather than theory: - The pattern that finds a `babel_tests` block ended with `\s+.*?\s+` before the closing fence. Those three nested backtracking quantifiers made matching cubic on a body that opens a block and never closes the fence — 6.8s at 4KB, 53s at 8KB, and hours at GitHub's 65536-character body limit. It runs during collection, which `pytest-timeout` does not cover, so a single such issue hung the whole run before any test started. Anchoring on the newline that follows `babel_tests:` in a real fenced block removes the ambiguity: 0.0008s at 65536 characters. - `yaml.safe_load` blocks code execution but still resolves aliases, and PyYAML shares the aliased nodes rather than copying them, so the load looks cheap and the cost lands on whatever formats the result afterwards. A 337-byte body of chained anchors reached 25MB in an error message. Aliases are refused outright, which covers merge keys too. On top of those: caps on body length, assertions, param sets and parameters per issue; per-parameter checks for empty, over-long and non-printable values; duplicate YAML keys rejected, because YAML keeps the last silently and the block a reviewer reads would not be the block that runs; issue text `repr()`'d into log lines so an ANSI escape or bidi override cannot reach an operator's terminal; and `--issue` resolving only within the configured repositories. Structural caps fail the whole issue — the fix is to split it across several issues — while a bad parameter fails only its own param set, so the rest of the issue still runs. The caps sit far above anything the configured repositories currently contain, so they are a no-op for real issues today, and they are documented in the generated `assertions/README.md` where issue authors will meet them. **Caches moved out of the shared temp directory.** Both the issue-ID cache and the Google Sheet CSV cache used fixed names in the world-writable temp directory. The issue cache holds the IDs a later run fetches and executes assertions from, so being able to write it was close to being able to choose what the run tests. They now live in `~/.cache/babel-validation`, created `0700`, overridable with `BABEL_VALIDATION_CACHE_DIR` — and failing to create that directory names the override, since a read-only home on a locked-down runner otherwise raises a `PermissionError` that gives no hint an escape hatch exists. The cache sweep in `pytest_configure` also stopped deleting the Google Sheet `.lock` files. 48b1c44 had already removed that for the GitHub issue lock — a concurrent pytest holding the lock keeps its now-unlinked inode while this run creates a fresh one, so two processes end up inside "the" lock — and the Google Sheet path had the same shape. `unlink_if_exists()` now also refuses any path outside the cache directory, because it deletes whatever it is handed and runs before anything else in the session. **Test layout.** The offline tests live in `tests/github_issues/unit/` — `test_syntax.py` (what the two syntaxes mean), `test_discovery.py` (finding issues, identifying them, resolving an ID) and `test_untrusted_input.py` (the guards on body content). A subdirectory rather than three siblings because of the fixture: `tests/github_issues/conftest.py` defines a session-scoped `github_issues_test_cases` that needs a real `GITHUB_TOKEN`, and these tests want a dummy-token parser instead. A `conftest.py` in the subdirectory scopes that override to exactly the files that want it, where putting it in the parent would replace the real fixture for the live `test_github_issues.py` as well. **Documentation.** `CLAUDE.md` gains an `Untrusted Input` section: which inputs are hostile and which (`targets.ini`) are trusted config, and each failure mode above written as the shape to look for rather than as the fix that was applied — so the next parser added here starts from them. The generated `assertions/README.md` carries the caps, where issue authors meet them. And it now says in as many words that a red `pytest tests/github_issues` is the tool working, not a defect to be fixed by editing the assertions, because the obvious reading of eighteen red tests is otherwise the wrong one. And it warns against writing a complete `{{BabelTest|...}}` marker into an issue: this repository is itself in the scanned `Repositories` list, so an issue that merely *describes* an assertion gets collected and runs it. ## What a run produces `pytest -m unit` is fully offline and needs no token: **97 passed**, in about a second. This is what CI runs, and it is green. `pytest tests/github_issues --target dev` currently reports **18 failing issues**, and that is the tool working rather than a defect in it: - **10 open issues XPASS** — every assertion now passes, so they look closeable. - **7 closed issues have failing assertions** — #406, #552, #584, #711, #714, #723, #906 — so they look like they should be reopened. - **1 issue uses an assertion type we have not written yet**: `ShouldNotHaveSynonym`, in NCATSTranslator/Babel#744. Unknown assertion names fail loudly by design rather than being skipped, so this stays a hard failure until #110 lands. It is unrelated to any NodeNorm behaviour. ## What it deliberately does not do - **No new CLI options for the limits.** They are module constants. `pytest --timeout=N` already exists and is the runtime knob for a slow issue. - **No guards around the NodeNorm/NameRes responses.** Those URLs come from `targets.ini`, which is trusted config, not from issue bodies. - **CI runs `-m unit` only**, so nothing here exercises the live GitHub path on a PR. Enabling it needs `issues: read` and a token; the recipe is in a comment in `tests.yaml`, and #114 covers building a tier of tests that need a token but not a full crawl. Worth knowing that without a token the issue tests **skip rather than fail**, so a run can go green having tested nothing. ## Follow-on work Nothing is blocking this merge. Two pieces are tracked separately: - #110 — implement the `ShouldNotHaveSynonym` assertion type, which is the one live failure above that is about this harness rather than about NodeNorm. - #114 — a tier of GitHub API tests that need a token but not a full issue crawl, so CI can cover the integration itself. - #115 — the per-issue size caps are checked after every `GitHubIssueTest` has been built, so an oversized body still constructs a few thousand objects before being rejected. Bounded by GitHub's own body limit and off the network path, so it matters only if the caps are ever tightened much further. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
2 parents 8766968 + d51ca3e commit ab13a9e

30 files changed

Lines changed: 2186 additions & 90 deletions

‎.github/workflows/tests.yaml‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
name: Tests
2+
3+
on:
4+
pull_request:
5+
6+
# Only checkout needs the token. Declaring this at all matters: every scope not
7+
# listed here is set to none, so the job no longer inherits whatever the org's
8+
# default-permissions setting happens to be. The unit tests themselves need no
9+
# GitHub API access — `-m unit` deselects the issue tests before they look for a
10+
# token — so anything beyond contents:read would be unused.
11+
permissions:
12+
contents: read
13+
14+
jobs:
15+
unit-tests:
16+
runs-on: ubuntu-latest
17+
steps:
18+
- uses: actions/checkout@v4
19+
20+
- uses: astral-sh/setup-uv@v6
21+
22+
# To run the GitHub issue tests here too — drop the `-m unit` filter, or
23+
# add a second job running `uv run pytest tests/github_issues` — put back:
24+
#
25+
# env:
26+
# GITHUB_TOKEN: ${{ github.token }}
27+
#
28+
# and add `issues: read` to the permissions block above. Do not skip the
29+
# env: block — without a token the issue tests skip at the module level
30+
# rather than failing, so CI would go green having tested nothing.
31+
#
32+
# The token covers this repository only: the other repos in targets.ini's
33+
# Repositories list are readable because they are public, not because the
34+
# token is scoped to them, so adding a private one would mean a PAT in a
35+
# secret instead.
36+
- name: Run unit tests
37+
run: uv run pytest -m unit -v

‎.gitignore‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,6 @@
1+
# Ignore the root .env file.
2+
/.env
3+
14
# Ignore all data files.
25
data/
36

‎CLAUDE.md‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,10 @@ pytest tests/nodenorm/test_nodenorm_from_gsheet.py -k "row=42" # Run a specific
2929
black tests/ # Format Python test code
3030
```
3131

32+
Note that the repository is *not* currently black-clean — `black --check tests/ src/` reports
33+
~30 files it would reformat. Running `black` across the tree would bury a real change in
34+
unrelated churn, so format only the files you touch, or match the surrounding style.
35+
3236
### Vue Website (website-vue3-vite/)
3337

3438
```bash
@@ -76,6 +80,56 @@ The core of this project. Tests validate NodeNorm and NameRes services across mu
7680
- **`website/`** — Newer Astro-based site deployed to GitHub Pages with prefix comparator and autocomplete tools
7781
- **`scala-validation/`** — Legacy, unmaintained
7882

83+
## Untrusted Input
84+
85+
Most of what this project reads was written by someone else and reviewed by nobody. Treat it as
86+
hostile, not merely as data that might be malformed:
87+
88+
- **GitHub issue bodies** (`src/babel_validation/sources/github/`) — anyone with a GitHub account
89+
can write one, and we parse it into live NodeNorm/NameRes calls.
90+
- **The Google Sheet** (`src/babel_validation/sources/google_sheets/`) — anyone with edit access.
91+
- **Anything off the network**, including a service's response.
92+
93+
`tests/targets.ini` is the exception: its URLs and its `Repositories` list are checked-in config,
94+
so they are trusted, and guards belong on what the issue supplies rather than on them.
95+
96+
Every failure mode below was real, and found in this code. These are the shapes to look for.
97+
98+
**A regex over untrusted text can hang the process.** `\s+ .*? \s+` before a literal is three
99+
nested backtracking quantifiers, and matched in cubic time: 53s on an 8KB body, hours at GitHub's
100+
65536-character limit. Avoid adjacent quantifiers that can match the same characters — anchor on
101+
something disjoint, such as a newline or a literal. Note that `pytest --timeout` only wraps test
102+
execution, so anything running at **collection** time has no timeout at all.
103+
104+
**`yaml.safe_load` is not a safe parser, only a non-executing one.** It still resolves anchors,
105+
aliases and merge keys, and PyYAML shares the aliased nodes rather than copying them — so the load
106+
looks cheap and the blow-up lands on whatever formats the result afterwards. 337 bytes became a
107+
25MB error message. Use `_NoAliasSafeLoader` in `sources/github/github_issues_test_cases.py`.
108+
109+
**Format untrusted text with `%r` / `!r`, never `%s` / `{}`.** `repr()` escapes exactly the
110+
characters `str.isprintable()` rejects — ANSI escapes, C0/C1 controls, bidi overrides, zero-width
111+
characters — so it is the whole defence for anything reaching a terminal, a log line or a pytest
112+
ID. Truncate before `repr()`ing anything that might be large: the message is kept in pytest's
113+
report.
114+
115+
**A guard that runs after the value was logged is too late.** Validate at the one choke point that
116+
sees every value before anything formats it. For assertion params that is
117+
`AssertionHandler._rejection()`, because the per-handler CURIE check skips whatever
118+
`curie_params()` excludes and is turned off entirely by `VALIDATE_CURIES = False`.
119+
120+
**Never let outside text choose what we fetch.** `get_issues_by_ids()` takes an ID that decides
121+
which repository we read assertions from, and its `[^#]+` group admits slashes — so check the
122+
allowlist *before* the call, or the value reaches the GitHub API as a URL path.
123+
124+
**Fail loudly; skipping looks like passing.** Reject a bad issue rather than silently running a
125+
truncated part of it. The same goes for missing credentials: the GitHub issue tests *skip* without
126+
a token, so a green run may have tested nothing.
127+
128+
**Caches belong in `cache_dir()`** (`src/babel_validation/core/__init__.py`), a 0700 directory
129+
under the user's home — never a fixed name in the shared temp directory. On a CI runner or a
130+
shared machine anyone can pre-create such a file, and the issue cache decides what a later run
131+
fetches and executes.
132+
79133
## Key Dependencies
80134

81135
- Python >=3.11, pytest, requests, deepdiff, openapi-spec-validator, black
@@ -88,4 +142,27 @@ When writing new tests:
88142
- For Google Sheet-based tests, parametrize with `gsheet.test_rows()` and use the `test_category` fixture for category filtering
89143
- Use `pytest.mark.xfail(strict=True)` for known failures (strict=True means unexpected passes also fail)
90144
- Hand-written per-issue regression tests go in `tests/nodenorm/by_issue/`
145+
- **`pytest tests/github_issues` is expected to be red, and that is the tool working.** An open
146+
issue whose assertions all pass is a strict XPASS, meaning it looks closeable; a closed issue
147+
with failing assertions means it looks like it should be reopened. Those results are findings
148+
about Babel, not defects in this repo — do not "fix" them by editing the assertions. Only a
149+
hard ERROR (an unknown assertion name, a rejected issue body) is a problem here.
150+
- When checking that a new test really fails without its fix, **clear `__pycache__` between runs**.
151+
A same-length edit (`%r` for `%s`, say) leaves the source's size unchanged, and if the mtime lands
152+
in the same granularity the `.pyc` is not invalidated — so the mutation appears to pass a test
153+
that never saw it. Also avoid asserting on `caplog.text` for anything about control characters:
154+
it does not carry them through, so such a test passes whatever the code does. Read
155+
`caplog.records` and `getMessage()` instead.
156+
- **Never put a complete `{{BabelTest|...}}` marker or a fenced `babel_tests:` block into a GitHub
157+
issue you file** — not even in prose explaining the syntax. `TranslatorSRI/babel-validation` is
158+
itself in the scanned `Repositories` list, so the harness collects the marker and runs it: an
159+
issue that merely *describes* an assertion becomes a test of that assertion. Because a new issue
160+
is open, an assertion that passes then reports as a strict XPASS failure. This is not
161+
hypothetical — issue #115 was filed with a marker in it and immediately failed the live suite.
162+
Quote a partial marker instead, dropping the closing `}}`, which the pattern needs to match. A
163+
one-line ```` ```yaml babel_tests: ``` ```` in prose is already safe: the block pattern requires a
164+
newline after the key.
165+
- To check behaviour when no GitHub token is available, run with `GITHUB_TOKEN=` (set but
166+
empty) rather than unsetting it: `dotenv.load_dotenv()` will not override a key already
167+
present in `os.environ`, so this defeats the token in the developer's `.env` file
91168
- Import shared classes from `src.babel_validation.*` (e.g. `from src.babel_validation.services.nodenorm import CachedNodeNorm`)

‎README.md‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,66 @@ ssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssss
6161
======================================================= 41 passed, 1965 skipped, 4 xfailed in 10.11s ========================================================
6262
```
6363

64+
### GitHub issue tests
65+
66+
Assertions can also be embedded directly in GitHub issue bodies — see
67+
[`src/babel_validation/assertions/README.md`](./src/babel_validation/assertions/README.md)
68+
for the syntax and the available assertion types. The repositories scanned for them are
69+
listed under `Repositories` in the `[DEFAULT]` section of
70+
[`tests/targets.ini`](./tests/targets.ini).
71+
72+
Issue bodies are untrusted input, so the harness caps what one issue may contain — 100
73+
assertions, 1,000 params lists, 1,000 parameters, 1,000 characters per parameter — and
74+
rejects YAML anchors, aliases and duplicate keys. An issue over a cap fails loudly rather
75+
than running part of itself; split it into several issues. The caps are listed in
76+
[`src/babel_validation/assertions/README.md`](./src/babel_validation/assertions/README.md).
77+
`--issue` resolves only within the configured `Repositories`, so a run can never be pointed
78+
at assertions from somewhere else.
79+
80+
Beware when *discussing* the syntax in an issue: a complete `{{BabelTest|...}}` marker is
81+
picked up wherever it appears, backticks included, and an unrecognised assertion name fails
82+
the run rather than being ignored. Quote a partial marker instead — the pattern needs the
83+
closing `}}` to match.
84+
85+
```shell
86+
$ pytest tests/github_issues --target dev # every issue carrying assertions
87+
$ pytest tests/github_issues --target dev --issue 'org/repo#42' # just one (also 'repo#42' or '42')
88+
```
89+
90+
These tests need a `GITHUB_TOKEN`, in the environment or in a `.env` file. Without one they
91+
**skip rather than fail**, so a run can look green having tested nothing. Generate a
92+
[personal access token](https://docs.github.com/en/authentication/keeping-your-account-and-data-secure/managing-your-personal-access-tokens);
93+
inside a GitHub Action, use the
94+
[automatic `GITHUB_TOKEN`](https://docs.github.com/en/actions/security-for-github-actions/security-guides/automatic-token-authentication)
95+
instead.
96+
97+
The token is not needed for authentication as such — every repository we scan is public, and
98+
both the single-issue and search endpoints answer unauthenticated requests. It is needed for
99+
the [rate limits](https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api):
100+
101+
| | Unauthenticated | With a token |
102+
| --- | --- | --- |
103+
| Core | 60 / hour, **per IP** | 5,000 / hour |
104+
| [Search](https://docs.github.com/en/rest/search/search) | 10 / minute | 30 / minute |
105+
106+
Discovery is search-bound, not core-bound: two searches per configured repository (one per
107+
trigger keyword, plus a request per extra page of results), and then no core request at all,
108+
because a search result already carries the issue `body` and `html_url` the harness needs.
109+
Scanning the five configured repositories currently finds 96 issues for zero core requests.
110+
111+
Core requests are spent re-hydrating issues one at a time, which happens whenever the cached
112+
ID list is reused instead of the search being repeated — notably in every `pytest-xdist`
113+
worker after the first. That path costs one request per issue per worker, so an
114+
unauthenticated run would exhaust the 60/hour core budget well before finishing.
115+
116+
`GET /rate_limit` reports what is left without itself counting against the limit
117+
([docs](https://docs.github.com/en/rest/rate-limit/rate-limit)). Note that the search window
118+
resets every 60 seconds, so its counter is often back at zero by the time you look:
119+
120+
```shell
121+
$ curl -s -H "Authorization: Bearer $GITHUB_TOKEN" https://api.github.com/rate_limit
122+
```
123+
64124
## Log Analysis
65125

66126
The Jupyter Notebook in `log-analysis/` contains some basic analysis of the

‎pyproject.toml‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,17 @@ readme = "README.md"
77
requires-python = ">=3.11"
88
dependencies = [
99
"black>=25.9.0",
10+
"pyyaml>=6.0",
1011
"requests>=2.32.5",
1112
"filelock",
1213
"deepdiff>=8.6.1",
14+
"python-dotenv>=0.9.9",
1315
"openapi-spec-validator>=0.7.2",
14-
"pytest>=8.4.2",
16+
"pygithub>=2.8.1",
17+
"pytest>=9.0.2",
1518
"pytest-timeout>=2.4.0",
19+
"pytest-xdist[psutil]",
20+
"pytest-subtests",
1621
]
1722

1823
[project.urls]

‎src/babel_validation/assertions/README.md‎

Lines changed: 37 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,29 @@ The meaning of each element in a params list depends on the assertion type (see
3939
For most assertions the elements are CURIEs; for `HasLabel` the second element is a
4040
label string; for `ResolvesWithType` the first element is a Biolink type.
4141

42+
## Limits
43+
44+
Issue bodies are untrusted input — anyone can write one, and nothing reviews it before the
45+
harness parses it and turns it into live NodeNorm and NameRes calls. These caps bound what
46+
one issue can cost. They sit far above anything a real issue contains:
47+
48+
| Limit | Value |
49+
| --- | --- |
50+
| Assertions per issue | 100 |
51+
| Params lists per issue | 1,000 |
52+
| Parameters per issue | 1,000 |
53+
| Characters per parameter | 1,000 |
54+
55+
Exceeding one of the first three fails the whole issue rather than running part of it — split
56+
the assertions across several issues. An individual parameter that is too long, empty, or
57+
contains non-printable characters fails only its own params list; the rest of the issue still
58+
runs.
59+
60+
YAML anchors and aliases (`&name` / `*name`, including merge keys) are rejected: they let a few
61+
hundred bytes expand into megabytes. Duplicate keys in a `babel_tests` block are rejected too,
62+
since YAML would silently keep only the last one — and then the block a reviewer reads would
63+
not be the one that runs.
64+
4265
---
4366

4467
## NodeNorm Assertions
@@ -248,15 +271,21 @@ babel_tests:
248271
These are rendered into this file, so write them for someone reading this README
249272
rather than for someone reading the class.
250273

251-
3. Implement `test_params_list()` (or both `test_with_*` methods for `AssertionHandler`
252-
subclasses). It receives one params_list at a time, already stripped and — unless the
253-
handler sets `VALIDATE_CURIES = False` — with its CURIEs validated and pre-warmed in
254-
the NodeNorm cache. Yield one result per thing checked, usually one per CURIE, so a
255-
failure names the CURIE that failed. Override `curie_params()` if some params are not
256-
CURIEs; see `HasLabel` and `SearchByName`.
274+
3. Declare how many params a params_list may have with `MIN_PARAMS` and `MAX_PARAMS`
275+
(default: one or more). Arity is checked during preparation, so a params_list of the
276+
wrong length is rejected before any CURIE is looked up and `test_params_list()` never
277+
sees it — do not re-check it by hand.
278+
279+
4. Implement `test_params_list()` (or both `test_with_*` methods for `AssertionHandler`
280+
subclasses). It receives one params_list at a time, of a length you declared, already
281+
stripped and — unless the handler sets `VALIDATE_CURIES = False` — with its CURIEs
282+
validated and pre-warmed in the NodeNorm cache, so you can index into it directly.
283+
Yield one result per thing checked, usually one per CURIE, so a failure names the CURIE
284+
that failed. Override `curie_params()` if some params are not CURIEs; see `HasLabel`
285+
and `SearchByName`.
257286

258-
4. Import it in `__init__.py` and add an instance to `ASSERTION_HANDLERS`. Order does not
287+
5. Import it in `__init__.py` and add an instance to `ASSERTION_HANDLERS`. Order does not
259288
matter — this file groups handlers by the service they test.
260289

261-
5. Run `uv run python -m src.babel_validation.assertions.gen_docs` to regenerate `README.md`,
290+
6. Run `uv run python -m src.babel_validation.assertions.gen_docs` to regenerate `README.md`,
262291
and `uv run pytest -m unit` to confirm the checked-in copy is in sync.

0 commit comments

Comments
 (0)