Reorganize Babel Validation for more extensibility - #101
Merged
Conversation
Move the timeout setting out of tests/pytest.ini into [tool.pytest.ini_options], add testpaths=["tests"] so a bare pytest no longer scans the website node_modules, and add a hatchling build-system that packages src/ so `from src.babel_validation...` imports resolve when installed. Add filelock, used by the Google Sheet disk cache. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Relocate the code under tests/common into an importable src/babel_validation package, splitting the monolithic google_sheet_test_cases module into core (TestRow/TestStatus/TestResult), services (CachedNodeNorm, CachedNameRes) and sources/google_sheets (GoogleSheetTestCases, blocklist). Update the Google Sheet test modules to import from the new locations and to parametrize lazily in pytest_generate_tests via the new tests/_pytest_helpers.deselected_by_markexpr, so marker-deselected runs (e.g. `pytest -m unit`) never hit the network. conftest.py wipes the Google Sheet disk cache at the start of each run. Move test_env.py into tests/test_environment/. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jun 26, 2026
gaurav
added a commit
that referenced
this pull request
Jun 26, 2026
The repo's `.gitignore` predates the Python rewrite — it only covered Scala/Giter8/IntelliJ artifacts, so `__pycache__/` and `*.pyc` showed up as untracked noise throughout the tree and were easy to `git add` by accident. Appends the standard [GitHub `Python.gitignore`](https://github.com/github/gitignore/blob/main/Python.gitignore) template (bytecode, `build/`/`dist/`, `.pytest_cache/`, `.venv`/`.env`, mypy/coverage caches, …) below the existing entries, plus a `.DS_Store` line for macOS. Existing entries are kept at the top, so this is purely additive. Independent of the #67 split stack (#101–#104) and based on `main`, so it can merge immediately. No files are currently tracked that the new patterns would retroactively ignore — `git ls-tree` shows no `.pyc`/`__pycache__` in the tree — so nothing needs `git rm --cached`. ### Verify ``` git check-ignore -v src/__pycache__/x.pyc .venv .env # all matched ``` 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors shared Google Sheet–backed test utilities into an importable src/babel_validation library package, updates existing test modules to use the new import paths, and makes Google Sheet/blocklist parametrization lazy so marker-deselected runs avoid network access.
Changes:
- Carves Google Sheet parsing + shared datatypes into
src/babel_validation(core/services/sources) and updates tests to import from the new locations. - Adds lazy parametrization via
pytest_generate_tests+tests/_pytest_helpers.deselected_by_markexprto avoid collection-time network fetches for deselected runs. - Adds disk caching + file lock for Google Sheet CSV downloads; migrates pytest config into
pyproject.tomland adds hatchling wheel build configuration.
Reviewed changes
Copilot reviewed 14 out of 23 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Adds filelock and updates lock metadata for editable local package. |
| pyproject.toml | Adds filelock, hatchling build config, and moves pytest settings under [tool.pytest.ini_options]. |
| tests/pytest.ini | Removes legacy pytest.ini in favor of pyproject.toml configuration. |
| tests/conftest.py | Deletes tempdir Google Sheet CSV cache on run start (controller-only) and keeps existing pytest configuration/fixtures. |
| tests/_pytest_helpers.py | Adds helper to pre-evaluate -m mark expressions to skip network-backed parametrization when deselected. |
| tests/init.py | Establishes tests as a package (enabling imports like tests._pytest_helpers). |
| tests/test_environment/test_env.py | Updates import to new Google Sheet test-case module location. |
| tests/nodenorm/test_nodenorm_from_gsheet.py | Switches to lazy Google Sheet parametrization and new import path. |
| tests/nodenorm/test_nodenorm_descriptions.py | Changes description identifier collections from sets to lists. |
| tests/nameres/test_nameres_from_gsheet.py | Switches to lazy Google Sheet parametrization, new import path, and sends biolink_type as a list. |
| tests/nameres/test_blocklist.py | Switches blocklist loading to lazy parametrization and new import path. |
| src/init.py | Establishes src as a package so src.babel_validation.* imports work. |
| src/babel_validation/init.py | Initializes the new library package namespace. |
| src/babel_validation/core/init.py | Initializes core subpackage. |
| src/babel_validation/core/testrow.py | Introduces TestRow, TestStatus, and TestResult in the library. |
| src/babel_validation/services/init.py | Initializes services subpackage. |
| src/babel_validation/services/nodenorm.py | Adds CachedNodeNorm wrapper with in-process caching for NodeNorm calls. |
| src/babel_validation/services/nameres.py | Adds CachedNameRes wrapper with in-process caching for NameRes calls. |
| src/babel_validation/sources/init.py | Initializes sources subpackage. |
| src/babel_validation/sources/google_sheets/init.py | Initializes Google Sheets source subpackage. |
| src/babel_validation/sources/google_sheets/google_sheet_test_cases.py | Implements tempdir CSV cache + file lock and uses extracted TestRow. |
| src/babel_validation/sources/google_sheets/blocklist.py | Adds blocklist Google Sheet loader + dataclass representation. |
| CLAUDE.md | Updates architecture/docs to reflect new library layout and import paths. |
Comments suppressed due to low confidence (2)
src/babel_validation/sources/google_sheets/google_sheet_test_cases.py:18
- The library module is using an absolute import through the top-level
srcpackage. Using a relative import here avoids coupling internal modules to the packaging layout (and makes the planned rename away fromsrc.babel_validationeasier), while still working with the current structure.
src/babel_validation/sources/google_sheets/google_sheet_test_cases.py:42 - The cache filename uses only the first 8 hex chars of an MD5, which makes collisions plausible across different Google Sheet IDs; a collision would cause one sheet’s CSV to be reused for another sheet ID (incorrect test data). Using a longer/stronger hash (or the full digest) removes this risk with negligible downside.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Switch absolute src.babel_validation import to a relative import (..core.testrow) so internal modules aren't coupled to the top-level packaging layout. - Use full MD5 digest for cache filename instead of truncating to 8 chars, eliminating the theoretical hash-collision risk across different sheet IDs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
gaurav
changed the title
Adds module docstrings explaining the per-(identifier, params) cache key, the no-auto-eviction policy, and the intended cache-warming pattern: call the batch method once for all identifiers in a task, then use the single-item method per assertion at zero HTTP cost. Adds method docstrings to from_url(), the batch methods (normalize_curies / bulk_lookup), the single-item methods (normalize_curie / lookup), and the cache-clearing methods. Includes a note that lookup() targets a distinct NameRes endpoint from bulk_lookup() and does not delegate to it. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
NodeNormService and NameResService Protocols document the public interface callers should type against. When the implementation is later replaced by an external library, any code typed against the Protocol will need no changes. Rename clear_curie() → invalidate_curie() and delete_query() → invalidate_query(). "Invalidate" is standard cache vocabulary and makes the full-eviction-across-all-param-variants semantics clearer than "clear/delete". Also tightens type annotations on normalize_curie / lookup signatures (str, return type) to match the Protocol. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
gaurav
added a commit
that referenced
this pull request
Aug 18, 2026
main's #101 landed a parallel library reorg, so the shared files conflicted. Resolved by taking main's structure and re-applying this branch's additions: - services/nameres.py, services/nodenorm.py: main's docstrings, Protocol interfaces, and clear_curie/delete_query → invalidate_curie/invalidate_query renames. Re-applied the normalize_curies result seeding from d1d1590 and documented the "one entry per requested CURIE" guarantee in the docstring. - sources/google_sheets/google_sheet_test_cases.py: main's cache_ttl_seconds constructor parameter and relative import. No callers referenced the removed CACHE_TTL_SECONDS class attribute. - tests/conftest.py: main's unlink_if_exists rename, keeping the OSError widening from bcbca58 plus the issue-cache cleanup and the selected_github_issues fixture. - CLAUDE.md, pyproject.toml: this branch is a superset of main's version. - uv.lock: regenerated with `uv lock`. pytest -m unit: 56 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
gaurav
added a commit
that referenced
this pull request
Aug 19, 2026
Adds `src/babel_validation/assertions`: the engine that turns a named BabelTest assertion plus its parameters into a check evaluated against NodeNorm/NameRes. Nothing consumes it yet — the GitHub issue parser that produces the parameters arrives later in the stack — so this PR is the engine, its generated documentation, and its offline tests. **Stack 2 of 4** splitting #67. Base: `main` (#101 is merged). ### The assertion types `AssertionHandler` is the base class; `NodeNormTest` and `NameResTest` specialize it per service. Each concrete handler declares its parameters and yields `TestResult`s. Registered: `Resolves`, `DoesNotResolve`, `ResolvesWith`, `DoesNotResolveWith`, `HasLabel`, `ResolvesWithType`, `SearchByName`, and `Needed` (a placeholder that always fails, marking an issue as still needing a real test). Both wiki (`{{BabelTest|Resolves|CHEBI:15365}}`) and YAML syntax are supported, and an assertion can carry several independent **params lists**, each evaluated separately so one bad one doesn't sink the rest. ### Shared parameter handling `AssertionHandler.prepare_params_lists()` strips whitespace from every param, rejects params lists whose CURIEs are malformed, and warms the NodeNorm cache for the survivors in a single batched request. Both `test_with_nodenorm()` and `test_with_nameres()` route through it, so the NameRes path gets the same validation and the same one-request warming rather than a NodeNorm round-trip per params list. Two escape hatches keep that uniform treatment from being wrong for particular assertions: - `curie_params()` narrows which params are CURIEs — `HasLabel` to the first, `ResolvesWithType` to everything after the Biolink type, `SearchByName` to the expected CURIE only (its first param is a free-text query). - `VALIDATE_CURIES = False` opts an assertion out of format validation entirely. `DoesNotResolve` sets it: an identifier that isn't even a well-formed CURIE trivially doesn't resolve, which is exactly what that assertion exists to state, so rejecting it up front would leave the assertion unable to express its own purpose. ### A NodeNorm bulk-normalization fix `CachedNodeNorm.normalize_curies()` built its return value from `response.json()`, so a CURIE that NodeNorm silently omitted from its response was absent from the returned dict rather than present with a `None` value. A caller iterating the results would never see it and would report success for a CURIE it never tested. The warm-cache path happened to re-add the missing key, so the hole only opened on a cold lookup. It now builds the result from the requested CURIEs: exactly one entry per request, in request order. The ordering guarantee matters independently — "first CURIE that resolved" logic previously depended on the server's JSON ordering when cold and on set iteration order (per-process string hash randomization) when warm, so *which* CURIE got blamed in a failure message could vary between runs of the same test. ### Types and naming Parameters are named rather than left as nested `list[str]`: - `ParamsList` — one assertion invocation's parameters. Position is significant (`ResolvesWithType` takes its Biolink type first, `HasLabel` is `[curie, label]`), which is why this is a list. - `PreparedParamsList` — a frozen `(params, failure)` record. `prepare_params_lists()` returns a list of these rather than a `(stripped, failures_by_index)` tuple, so callers don't re-zip two structures by hand. Service parameters are annotated with the `NodeNormService` / `NameResService` Protocols that `services/` already defines for the purpose, rather than the concrete `CachedNodeNorm` / `CachedNameRes`, so a future drop-in replacement needs no changes here. ### Guardrails - **Registration.** `_register()` replaces the `{h.NAME: h for h in [...]}` comprehension and raises on a `NAME` that isn't lowercase or one that's already taken. Lowercase is load-bearing — the README promises users that assertion names are matched case-insensitively, which only holds if every registry key is lowercase — and a duplicate `NAME` would otherwise silently drop a handler. Both are mistakes only made while adding an assertion, so they fail loudly at import. - **Missing Biolink types.** When NodeNorm returns a node with no type, messages show `NO TYPE RETURNED`. The earlier placeholder, `unknown type`, had the shape of a real type — the older Biolink vocabulary was lowercase prose like `chemical entity` — so it could be misread as something Babel actually returned. ### Documentation `gen_docs.py` renders `assertions/README.md` from the handler class attributes, grouping handlers by the service they test rather than by their order in `ASSERTION_HANDLERS` — otherwise a handler registered in the wrong place lands under the wrong heading, and the sync test can't catch it because it regenerates the same wrong output. `tests/test_environment/test_assertions_docs.py` asserts the checked-in README stays in sync. The "Adding a New Assertion Type" instructions live in that generated README and nowhere else. There had been a second copy in the package docstring and the two had already drifted; the docstring now points at the README and describes the module layout instead. ### Tests `tests/test_environment/test_assertions.py` stubs `requests.post` rather than the service, so handlers run against the real `CachedNodeNorm` and exercise its contract instead of a fake restating it. The fixture DB drops one CURIE from the response entirely, reproducing what NodeNorm does for some unknown identifiers. 13 unit tests, all offline. Also registers the `unit` pytest marker, first used by these tests. ### Outcomes - The assertion vocabulary is fixed, documented, and enforced, so the issue parser in the next part of the stack has a stable target. - Every check here runs offline — no network, no GitHub API, no Google Sheet. - Deliberately not here: parsing assertions out of issue bodies, and any wiring into the existing pytest suites. ### Notes for review `gh pr diff` shows hunks in `.gitignore`, `services/nameres.py`, `sources/google_sheets/`, and `tests/conftest.py` that came from #101 and are already on `main` — they merge as no-ops. The net change against `main` is the `assertions/` package, the `normalize_curies()` fix, the two test files, and the pytest marker. `result: dict` is left untyped for NodeNorm response entries. A `TypedDict` would be more precise, but the response shape is Babel's to change and a wrong one is worse than an honest `dict`, so the docstrings say what the dict is instead. ### Verify - `uv run pytest -m unit -q` → 13 assertion tests pass offline. - `uv run python -m src.babel_validation.assertions.gen_docs` reproduces the committed README. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
gaurav
added a commit
that referenced
this pull request
Aug 26, 2026
) 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)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pure-ish refactor, no service-behavior change. Moves the code under
tests/common/into an importablesrc/babel_validationpackage and updates the Google Sheet test modules to match.What changes
tests/common/google_sheet_test_cases.pyinto:core/testrow.py—TestRow/TestStatus/TestResultservices/{nodenorm,nameres}.py—CachedNodeNorm/CachedNameRessources/google_sheets/{google_sheet_test_cases,blocklist}.pypytest_generate_testsviatests/_pytest_helpers.deselected_by_markexpr, so marker-deselected runs never hit the network.tests/pytest.iniinto[tool.pytest.ini_options]; add a hatchling build that packagessrc/; addfilelock(gsheet disk cache).test_env.py→tests/test_environment/.Two incidental one-liners ride along in the final test files (NameRes
biolink_typepassed as a list; description identifiers as lists not sets) — kept as-is so #67 rebases cleanly.Verify
uv run pytest -m unit --collect-only -q— full suite imports/collects offline (no network).uv run pytest tests/test_environment/test_env.py— live Google Sheet download+parse through the moved code.🤖 Generated with Claude Code