diff --git a/.github/workflows/examples.yml b/.github/workflows/examples.yml index 8c2b3c2d..cad0f7b2 100644 --- a/.github/workflows/examples.yml +++ b/.github/workflows/examples.yml @@ -8,6 +8,13 @@ on: schedule: - cron: '0 0 1 * *' # Runs at 00:00 UTC on the 1st day of every month +# One live run per branch — the README examples run real queries against the +# production VFB backend, so superseded runs are cancelled rather than left to +# pile up alongside the other live-backend workflows. +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + jobs: test-examples: runs-on: ubuntu-latest @@ -21,8 +28,10 @@ jobs: - name: Install dependencies run: | python -m pip install --upgrade pip - pip install -r requirements.txt - pip install deepdiff colorama + # Runtime set + test tooling. deepdiff and colorama, previously + # installed ad hoc here, are now declared in tests/requirements.txt + # alongside the rest of the test-only dependencies. + pip install -r requirements.txt -r tests/requirements.txt pip install . - name: Check SOLR availability run: | diff --git a/.github/workflows/performance-test.yml b/.github/workflows/performance-test.yml index 3311c967..c2516e46 100644 --- a/.github/workflows/performance-test.yml +++ b/.github/workflows/performance-test.yml @@ -9,6 +9,15 @@ on: schedule: - cron: '0 2 * * *' # Runs daily at 2 AM UTC +# One live run per branch. This job hammers production VFB infra for up to two +# hours; several stacked runs of it are the heaviest load this repo can put on +# the backend. cancel-in-progress is limited to pull_request events because the +# push and scheduled runs commit the refreshed performance.md back to main — +# cancelling one of those loses the measurement rather than just duplicating it. +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + jobs: performance: name: "Performance Test" @@ -35,12 +44,12 @@ jobs: - name: Install dependencies run: | python -m pip install --upgrade pip - python -m pip install --upgrade -r requirements.txt + # Runtime set + test tooling (pytest, pytest-timeout, pytest-xdist — + # the last powers the parallel Connectivity Tests step below). The two + # files are separate so test tooling stays out of the published + # package and the Docker image; see tests/requirements.txt. + python -m pip install --upgrade -r requirements.txt -r tests/requirements.txt python -m pip install -e . # Editable install ensures we test the actual source code - # pytest-xdist powers the `-n auto` parallel run in the - # Connectivity Tests step below. Installed here rather than in - # requirements.txt because it's only needed at test time. - python -m pip install pytest-xdist - name: Test Owlery Connectivity run: | @@ -127,12 +136,15 @@ jobs: run: | # These files are pytest-style (plain classes + @pytest.mark.integration). # Run with pytest so the markers are honoured and collection works. - # `-n auto` parallelises across all available CPU cores via - # pytest-xdist (typically 2-4 on GitHub-hosted ubuntu runners). The - # connectivity tests hit the live upstream and don't share fixtures - # or in-process state, so they parallelise cleanly. SOLR cache writes - # are idempotent so a race between two cold workers on the same - # term_id just produces two identical writes. + # `-n 4` parallelises via pytest-xdist. The connectivity tests hit the + # live upstream and don't share fixtures or in-process state, so they + # parallelise cleanly; SOLR cache writes are idempotent, so a race + # between two cold workers on the same term_id just produces two + # identical writes. + # Capped at 4 (was 8): the GitHub-hosted ubuntu runner only has 4 + # vCPUs, so 8 workers bought no extra throughput — it just doubled the + # number of concurrent connectomics queries pointed at production + # Neo4j. 4 matches the runner and the "Run Tests" workflow. # Auto-retry once on failure — same rationale as Run Performance Test. # Per-step log is concatenated into the canonical # performance_test_output.log only after the final (possibly retried) @@ -140,7 +152,7 @@ jobs: # grades on the last attempt only. set +e echo "=== Connectivity test attempt 1/2 (parallel) ===" - pytest -v -s -n 8 \ + pytest -v -s -n 4 \ src/test/test_neuron_neuron_connectivity.py \ src/test/test_neuron_region_connectivity.py \ src/test/test_upstream_class_connectivity.py \ @@ -154,7 +166,7 @@ jobs: fi echo "" echo "=== Connectivity attempt 1 failed (exit $FIRST_EXIT). Retrying once with warm cache. ===" - pytest -v -s -n 8 \ + pytest -v -s -n 4 \ src/test/test_neuron_neuron_connectivity.py \ src/test/test_neuron_region_connectivity.py \ src/test/test_upstream_class_connectivity.py \ diff --git a/.github/workflows/python-test.yml b/.github/workflows/python-test.yml index 64795e57..5413d8d5 100644 --- a/.github/workflows/python-test.yml +++ b/.github/workflows/python-test.yml @@ -9,6 +9,23 @@ on: schedule: - cron: '0 0 2 * *' # Runs at 00:00 UTC on the 2nd day of every month +# One live run per branch. Every run drives the full correctness suite against +# the production VFB backend (Neo4j / SOLR / Owlery), so a series of quick +# pushes to a PR would otherwise stack several full suites against production +# simultaneously. Superseded runs are cancelled — only the newest commit's +# result is meaningful anyway. +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +# Needed by the "Comment skip warning on PR" step to post/update a sticky +# comment on the PR conversation. (A ::warning:: annotation alone only shows on +# the Checks/Files tabs — the conversation timeline stays green despite skips.) +permissions: + contents: read + pull-requests: write # sticky skip-warning comment on the PR conversation + checks: write # a neutral (grey) "Backend coverage" check when skipped + jobs: notebooks: name: "Run Tests" @@ -19,13 +36,18 @@ jobs: - name: Set up Python uses: actions/setup-python@v2 with: - python-version: 3.8 + # Match the Performance Test workflow (the repo's other pytest runner) + # rather than the retired 3.8 this job used when it ran a single + # unittest file, so pytest / pytest-xdist resolve the same versions. + python-version: '3.10' - name: Install dependencies run: | python -m pip install -U pip - python -m pip install -U -r requirements.txt + # Runtime set + test tooling (pytest, pytest-timeout, pytest-xdist). + # See tests/requirements.txt for why the two are separate files. + python -m pip install -U -r requirements.txt -r tests/requirements.txt python -m pip install . - - name: Run term_info_queries_test + - name: Run full test suite env: VFBQUERY_CACHE_ENABLED: 'false' MPLBACKEND: 'Agg' @@ -33,4 +55,135 @@ jobs: VISPY_USE_EGL: '0' run: | export PYTHONPATH=$PYTHONPATH:$PWD/ - python -m unittest -v src/test/term_info_queries_test.py + set -o pipefail + # Full correctness suite across src/test and tests (was: only + # term_info_queries_test.py). Parallel via pytest-xdist, grouped per + # file (--dist loadscope) so each file's backend connections stay on + # one worker; the 300s per-test timeout from pyproject.toml bounds any + # single hung upstream call. `-ra` prints a summary of skips/failures. + # A backend outage SKIPS the affected tests (see conftest.py) rather + # than failing them; empty-but-connected results still fail. The next + # step turns any skips into a PR-visible warning. + # Excludes: test_query_performance.py — wall-clock threshold + # assertions that flap under parallel load, already gated by the + # dedicated "Performance Test" workflow; and test_examples_diff.py / + # test_examples_code.py — README-example scripts (no pytest tests, + # pull in deepdiff/colorama) run by the "Test VFBquery examples" + # workflow instead. + # -n 4 rather than -n auto: an explicit cap on how many concurrent + # query streams one run points at production. `auto` happens to be 4 + # on today's GitHub-hosted ubuntu runner, so this is not a slowdown — + # it just stops the load on VFB infra changing silently if the hosted + # runner spec grows. + pytest -v -ra -n 4 --dist loadscope \ + --ignore=src/test/test_query_performance.py \ + --ignore=src/test/test_examples_diff.py \ + --ignore=src/test/test_examples_code.py \ + src/test tests 2>&1 | tee pytest_output.log + - name: Flag skipped tests (backend unavailable) + if: always() + run: | + # Skips are invisible on the PR otherwise (a pass+skip run is a green + # check). Surface them as a warning annotation so a backend outage — + # which the conftest.py skip hook turns into skips rather than a false + # red — is visible without opening the Actions logs. + if [ ! -f pytest_output.log ]; then + echo "No test output captured."; exit 0 + fi + summary=$(grep -Eo '[0-9]+ skipped' pytest_output.log | tail -1 || true) + if [ -n "$summary" ]; then + echo "::warning title=Tests skipped — VFB backend unreachable::${summary}. These are NOT test failures and not a problem with this branch: the VFB backend (Neo4j / SOLR / Owlery) did not answer, so those queries went unverified this run. Treat a green check with skips as an incomplete run — re-run once the backend is healthy before relying on it. See the job log for the list." + else + echo "No tests skipped." + fi + - name: Comment skip warning on PR + # The ::warning:: above only surfaces on the Checks/Files tabs; the PR + # conversation still shows a green check. Post a sticky comment there so a + # skipped (== incomplete) run is visible without opening the Actions logs. + # Same-repo PRs only — a fork PR gets a read-only token and can't comment. + if: always() && github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository + uses: actions/github-script@v7 + with: + script: | + const fs = require('fs'); + const marker = ''; + + let skipped = 0, summary = ''; + try { + const log = fs.readFileSync('pytest_output.log', 'utf8'); + const s = [...log.matchAll(/(\d+) skipped/g)]; + if (s.length) skipped = parseInt(s[s.length - 1][1], 10); + const line = log.match(/^=+ (.+ in [\d.]+s.*?) =+\s*$/gm); + if (line) summary = line[line.length - 1].replace(/=/g, '').trim(); + } catch (e) { + core.info('No pytest_output.log to read: ' + e.message); + } + + const { owner, repo } = context.repo; + const issue_number = context.issue.number; + + // Direct link to THIS run so the reader re-runs the right thing: the + // "Run Tests" job (this "${{ github.workflow }}" workflow), NOT the + // neutral status check below — that check has no job behind it, so + // re-running it would do nothing. + const runUrl = `${process.env.GITHUB_SERVER_URL}/${owner}/${repo}/actions/runs/${context.runId}`; + const rerun = `To re-run: open [this workflow run](${runUrl}) and click ` + + `**Re-run all jobs** once the backend is healthy (re-running the ` + + `“Run completeness” check itself does nothing — it has no job behind it).`; + + // A neutral (grey) status check so the PR's checks box stops reading + // as a plain green pass when the run was actually incomplete. Neutral + // does not fail the PR or block merge — it just isn't "success". Named + // "Run completeness" (a verdict, not a runnable job) so it isn't + // mistaken for the thing to re-run. + const head_sha = context.payload.pull_request.head.sha; + await github.rest.checks.create({ + owner, repo, head_sha, + name: 'Run completeness', + status: 'completed', + conclusion: skipped > 0 ? 'neutral' : 'success', + details_url: runUrl, + output: { + title: skipped > 0 + ? `${skipped} test(s) skipped — backend unreachable (incomplete run)` + : 'All backend tests ran', + summary: skipped > 0 + ? (`**${skipped}** test(s) were skipped because the VFB backend ` + + `(Neo4j / SOLR / Owlery) did not answer, so those queries went ` + + `unverified. This is not a branch failure — but the run is ` + + `incomplete.\n\n${rerun}` + + (summary ? '\n\n```\n' + summary + '\n```' : '')) + : 'Every backend-dependent test reached the VFB backend and ran.', + }, + }); + const comments = await github.paginate(github.rest.issues.listComments, + { owner, repo, issue_number, per_page: 100 }); + const existing = comments.find(c => c.body && c.body.includes(marker)); + + if (skipped > 0) { + const body = [ + marker, + `### ⚠️ ${skipped} test(s) skipped — VFB backend was unreachable`, + '', + `The full suite ran, but **${skipped}** test(s) were **skipped** because the ` + + `VFB backend (Neo4j / SOLR / Owlery) did not answer during this run.`, + '', + 'These are **not failures** and **not a problem with this branch** — but those ' + + 'queries went **unverified**, so a green check here is an *incomplete* run.', + '', + '> ' + rerun, + summary ? '\n```\n' + summary + '\n```' : '', + '', + 'Posted automatically. This comment is removed once a run completes with zero skips.', + ].join('\n'); + if (existing) { + await github.rest.issues.updateComment({ owner, repo, comment_id: existing.id, body }); + } else { + await github.rest.issues.createComment({ owner, repo, issue_number, body }); + } + core.warning(`${skipped} test(s) skipped — posted PR comment.`); + } else if (existing) { + // Clean run: drop the stale warning so the conversation reflects reality. + await github.rest.issues.deleteComment({ owner, repo, comment_id: existing.id }); + core.info('Zero skips — removed the previous skip-warning comment.'); + } diff --git a/.github/workflows/test-lint.yml b/.github/workflows/test-lint.yml new file mode 100644 index 00000000..0bf2146b --- /dev/null +++ b/.github/workflows/test-lint.yml @@ -0,0 +1,35 @@ +name: Test Lint + +# Fast, static check that a PR doesn't introduce the silently-passing test +# anti-patterns documented in TESTING.md (empty-suppressing guards, stale keys, +# error-swallowing except blocks). Only the lines the PR ADDS are checked, and a +# `# test-lint: allow` comment opts a line out for a genuine exception. + +on: + pull_request: + branches: [ main, dev ] + workflow_dispatch: + +# Purely static — no backend involved — but there is no value in finishing a +# lint of a commit that has already been superseded. +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +jobs: + test-lint: + name: "Test conventions" + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 # need history to diff against the base branch + + - uses: actions/setup-python@v5 + with: + python-version: '3.10' + + - name: Lint added test lines + run: | + git fetch --no-tags --quiet origin "${{ github.base_ref }}" + python scripts/lint_tests.py "origin/${{ github.base_ref }}" diff --git a/README.md b/README.md index af84fcde..a37c44b3 100644 --- a/README.md +++ b/README.md @@ -9,6 +9,11 @@ pip install --upgrade vfbquery HTTP API, the `/combine` set-algebra reference, the lightweight `vfbquery-client` package, and the query catalogue, all rendered and cross-linked. This README is the quick start. +🧪 **Adding or changing a test?** Read **[TESTING.md](TESTING.md)** first. The suite runs live +queries against the VFB backend, so tests must assert real content (never suppress an empty result, +never swallow errors, always verify fixtures return data). Those rules exist because a batch of +silently-passing tests was found and fixed — the doc is how we keep them fixed. + ## 🚀 Performance & Caching VFBquery includes intelligent SOLR-based caching for optimal performance: diff --git a/TESTING.md b/TESTING.md new file mode 100644 index 00000000..e2e24f45 --- /dev/null +++ b/TESTING.md @@ -0,0 +1,186 @@ +# Testing VFBquery + +Most VFBquery tests run **live queries against the production VFB backend** +(SOLR, Neo4j, Owlery, FlyBase Chado). That makes them powerful — they catch real +regressions in query results — but also easy to write badly: a test that never +checks its query returned anything passes forever while the query is silently +broken. A whole class of such tests was found and fixed in Aug 2026; this doc +exists so they don't come back. + +Read this before adding or changing a test. + +## Installing the test dependencies + +Test tooling is declared in `tests/requirements.txt`, separately from the +runtime dependencies in `requirements.txt`. Install both: + +```bash +pip install -r requirements.txt -r tests/requirements.txt +pip install -e . +``` + +The split exists so that test tooling never reaches an end user: `requirements.txt` +mirrors `setup.py`'s `install_requires` and is what the `Dockerfile` copies into +the runtime image, while `tests/requirements.txt` holds pytest, `pytest-timeout` +(which enforces the 300 s per-test ceiling from `pyproject.toml`), `pytest-xdist` +(the `-n` parallel runner) and the `deepdiff`/`colorama` pair used only by +`src/test/test_examples_diff.py`. If you add a test-only dependency, put it in +`tests/requirements.txt` — not in `requirements.txt`, and not as an ad-hoc +`pip install` inside a workflow step. + +## Running the suite + +```bash +export PYTHONPATH=$PYTHONPATH:$PWD/ +export VFBQUERY_CACHE_ENABLED=false # test the code, not the cache +pytest -v -ra -n 4 --dist loadscope src/test tests +``` + +- The whole suite runs on every PR via `.github/workflows/python-test.yml`. + Timing/performance checks live separately in `performance-test.yml` + (`test_query_performance.py`), because their thresholds only hold with the + cache warm. +- `-ra` prints a summary of skips at the end; the CI job turns any skips into a + PR **warning** so a backend outage can't hide behind a green check. A green + check *with* that warning means the run was incomplete because the VFB backend + was unavailable — it is a report on backend health, not on the branch. Re-run + it once the backend is answering before treating the branch as verified. +- Every live-backend workflow sets a `concurrency` group keyed on the branch, so + a run is cancelled when a newer commit supersedes it. Parallelism is pinned at + `-n 4` (the hosted runner's vCPU count) rather than `-n auto`, to keep the + number of concurrent query streams aimed at production explicit and stable. +- A separate **Test Lint** check (`test-lint.yml` → `scripts/lint_tests.py`) + fails the PR if it introduces any of the anti-patterns below. It only inspects + the lines your PR *adds*. For a genuine exception (a deliberate empty-result + test, a graceful-handling test), put `# test-lint: allow` on that line. You can + run it locally: `python scripts/lint_tests.py origin/main`. + +## How the suite treats the backend (read this — it drives the rules below) + +`conftest.py` enforces one policy, and every test must fit it: + +| Situation | Outcome | +|---|---| +| Backend **unreachable** (connection refused / timeout / 5xx gateway) | **SKIP** (shown as a PR warning) | +| Query **reaches** the backend and returns **no rows** | **FAIL** | +| Query returns rows | assertions run normally | + +So an empty result is **never** an acceptable outcome for a known-populated +term — it is a bug. A backend outage is handled *for you*; you do not need +(and must not add) your own try/except to survive it. + +## The rules + +### 1. Assert content, not just shape + +A backend test must assert the query **did its job** — returned the rows it +should — not merely that it returned a dict/DataFrame with the right keys. + +```python +# BAD — passes even when the query returns nothing +result = get_parts_of("FBbt_00003748") +self.assertIn("rows", result) + +# GOOD +result = get_parts_of("FBbt_00003748") +self.assertTrue(result["rows"], "mushroom body should have parts") +``` + +### 2. Never suppress an empty result + +Do **not** wrap assertions in a truthiness guard. When the query returns nothing +the guarded assertions silently don't run and the test passes. + +```python +# BAD — every check below is skipped on an empty result +if not result.empty: + self.assertIn("id", result.columns) + +# BAD — same thing with a dict +if result["rows"]: + self.assertEqual(result["rows"][0]["id"], expected) + +# GOOD — empty is a failure; the checks always run +self.assertFalse(result.empty, " should return rows") +self.assertIn("id", result.columns) +``` + +The only legitimate empties are **deliberate negative tests** — e.g. querying an +invalid/nonexistent id to check graceful handling. Name them clearly +(`test_..._empty_result`) and assert the empty shape on purpose. + +### 3. Never swallow exceptions + +Do not put a query inside `try/except` that turns a failure into a pass, a +silent skip, or a hard failure. It defeats the connection-skip policy and hides +real errors. + +```python +# BAD — a connection outage becomes a hard failure; a real bug is masked +try: + result = get_similar_morphology(neuron) + ... +except Exception as e: + self.fail(f"Query failed: {e}") # or: pass / self.skipTest(...) + +# GOOD — let it propagate. conftest turns an outage into a skip; a real +# error surfaces as an error. +result = get_similar_morphology(neuron) +self.assertFalse(result.empty, " should have NBLAST matches") +``` + +### 4. Verify every fixture actually returns data + +Before you assert on a term/id, confirm it is real, of the right **type**, and +**populated** — then record the count in a comment so the next person can trust +it. Real bugs found this way: + +- a **template** (`VFB_00101567`, `VFB_00050000`) used as an "example neuron" — + NBLAST returned 0 for it; +- a **Channel** node (`VFBc_00050000`) used as a "template" — painted domains + returned 0; +- a non-existent DOI-style id (`DOI_10_7554_eLife_04577`) — the real node is + `FBrf0227179`; +- placeholder ids commented "may need to be updated with real data". + +```python +# GOOD — real, typed, populated, and the count is documented +self.nblast_term = "VFB_jrchk00s" # neuron with NBLAST matches (215) +``` + +Quick check while writing: + +```python +print(get_similar_morphology("VFB_jrchk00s", return_dataframe=False, limit=5)["count"]) +``` + +Prefer stable, well-known ids (classic alleles, standard templates, +long-standing anatomy classes) over incidental ones. + +### 5. Assert the keys/columns the function *actually* returns + +Read the query function (or run it once) — do not assume. Two vacuous tests +existed because they checked keys that never appear: + +- checking `result["data"]` when the function returns `rows` (so + `if "data" in result` was always false and nothing ran); +- asserting a `label` column when the function returns `name`. + +### 6. Wire new test files into CI + +`python-test.yml` runs `pytest src/test tests` — a new `test_*.py` under either +directory is picked up automatically. If you add a file that is a script rather +than a pytest module, or a pure-timing test, add an explicit `--ignore` there +with a comment (see the existing `test_examples_*` / `test_query_performance` +ignores). + +## Checklist for a new backend test + +- [ ] Uses a real, correctly-typed id, verified to return rows (count in a comment). +- [ ] Asserts the result is **non-empty** (no `if not empty:` / `if rows:` guard). +- [ ] Asserts the **real** keys/columns the function returns. +- [ ] No `try/except` around the query that swallows errors. +- [ ] Deliberate empty/negative cases are named and assert the empty shape on purpose. +- [ ] Runs under `pytest src/test tests` (picked up by CI). + +See `conftest.py` for the connection-skip mechanics. diff --git a/conftest.py b/conftest.py new file mode 100644 index 00000000..0091f69e --- /dev/null +++ b/conftest.py @@ -0,0 +1,277 @@ +"""Root pytest configuration shared by the whole suite (``src/test`` + ``tests``). + +For how to WRITE a good test (assert real content, never suppress empty +results, verify fixtures), see ``TESTING.md``. This file is the runtime +mechanics that make those rules safe on CI. + +Everything here is about one thing: how the suite reacts to the live VFB +backend (SOLR / Neo4j / Owlery / FlyBase Chado) being unreachable on CI. + +Policy: + * A **connection failure SKIPS** the test rather than failing it, so a backend + outage does not turn every PR red (``pytest_runtest_makereport`` below). The + CI job surfaces the skip count as a ``::warning::`` annotation, so a mass + skip is visible on the PR instead of hiding behind a green check. + * A genuinely **empty result still FAILS** — a query that reaches the backend + and returns no rows is a real defect, not an outage. The skip path is + reserved for transport-level failures (can't connect / timed out), never for + an empty-but-successful response. + +The Neo4j REST client is the awkward case. ``neo4j_client.commit_list`` returns +``False`` on a connection failure (``dict_cursor`` then turns that into an empty +list) instead of raising, so a dead Neo4j looks identical to an empty result at +the call site. To keep the two apart we probe the backend once per session and +only when that probe shows Neo4j is down do we make ``commit_list`` raise on +``False`` (so it routes into the skip path). When Neo4j is up, a ``False`` means +a real server/query error and is left to fail. This shim is test-only; the +library's production behaviour is untouched. + +Finally, a mid-run circuit breaker: if the backend dies PART-WAY through a run +(healthy at session start, so the shim above never armed), the remaining +backend tests would each burn their full 300s timeout. Instead, the first +connection failure / timeout sets a shared latch; subsequent tests do a short +health probe and skip fast while the backend stays down, resuming the moment it +answers again. Zero cost on a healthy run. +""" +import os +import socket +import tempfile +import time +import concurrent.futures + +import pytest +import requests + + +# -------------------------------------------------------------------------- +# Connection-failure detection +# -------------------------------------------------------------------------- + +# Exception type *names* (matched by name so optional deps needn't be imported) +# that always mean "couldn't reach / talk to the backend". +_CONNECTION_TYPE_NAMES = frozenset({ + "ConnectionError", "ConnectionResetError", "ConnectionRefusedError", + "ConnectionAbortedError", "TimeoutError", "ConnectTimeout", + "ConnectTimeoutError", "ReadTimeout", "ReadTimeoutError", "MaxRetryError", + "NewConnectionError", "ProtocolError", "ServiceUnavailable", + "SessionExpired", "OperationalError", +}) + +# Substrings that mark a transport failure even when the concrete exception is a +# generic wrapper (pysolr.SolrError, RuntimeError, …). Deliberately narrow: +# gateway 502/503/504 and socket phrases only — NOT bare "500" / "server error", +# which can be a genuine query bug that must stay a failure. +_CONNECTION_MESSAGE_MARKERS = ( + "failed to establish a new connection", "max retries exceeded", + "connection refused", "connection reset", "connection aborted", + "connection timed out", "read timed out", "name or service not known", + "temporary failure in name resolution", "no route to host", + "network is unreachable", "neo4j unreachable", + "502 bad gateway", "503 service unavailable", "504 gateway", +) + + +def _is_connection_failure(exc): + """True if ``exc`` (or a cause/context in its chain) is a transport failure.""" + seen = set() + while exc is not None and id(exc) not in seen: + seen.add(id(exc)) + if isinstance(exc, (socket.timeout, socket.gaierror, + concurrent.futures.TimeoutError)): + return True + if type(exc).__name__ in _CONNECTION_TYPE_NAMES: + return True + if any(m in str(exc).lower() for m in _CONNECTION_MESSAGE_MARKERS): + return True + exc = exc.__cause__ or exc.__context__ + return False + + +@pytest.hookimpl(hookwrapper=True) +def pytest_runtest_makereport(item, call): + """Turn a transport-level failure into a skip (never an empty result), and + arm the mid-run circuit breaker on a connection failure or a timeout.""" + outcome = yield + rep = outcome.get_result() + if rep.when in ("setup", "call") and rep.failed and call.excinfo is not None: + exc = call.excinfo.value + if _is_connection_failure(exc): + rep.outcome = "skipped" + rep.longrepr = ( + str(item.fspath), + item.location[1] or 0, + f"VFB backend unreachable: {type(exc).__name__}: {exc}", + ) + _mark_outage() + elif "pytest-timeout" in str(exc): + # A test hit the per-test ceiling. If the backend is down this is an + # outage casualty, not a slow query — record it and let it read as a + # skip; otherwise it's a genuine hang/perf failure, left untouched. + # + # A single instantaneous probe is not enough here: a heavy query + # (term-info fan-out) hangs the moment the backend degrades, but the + # lightweight HTTP health endpoints keep answering for up to ~a + # minute after — so the first in-flight tests to hit the 300s ceiling + # would see a "healthy" ping and be left as failures while every + # later test correctly skipped. Poll for a short window so the lagging + # transport symptom is caught and the canary tests skip too. + _mark_outage() + if _backend_down_confirm(): + rep.outcome = "skipped" + rep.longrepr = ( + str(item.fspath), + item.location[1] or 0, + "VFB backend outage: test timed out and a health probe " + "confirms the backend is down", + ) + + +# -------------------------------------------------------------------------- +# One-shot Neo4j probe + False->raise shim (see module docstring) +# -------------------------------------------------------------------------- + +def _neo4j_is_down(): + try: + from vfbquery import vfb_queries as vq + return not vq.vc.nc.commit_list(["RETURN 1 AS ok"]) + except Exception as exc: # a raising client on a dead host also means down + return _is_connection_failure(exc) or True + + +@pytest.fixture(scope="session", autouse=True) +def _neo4j_connection_shim(): + """Only when Neo4j is unreachable, make ``commit_list`` raise on its + ``False`` connection-failure return so the skip hook catches it. No-op when + Neo4j is up — a ``False`` then is a real error and must still fail. Only + tests that actually call ``commit_list`` are affected, so pure/offline tests + are untouched.""" + if not _neo4j_is_down(): + yield + return + from vfbquery import vfb_queries as vq + nc = vq.vc.nc + original = nc.commit_list + + def _raising_commit_list(*args, **kwargs): + result = original(*args, **kwargs) + if result is False: + raise ConnectionError("Neo4j unreachable (commit_list returned False)") + return result + + nc.commit_list = _raising_commit_list + try: + yield + finally: + nc.commit_list = original + + +# -------------------------------------------------------------------------- +# Mid-run outage circuit breaker (see module docstring) +# -------------------------------------------------------------------------- + +# Shared across xdist workers via a file — each worker is a separate process, so +# in-memory state would not be seen by the others. Keyed on the run so parallel +# invocations don't collide. +_OUTAGE_LATCH = os.path.join( + tempfile.gettempdir(), + "vfbquery_outage_" + os.environ.get("PYTEST_XDIST_TESTRUNUID", str(os.getppid())), +) +_OUTAGE_RECENT_S = 60 # a failure newer than this means "trouble right now" +_PROBE_CACHE_S = 10 # re-probe at most this often, per worker +_PROBE_TIMEOUT_S = 5 + +# Cheap health endpoints. Any HTTP answer — even Owlery's 404 on the base path — +# means the host is reachable; a 5xx or a transport error means it is not. +_PROBE_URLS = ( + "http://solr.virtualflybrain.org/solr/vfb_json/admin/ping", + "http://pdb.virtualflybrain.org/", + "http://owl.virtualflybrain.org/kbs/vfb/", +) +_probe_cache = {"at": 0.0, "down": False} + + +def pytest_sessionstart(session): + # Start every run with a clean latch — the latch path can be reused across + # runs launched from the same shell, and a stale one would make the first + # tests probe needlessly. + _clear_outage() + + +def _mark_outage(): + try: + with open(_OUTAGE_LATCH, "w") as fh: + fh.write(repr(time.time())) + except OSError: + pass + + +def _outage_signalled_recently(): + try: + with open(_OUTAGE_LATCH) as fh: + return (time.time() - float(fh.read().strip())) < _OUTAGE_RECENT_S + except (OSError, ValueError): + return False + + +def _clear_outage(): + try: + os.remove(_OUTAGE_LATCH) + except OSError: + pass + + +def _backend_down(): + """Short, per-worker-cached health probe. True if any VFB backend is + unreachable or returning 5xx. Errs toward 'down' so a partial outage still + trips the breaker rather than letting those tests time out.""" + now = time.time() + if now - _probe_cache["at"] < _PROBE_CACHE_S: + return _probe_cache["down"] + down = False + for url in _PROBE_URLS: + try: + if requests.get(url, timeout=_PROBE_TIMEOUT_S).status_code >= 500: + down = True + break + except requests.RequestException: + down = True + break + _probe_cache.update(at=now, down=down) + return down + + +# How long, and how often, to keep re-probing after a pytest-timeout before +# concluding the backend is genuinely healthy (so the hang was a real code +# defect, not an outage). Sized to cover the observed lag between a heavy query +# hanging and the HTTP health endpoints degrading (~40-70s in run 32834691217). +_CONFIRM_WINDOW_S = 90 +_CONFIRM_INTERVAL_S = 5 + + +def _backend_down_confirm(): + """Stronger version of :func:`_backend_down` used only after a pytest + timeout. An outage's transport symptoms can lag a heavy query's hang by up + to a minute, so poll the health endpoints for a short window and report + down as soon as any probe fails; only conclude 'healthy' (a real hang, kept + as a failure) after the whole window stays up.""" + deadline = time.time() + _CONFIRM_WINDOW_S + while True: + _probe_cache["at"] = 0.0 # bypass the 10s cache — we want a fresh read + if _backend_down(): + return True + if time.time() >= deadline: + return False + time.sleep(_CONFIRM_INTERVAL_S) + + +def pytest_runtest_setup(item): + """Fast-skip a test when a backend outage was signalled recently AND a fresh + probe confirms the backend is still down — rather than letting it burn the + full per-test timeout. Clears the latch and resumes once the backend answers + again, so a transient blip only pauses the suite briefly.""" + if _outage_signalled_recently(): + if _backend_down(): + pytest.skip("VFB backend outage detected mid-run — skipping to avoid " + "per-test timeouts; re-run once the backend is healthy") + else: + _clear_outage() diff --git a/docs/ci/performance-test.yml b/docs/ci/performance-test.yml index a860678c..dbae0a08 100644 --- a/docs/ci/performance-test.yml +++ b/docs/ci/performance-test.yml @@ -9,6 +9,15 @@ on: schedule: - cron: '0 2 * * *' # Runs daily at 2 AM UTC +# One live run per branch. This job hammers production VFB infra for up to two +# hours; several stacked runs of it are the heaviest load this repo can put on +# the backend. cancel-in-progress is limited to pull_request events because the +# push and scheduled runs commit the refreshed performance.md back to main — +# cancelling one of those loses the measurement rather than just duplicating it. +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + jobs: performance: name: "Performance Test" @@ -35,12 +44,12 @@ jobs: - name: Install dependencies run: | python -m pip install --upgrade pip - python -m pip install --upgrade -r requirements.txt + # Runtime set + test tooling (pytest, pytest-timeout, pytest-xdist — + # the last powers the parallel Connectivity Tests step below). The two + # files are separate so test tooling stays out of the published + # package and the Docker image; see tests/requirements.txt. + python -m pip install --upgrade -r requirements.txt -r tests/requirements.txt python -m pip install -e . # Editable install ensures we test the actual source code - # pytest-xdist powers the `-n auto` parallel run in the - # Connectivity Tests step below. Installed here rather than in - # requirements.txt because it's only needed at test time. - python -m pip install pytest-xdist - name: Test Owlery Connectivity run: | @@ -177,12 +186,15 @@ jobs: run: | # These files are pytest-style (plain classes + @pytest.mark.integration). # Run with pytest so the markers are honoured and collection works. - # `-n auto` parallelises across all available CPU cores via - # pytest-xdist (typically 2-4 on GitHub-hosted ubuntu runners). The - # connectivity tests hit the live upstream and don't share fixtures - # or in-process state, so they parallelise cleanly. SOLR cache writes - # are idempotent so a race between two cold workers on the same - # term_id just produces two identical writes. + # `-n 4` parallelises via pytest-xdist. The connectivity tests hit the + # live upstream and don't share fixtures or in-process state, so they + # parallelise cleanly; SOLR cache writes are idempotent, so a race + # between two cold workers on the same term_id just produces two + # identical writes. + # Capped at 4 (was 8): the GitHub-hosted ubuntu runner only has 4 + # vCPUs, so 8 workers bought no extra throughput — it just doubled the + # number of concurrent connectomics queries pointed at production + # Neo4j. 4 matches the runner and the "Run Tests" workflow. # Auto-retry once on failure — same rationale as Run Performance Test. # Per-step log is concatenated into the canonical # performance_test_output.log only after the final (possibly retried) @@ -190,7 +202,7 @@ jobs: # grades on the last attempt only. set +e echo "=== Connectivity test attempt 1/2 (parallel) ===" - pytest -v -s -n 8 \ + pytest -v -s -n 4 \ src/test/test_neuron_neuron_connectivity.py \ src/test/test_neuron_region_connectivity.py \ src/test/test_upstream_class_connectivity.py \ @@ -204,7 +216,7 @@ jobs: fi echo "" echo "=== Connectivity attempt 1 failed (exit $FIRST_EXIT). Retrying once with warm cache. ===" - pytest -v -s -n 8 \ + pytest -v -s -n 4 \ src/test/test_neuron_neuron_connectivity.py \ src/test/test_neuron_region_connectivity.py \ src/test/test_upstream_class_connectivity.py \ diff --git a/requirements.txt b/requirements.txt index b56fdded..34bf3ef2 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,3 +1,8 @@ +# Runtime dependencies only — this file mirrors setup.py's `install_requires` +# for CI and for the Docker image build (see Dockerfile). Test tooling (pytest, +# pytest-timeout, pytest-xdist, deepdiff, colorama) lives in +# tests/requirements.txt so it stays out of the runtime image; install both with +# pip install -r requirements.txt -r tests/requirements.txt vfb_connect dataclasses-json dacite @@ -6,5 +11,3 @@ pysolr get_version aiohttp psycopg[binary]>=3.0 -pytest -pytest-timeout diff --git a/scripts/lint_tests.py b/scripts/lint_tests.py new file mode 100644 index 00000000..ae0b411b --- /dev/null +++ b/scripts/lint_tests.py @@ -0,0 +1,135 @@ +#!/usr/bin/env python3 +"""Lint test files for the silently-passing anti-patterns documented in TESTING.md. + +Operates on the lines a change ADDS (diffs ``...HEAD``), so it flags what a +PR introduces rather than pre-existing code. For a genuine exception (a +deliberate empty-result test, a graceful-handling test), put ``# test-lint: allow`` +on the offending line. + +Usage: + python scripts/lint_tests.py [] + LINT_BASE=origin/main python scripts/lint_tests.py + +Exit code 1 if any new violations are found. +""" +import os +import re +import subprocess +import sys + +TESTING_DOC = "TESTING.md" +ALLOW = "test-lint: allow" + +# Single-line guards that hide an empty result. Matched against the stripped line. +LINE_RULES = [ + (re.compile(r"^if\s+not\s+[\w.]+\.empty\s*:"), + "empty-guard", + "`if not X.empty:` around assertions lets an empty result pass silently — " + "use `self.assertFalse(X.empty, ...)` instead"), + (re.compile(r"^if\s+[\w.]+\[['\"]rows['\"]\]\s*:"), + "rows-guard", + "`if result['rows']:` around assertions lets an empty result pass silently — " + "assert `self.assertTrue(result['rows'], ...)` first"), + (re.compile(r"^if\s+[\w.]+\.get\(['\"](?:rows|data)['\"]\)\s*:"), + "rows-guard", + "`if result.get('rows'):` around assertions lets an empty result pass " + "silently — assert non-empty first"), + (re.compile(r"^if\s+['\"]data['\"]\s+in\s+\w+"), + "stale-data-key", + "VFBquery query results use the 'rows' key, not 'data' — this guard is " + "always false; use 'rows' and assert non-empty"), + (re.compile(r"^if\s+.*\blen\([^)]*\)\s*>\s*0\s*:"), + "len-guard", + "`if len(...) > 0:` around assertions lets an empty result pass silently — " + "assert non-empty first"), + (re.compile(r"^if\s+[\w.]+\[['\"]count['\"]\]\s*>\s*0\s*:"), + "count-guard", + "`if result['count'] > 0:` around assertions lets an empty result pass " + "silently — assert `self.assertGreater(result['count'], 0, ...)`"), +] + +# Error-hiding handler bodies (flagged only inside an `except`). +SWALLOW_BODY = re.compile(r"self\.(fail|skipTest)\(") + +_TEST_FILE = re.compile(r"(^|/)test_[^/]*\.py$") + + +def changed_test_lines(base): + """{path: set(added line numbers)} for changed test files under src/test, tests.""" + diff = subprocess.run( + ["git", "diff", "--unified=0", f"{base}...HEAD", "--", "src/test", "tests"], + capture_output=True, text=True, check=True).stdout + files, path = {}, None + for line in diff.splitlines(): + if line.startswith("+++ b/"): + candidate = line[6:] + path = candidate if _TEST_FILE.search(candidate) else None + if path: + files.setdefault(path, set()) + elif path and line.startswith("@@"): + m = re.search(r"\+(\d+)(?:,(\d+))?", line) + if m: + start, count = int(m.group(1)), int(m.group(2) or 1) + files[path].update(range(start, start + count)) + return files + + +def _indent(line): + return len(line) - len(line.lstrip()) + + +def enclosing_is_except(lines, idx): + """True if the block directly containing line ``idx`` (0-based) is an `except`.""" + body_indent = _indent(lines[idx]) + for j in range(idx - 1, -1, -1): + s = lines[j].strip() + if not s or s.startswith("#"): + continue + if _indent(lines[j]) < body_indent: + return s.startswith("except") + return False + + +def main(): + base = sys.argv[1] if len(sys.argv) > 1 else os.environ.get("LINT_BASE", "origin/main") + violations = [] + for path, added in changed_test_lines(base).items(): + try: + lines = open(path, encoding="utf-8").read().splitlines() + except OSError: + continue + for n in sorted(added): + if n - 1 >= len(lines): + continue + raw = lines[n - 1] + if ALLOW in raw: + continue + stripped = raw.strip() + for rx, rule, msg in LINE_RULES: + if rx.search(stripped): + violations.append((path, n, rule, msg, stripped)) + break + else: + if SWALLOW_BODY.search(stripped) and enclosing_is_except(lines, n - 1): + violations.append(( + path, n, "swallow-in-except", + "swallowing a query error in `except` hides real failures and " + "defeats the connection-skip policy — let it propagate", + stripped)) + + if violations: + print(f"\n✗ Test-lint found {len(violations)} issue(s) — see {TESTING_DOC}:\n") + for path, n, rule, msg, stripped in violations: + print(f" {path}:{n} [{rule}]") + print(f" {msg}") + print(f" | {stripped}\n") + print(f"Fix per {TESTING_DOC}, or add `# {ALLOW}` on the line for a genuine " + f"exception (e.g. a deliberate empty-result test).") + return 1 + + print("✓ Test-lint: no new silently-passing patterns in changed test lines.") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/src/test/test_anatomy_expressed_in.py b/src/test/test_anatomy_expressed_in.py index e5458069..e39bc012 100644 --- a/src/test/test_anatomy_expressed_in.py +++ b/src/test/test_anatomy_expressed_in.py @@ -11,11 +11,13 @@ """ import unittest +import os import sys import pandas as pd -# Add src directory to path for imports -sys.path.insert(0, '/Users/rcourt/GIT/VFBquery/src') +# Add the repo's src directory to the path for imports (relative to this file, +# not a hardcoded developer path). +sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), '..'))) from vfbquery import vfb_queries as vq @@ -25,24 +27,34 @@ class TestAnatomyExpressedIn(unittest.TestCase): def test_anatomy_expressed_in_basic_dataframe(self): """Test basic query returns DataFrame with expected columns""" - result = vq.get_expression_overlaps_here('VFBexp_FBtp0001321', return_dataframe=True) + # limit=5 keeps this structural check fast and robust: at limit=-1 the + # per-row enrichment over all ~79 overlapping anatomy classes can time + # out under parallel CI load and come back empty, which this test (now + # asserting non-empty) would read as a failure. + result = vq.get_expression_overlaps_here('VFBexp_FBtp0001321', return_dataframe=True, limit=5) self.assertIsInstance(result, pd.DataFrame, "Should return pandas DataFrame") - if not result.empty: - expected_columns = ['id', 'name', 'tags', 'pubs'] - for col in expected_columns: - self.assertIn(col, result.columns, f"DataFrame should contain '{col}' column") + # VFBexp_FBtp0001321 is a known-populated expression pattern: an empty + # result is a defect, not an acceptable outcome (a backend outage skips + # this test upstream via conftest.py rather than reaching here empty). + self.assertFalse(result.empty, "Query for a known-populated expression pattern returned no rows") + expected_columns = ['id', 'name', 'tags', 'pubs'] + for col in expected_columns: + self.assertIn(col, result.columns, f"DataFrame should contain '{col}' column") - self.assertTrue(all(isinstance(x, str) for x in result['id']), "IDs should be strings") - self.assertTrue(all(isinstance(x, str) for x in result['name']), "Names should be strings") + self.assertTrue(all(isinstance(x, str) for x in result['id']), "IDs should be strings") + self.assertTrue(all(isinstance(x, str) for x in result['name']), "Names should be strings") - print(f"\nFound {len(result)} anatomy classes where VFBexp_FBtp0001321 is expressed") - print(f"Sample results: {result.head(3)[['id', 'name']].to_dict('records')}") + print(f"\nFound {len(result)} anatomy classes where VFBexp_FBtp0001321 is expressed") + print(f"Sample results: {result.head(3)[['id', 'name']].to_dict('records')}") def test_anatomy_expressed_in_formatted_output(self): """Test query returns properly formatted dictionary output""" - result = vq.get_expression_overlaps_here('VFBexp_FBtp0001321', return_dataframe=False) + # limit the enrichment (per-row Stage/Template/Technique/Thumbnail + # walks) to keep this structural check fast; `count` is independent of + # the limit so the printed total is still the full result size. + result = vq.get_expression_overlaps_here('VFBexp_FBtp0001321', return_dataframe=False, limit=5) self.assertIsInstance(result, dict, "Should return dictionary when return_dataframe=False") @@ -51,34 +63,42 @@ def test_anatomy_expressed_in_formatted_output(self): self.assertIn('count', result, "Result should contain 'count'") headers = result['headers'] - expected_headers = ['id', 'name', 'tags', 'pubs'] - for header in expected_headers: + # v1.14.2: full column shape (Name / Reference / Gross_Type / Stage / + # Template_Space / Imaging_Technique / Images). + expected_types = { + 'id': 'selection_id', + 'name': 'markdown', + 'pubs': 'markdown', + 'tags': 'tags', + 'stages': 'text', + 'template': 'markdown', + 'technique': 'text', + 'thumbnail': 'markdown', + } + for header, expected_type in expected_types.items(): self.assertIn(header, headers, f"Headers should contain '{header}'") self.assertIn('title', headers[header], f"Header '{header}' should have 'title'") self.assertIn('type', headers[header], f"Header '{header}' should have 'type'") self.assertIn('order', headers[header], f"Header '{header}' should have 'order'") + self.assertEqual(headers[header]['type'], expected_type, + f"Header '{header}' should be type '{expected_type}'") - self.assertEqual(headers['id']['type'], 'selection_id') - self.assertEqual(headers['name']['type'], 'markdown') - self.assertEqual(headers['tags']['type'], 'tags') - self.assertEqual(headers['pubs']['type'], 'metadata') - - if result['rows']: - first_row = result['rows'][0] - for key in expected_headers: - self.assertIn(key, first_row, f"Row should contain '{key}'") + self.assertTrue(result['rows'], "Query for a known-populated expression pattern returned no rows") + first_row = result['rows'][0] + for key in expected_types: + self.assertIn(key, first_row, f"Row should contain '{key}'") - print(f"\nFormatted output contains {result['count']} anatomy classes") - print(f"Sample row keys: {list(first_row.keys())}") + print(f"\nFormatted output contains {result['count']} anatomy classes") + print(f"Sample row keys: {list(first_row.keys())}") def test_anatomy_expressed_in_limit(self): """Test limit parameter restricts number of results""" limit = 3 result = vq.get_expression_overlaps_here('VFBexp_FBtp0001321', return_dataframe=True, limit=limit) - if not result.empty: - self.assertLessEqual(len(result), limit, f"Should return at most {limit} results") - print(f"\nLimit parameter working: requested {limit}, got {len(result)}") + self.assertFalse(result.empty, "Query for a known-populated expression pattern returned no rows") + self.assertLessEqual(len(result), limit, f"Should return at most {limit} results") + print(f"\nLimit parameter working: requested {limit}, got {len(result)}") def test_anatomy_expressed_in_empty_result(self): """Test query with an id that has no expression overlaps""" @@ -89,51 +109,54 @@ def test_anatomy_expressed_in_empty_result(self): print(f"\nEmpty result handling works correctly") def test_anatomy_expressed_in_publication_data(self): - """Test that publication data is properly formatted when present""" - result = vq.get_expression_overlaps_here('VFBexp_FBtp0001321', return_dataframe=True, limit=10) - - if not result.empty: - self.assertIn('pubs', result.columns, "Should have 'pubs' column") + """Test that publication data is formatted as markdown links when present. - for idx, row in result.iterrows(): - if row['pubs']: - pubs = row['pubs'] - self.assertIsInstance(pubs, list, "Publications should be a list") - - if pubs: - first_pub = pubs[0] - self.assertIsInstance(first_pub, dict, "Publication should be a dict") + v1.14.7: the pubs column is a `; `-joined string of `[label](id)` + markdown links (rendered by V2's QueryLinkArrayComponent), not the + legacy list-of-pub-dicts. An anatomy row with no citation is an empty + string. + """ + result = vq.get_expression_overlaps_here('VFBexp_FBtp0001321', return_dataframe=True, limit=10) - if 'core' in first_pub: - self.assertIn('short_form', first_pub['core'], "Publication should have short_form") + self.assertFalse(result.empty, "Query for a known-populated expression pattern returned no rows") + self.assertIn('pubs', result.columns, "Should have 'pubs' column") - print(f"\nPublication data properly structured") - break + for idx, row in result.iterrows(): + pubs = row['pubs'] + self.assertIsInstance(pubs, str, "Publications should be a markdown string") + if pubs: + # Each entry is a `[label](id)` markdown link. + self.assertIn('[', pubs, "Publication should contain markdown link start") + self.assertIn('](', pubs, "Publication should contain markdown link separator") + self.assertIn(')', pubs, "Publication should contain markdown link end") + print(f"\nPublication data properly structured: {pubs}") + break def test_anatomy_expressed_in_markdown_encoding(self): """Test that markdown links are properly formatted""" result = vq.get_expression_overlaps_here('VFBexp_FBtp0001321', return_dataframe=True, limit=5) - if not result.empty: - for name in result['name']: - self.assertIn('[', name, "Name should contain markdown link start") - self.assertIn('](', name, "Name should contain markdown link separator") - self.assertIn(')', name, "Name should contain markdown link end") + self.assertFalse(result.empty, "Query for a known-populated expression pattern returned no rows") + for name in result['name']: + self.assertIn('[', name, "Name should contain markdown link start") + self.assertIn('](', name, "Name should contain markdown link separator") + self.assertIn(')', name, "Name should contain markdown link end") - print(f"\nMarkdown links properly formatted") + print(f"\nMarkdown links properly formatted") def test_anatomy_expressed_in_tags_format(self): """Test that tags are properly formatted as pipe-separated strings""" result = vq.get_expression_overlaps_here('VFBexp_FBtp0001321', return_dataframe=True, limit=5) - if not result.empty and 'tags' in result.columns: - for tags in result['tags']: - if pd.notna(tags) and tags: - self.assertIsInstance(tags, str, "Tags should be string type") - parts = tags.split('|') - self.assertTrue(all(isinstance(p, str) for p in parts), "Tag parts should be strings") + self.assertFalse(result.empty, "Query for a known-populated expression pattern returned no rows") + self.assertIn('tags', result.columns, "Should have 'tags' column") + for tags in result['tags']: + if pd.notna(tags) and tags: + self.assertIsInstance(tags, str, "Tags should be string type") + parts = tags.split('|') + self.assertTrue(all(isinstance(p, str) for p in parts), "Tag parts should be strings") - print(f"\nTags format verified") + print(f"\nTags format verified") class TestAnatomyExpressedInSchema(unittest.TestCase): @@ -159,7 +182,12 @@ def test_schema_structure(self): self.assertEqual(schema.function, "get_expression_overlaps_here") self.assertIn("Anatomy where", schema.label) self.assertEqual(schema.preview, 5) - self.assertEqual(schema.preview_columns, ["id", "name", "tags", "pubs"]) + # v1.14.2: gained Stage / Template / Imaging Technique / Thumbnail + # columns to match the legacy ExpressionOverlapsHere column shape. + self.assertEqual( + schema.preview_columns, + ["id", "name", "pubs", "tags", "stages", "template", "technique", "thumbnail"], + ) # takes constrains the input to an expression pattern (or fragment) self.assertIn("short_form", schema.takes) diff --git a/src/test/test_dataset_template_queries.py b/src/test/test_dataset_template_queries.py index 9818ba54..72ef60de 100644 --- a/src/test/test_dataset_template_queries.py +++ b/src/test/test_dataset_template_queries.py @@ -39,8 +39,12 @@ class DatasetTemplateQueriesTest(unittest.TestCase): def setUp(self): """Set up test fixtures""" - self.template_term = 'VFBc_00050000' # Adult Brain template - self.dataset_term = 'VFBc_00101384' # Example dataset + # Real fixtures: the previous values (VFBc_00050000 / VFBc_00101384) + # were Channel nodes, so every query returned 0 and the guards below + # passed vacuously. VFB_00017894 is the adult brain template (58 painted + # domains, 21 aligned datasets); Takagi2017 is a dataset with images. + self.template_term = 'VFB_00017894' # adult brain template + self.dataset_term = 'Takagi2017' # a dataset with images def test_get_painted_domains(self): """Test get_painted_domains query""" @@ -48,20 +52,20 @@ def test_get_painted_domains(self): self.assertIsNotNone(result, "Result should not be None") import pandas as pd - if isinstance(result, pd.DataFrame) and len(result) > 0: - print(f"\n✓ Found {len(result)} painted domains for {self.template_term}") - self.assertIn('id', result.columns) - self.assertIn('label', result.columns) - self.assertIn('thumbnail', result.columns) + self.assertIsInstance(result, pd.DataFrame) + self.assertFalse(result.empty, f"{self.template_term} should have painted domains") + print(f"\n✓ Found {len(result)} painted domains for {self.template_term}") + self.assertIn('id', result.columns) + self.assertIn('name', result.columns) + self.assertIn('thumbnail', result.columns) def test_get_painted_domains_formatted(self): """Test get_painted_domains with formatted output""" result = get_painted_domains(self.template_term, return_dataframe=False, limit=5) - self.assertIsNotNone(result) - - if isinstance(result, dict): - self.assertIn('headers', result) - self.assertIn('rows', result) + self.assertIsInstance(result, dict) + self.assertIn('headers', result) + self.assertIn('rows', result) + self.assertTrue(result['rows'], f"{self.template_term} should have painted domains") def test_get_dataset_images(self): """Test get_dataset_images query""" @@ -69,9 +73,10 @@ def test_get_dataset_images(self): self.assertIsNotNone(result) import pandas as pd - if isinstance(result, pd.DataFrame) and len(result) > 0: - print(f"\n✓ Found {len(result)} images in dataset {self.dataset_term}") - self.assertIn('id', result.columns) + self.assertIsInstance(result, pd.DataFrame) + self.assertFalse(result.empty, f"{self.dataset_term} should have images") + print(f"\n✓ Found {len(result)} images in dataset {self.dataset_term}") + self.assertIn('id', result.columns) def test_get_all_aligned_images(self): """Test get_all_aligned_images query""" @@ -79,8 +84,9 @@ def test_get_all_aligned_images(self): self.assertIsNotNone(result) import pandas as pd - if isinstance(result, pd.DataFrame) and len(result) > 0: - print(f"\n✓ Found {len(result)} aligned images for {self.template_term}") + self.assertIsInstance(result, pd.DataFrame) + self.assertFalse(result.empty, f"{self.template_term} should have aligned images") + print(f"\n✓ Found {len(result)} aligned images for {self.template_term}") def test_get_aligned_datasets(self): """Test get_aligned_datasets query""" @@ -88,8 +94,9 @@ def test_get_aligned_datasets(self): self.assertIsNotNone(result) import pandas as pd - if isinstance(result, pd.DataFrame) and len(result) > 0: - print(f"\n✓ Found {len(result)} aligned datasets for {self.template_term}") + self.assertIsInstance(result, pd.DataFrame) + self.assertFalse(result.empty, f"{self.template_term} should have aligned datasets") + print(f"\n✓ Found {len(result)} aligned datasets for {self.template_term}") def test_get_all_datasets(self): """Test get_all_datasets query (no parameters)""" diff --git a/src/test/test_default_caching.py b/src/test/test_default_caching.py deleted file mode 100644 index fcc659dc..00000000 --- a/src/test/test_default_caching.py +++ /dev/null @@ -1,178 +0,0 @@ -""" -Test VFBquery default caching functionality. - -These tests ensure that the SOLR-based caching system works correctly -and provides expected performance benefits with 3-month TTL. -""" - -import unittest -import os -import time -from unittest.mock import MagicMock -import sys - -# Mock vispy imports before importing vfbquery -for module in ['vispy', 'vispy.scene', 'vispy.util', 'vispy.util.fonts', - 'vispy.util.fonts._triage', 'vispy.util.fonts._quartz', - 'vispy.ext', 'vispy.ext.cocoapy', 'navis', 'navis.plotting', - 'navis.plotting.vispy', 'navis.plotting.vispy.viewer']: - sys.modules[module] = MagicMock() - -# Set environment variables -os.environ.update({ - 'MPLBACKEND': 'Agg', - 'VISPY_GL_LIB': 'osmesa', - 'VISPY_USE_EGL': '0', - 'VFBQUERY_CACHE_ENABLED': 'true' -}) - - -class TestDefaultCaching(unittest.TestCase): - """Test default SOLR caching behavior in VFBquery.""" - - def setUp(self): - """Set up test environment.""" - # Clear any existing cache before each test - try: - import vfbquery - if hasattr(vfbquery, 'clear_solr_cache'): - # Clear cache for a test term - vfbquery.clear_solr_cache('term_info', 'FBbt_00003748') - except ImportError: - pass - - def test_caching_enabled_by_default(self): - """Test that SOLR caching is automatically enabled when importing vfbquery.""" - import vfbquery - - # Check that SOLR caching functions are available - self.assertTrue(hasattr(vfbquery, 'get_solr_cache')) - self.assertTrue(hasattr(vfbquery, 'clear_solr_cache')) - self.assertTrue(hasattr(vfbquery, 'get_solr_cache_stats_func')) - - # Check that caching is enabled (we can't easily check SOLR stats without network calls) - # But we can verify the infrastructure is in place - self.assertTrue(hasattr(vfbquery, '__caching_available__')) - self.assertTrue(vfbquery.__caching_available__) - - def test_cache_performance_improvement(self): - """Test that SOLR caching provides performance improvement.""" - import vfbquery - - test_term = 'FBbt_00003748' # medulla - - # First call (cold - populates cache) - start_time = time.time() - result1 = vfbquery.get_term_info(test_term) - cold_time = time.time() - start_time - - # Verify we got a result - self.assertIsNotNone(result1) - if result1 is not None: - self.assertIn('Name', result1) - - # Second call (warm - should hit cache) - start_time = time.time() - result2 = vfbquery.get_term_info(test_term) - warm_time = time.time() - start_time - - # Verify caching is working (results should be identical) - self.assertIsNotNone(result2) - self.assertEqual(result1, result2) # Should be identical - - # Note: Performance improvement may vary due to network conditions - # The main test is that caching prevents redundant computation - - # Check SOLR cache statistics - solr_stats = vfbquery.get_solr_cache_stats_func() - self.assertIsInstance(solr_stats, dict) - self.assertIn('total_cache_documents', solr_stats) - - def test_cache_statistics_tracking(self): - """Test that SOLR cache statistics are properly tracked.""" - import vfbquery - - # Get baseline SOLR stats - initial_stats = vfbquery.get_solr_cache_stats_func() - initial_docs = initial_stats['total_cache_documents'] - - # Make a unique query that should populate cache - unique_term = 'FBbt_00005106' # Use a different term - result = vfbquery.get_term_info(unique_term) - self.assertIsNotNone(result) - - # Check that SOLR stats were updated (may take time to reflect) - # We mainly verify the stats function works and returns reasonable data - updated_stats = vfbquery.get_solr_cache_stats_func() - self.assertIsInstance(updated_stats, dict) - self.assertIn('total_cache_documents', updated_stats) - self.assertIn('cache_efficiency', updated_stats) - - def test_memory_size_tracking(self): - """Test that SOLR cache size is properly tracked.""" - import vfbquery - - # Cache a few different terms - test_terms = ['FBbt_00003748', 'VFB_00101567'] - - for term in test_terms: - result = vfbquery.get_term_info(term) - self.assertIsNotNone(result) - - # Check SOLR cache stats are available - stats = vfbquery.get_solr_cache_stats_func() - self.assertIsInstance(stats, dict) - self.assertIn('estimated_size_mb', stats) - self.assertGreaterEqual(stats['estimated_size_mb'], 0) - - def test_cache_ttl_configuration(self): - """Test that SOLR cache TTL is properly configured.""" - import vfbquery - - # Get SOLR cache instance to check TTL - solr_cache = vfbquery.get_solr_cache() - self.assertIsNotNone(solr_cache) - - # Check that TTL is configured (we can't easily check the exact value without accessing private attributes) - # But we can verify the cache object exists and has expected methods - self.assertTrue(hasattr(solr_cache, 'ttl_hours')) - self.assertTrue(hasattr(solr_cache, 'cache_result')) - self.assertTrue(hasattr(solr_cache, 'get_cached_result')) - - def test_transparent_caching(self): - """Test that regular VFBquery functions are transparently cached.""" - import vfbquery - - # Test that get_term_info and get_instances are using cached versions - test_term = 'FBbt_00003748' - - # These should work with caching transparently - term_info = vfbquery.get_term_info(test_term) - self.assertIsNotNone(term_info) - - instances = vfbquery.get_instances(test_term, limit=5) - self.assertIsNotNone(instances) - - # SOLR cache should be accessible - solr_stats = vfbquery.get_solr_cache_stats_func() - self.assertIsInstance(solr_stats, dict) - self.assertIn('total_cache_documents', solr_stats) - - def test_cache_disable_environment_variable(self): - """Test that caching can be disabled via environment variable.""" - # This test would need to be run in a separate process to test - # the environment variable behavior at import time - # For now, just verify the current state respects the env var - - cache_enabled = os.getenv('VFBQUERY_CACHE_ENABLED', 'true').lower() - if cache_enabled not in ('false', '0', 'no', 'off'): - import vfbquery - # If caching is enabled, SOLR cache should be available - solr_cache = vfbquery.get_solr_cache() - self.assertIsNotNone(solr_cache) - self.assertTrue(hasattr(vfbquery, '__caching_available__')) - self.assertTrue(vfbquery.__caching_available__) - - -if __name__ == '__main__': - unittest.main(verbosity=2) diff --git a/src/test/test_expression_pattern_fragments.py b/src/test/test_expression_pattern_fragments.py index dcd6ad89..960b6f3c 100644 --- a/src/test/test_expression_pattern_fragments.py +++ b/src/test/test_expression_pattern_fragments.py @@ -6,12 +6,17 @@ FIXED: Query now works correctly with proper IRI resolution for VFBexp_* IDs. -NOTE: Some expression patterns cause Owlery server timeouts (>120s). This appears -to be a server-side performance issue with large result sets. The query implementation -is correct - confirmed by URL construction and smaller test cases. +The three execution tests below were skipped for a period because the Owlery +/instances endpoint exceeded the 300 s per-test budget for epFrag on every +expression pattern tried. That was a server-side limitation, not a code defect, +and it has since been resolved: the reference query now answers in ~4 s. -Test URL that times out: -http://owl.virtualflybrain.org/kbs/vfb/instances?object= some + http://owl.virtualflybrain.org/kbs/vfb/instances?object= some + +returns 5823 instances well inside the budget, so the skips are removed and +these tests run for real again. If Owlery regresses, the honest response is to +fix Owlery — re-adding a skip here would hide the regression behind a green +check, which is exactly what TESTING.md forbids. """ import unittest @@ -34,89 +39,70 @@ class TestExpressionPatternFragments(unittest.TestCase): def setUp(self): """Set up test fixtures.""" - # Expression pattern that has known fragments - # epFrag finds individual fragments (Expression_pattern_fragment) that are part_of a Class Expression_pattern - # NOTE: VFBexp_FBtp0022557 causes Owlery timeout (>120s) - likely due to large result set - # Using a smaller test case for faster testing + # epFrag finds individual fragments (Expression_pattern_fragment) that are + # part_of a Class Expression_pattern, via the Owlery /instances endpoint. self.test_expression_pattern = "VFBexp_FBtp0022557" # P{VGlut-GAL4.D} expression pattern - self.test_pattern_times_out = True # Flag indicating this specific test may timeout - + def test_schema_generation(self): """Test that the schema function generates correct Query object.""" schema = epFrag_to_schema("test expression pattern", {"short_form": self.test_expression_pattern}) - + self.assertEqual(schema.query, "epFrag") self.assertEqual(schema.function, "get_expression_pattern_fragments") self.assertIn("test expression pattern", schema.label) self.assertEqual(schema.preview, 5) self.assertIn("id", schema.preview_columns) self.assertIn("thumbnail", schema.preview_columns) - + def test_expression_pattern_fragments_execution(self): """Test that expression pattern fragments query executes and returns results.""" - # Skip this test if we know it will timeout - if self.test_pattern_times_out: - self.skipTest("Owlery server times out on this expression pattern (>120s). " - "This is a server performance issue, not a code bug. " - "Query implementation is correct - verified by URL construction.") - result = get_expression_pattern_fragments(self.test_expression_pattern) - + self.assertIsNotNone(result) # Result can be dict or DataFrame if isinstance(result, dict): self.assertIn('count', result) - # Should return at least 1 result (VFB_00008416) - self.assertGreater(result['count'], 0, + self.assertGreater(result['count'], 0, f"Expected at least 1 result for {self.test_expression_pattern}") print(f"\n✓ Query returned {result['count']} expression pattern fragments") else: # DataFrame self.assertIsInstance(result, pd.DataFrame) - self.assertGreater(len(result), 0, + self.assertGreater(len(result), 0, f"Expected at least 1 result for {self.test_expression_pattern}") print(f"\n✓ Query returned {len(result)} expression pattern fragments") - + def test_return_dataframe_parameter(self): """Test that return_dataframe parameter works correctly.""" - # Test with return_dataframe=True df_result = get_expression_pattern_fragments(self.test_expression_pattern, return_dataframe=True, limit=5) - - # Test with return_dataframe=False dict_result = get_expression_pattern_fragments(self.test_expression_pattern, return_dataframe=False, limit=5) - - # Both should return valid results - self.assertIsNotNone(df_result) - self.assertIsNotNone(dict_result) - + + self.assertIsInstance(df_result, pd.DataFrame) + self.assertFalse(df_result.empty, "expression pattern should have fragments") + self.assertIsInstance(dict_result, dict) + self.assertTrue(dict_result.get('rows'), "expression pattern should have fragments") + def test_limit_parameter(self): """Test that limit parameter restricts results.""" limited_result = get_expression_pattern_fragments(self.test_expression_pattern, return_dataframe=True, limit=3) - - self.assertIsNotNone(limited_result) - - # If results exist, should respect limit - if hasattr(limited_result, '__len__') and len(limited_result) > 0: - self.assertLessEqual(len(limited_result), 3) - + + self.assertIsInstance(limited_result, pd.DataFrame) + self.assertFalse(limited_result.empty, "expression pattern should have fragments") + self.assertLessEqual(len(limited_result), 3) + def test_term_info_integration(self): - """Test that epFrag appears in term_info for expression patterns.""" - # Get term info for an expression pattern + """epFrag must be offered in term_info for an expression pattern (fast: + no query execution, preview=False). Covers the epFrag wiring without the + slow Owlery /instances call.""" term_info = get_term_info(self.test_expression_pattern, preview=False) - + self.assertIsNotNone(term_info) - - # Check if epFrag query is in the queries list - # Note: This will only appear if the term has the correct supertypes - if term_info: - queries = term_info.get('Queries', []) - query_names = [q.get('query') for q in queries if isinstance(q, dict)] - - # epFrag should appear for expression patterns - if 'Expression_pattern' in term_info.get('SuperTypes', []): - self.assertIn('epFrag', query_names, - "epFrag should be available for expression pattern terms") - print(f"\n✓ epFrag query found in term_info for {self.test_expression_pattern}") + self.assertIn('Expression_pattern', term_info.get('SuperTypes', []), + f"{self.test_expression_pattern} should be an Expression_pattern") + query_names = [q.get('query') for q in term_info.get('Queries', []) if isinstance(q, dict)] + self.assertIn('epFrag', query_names, + "epFrag should be available for expression pattern terms") + print(f"\n✓ epFrag query found in term_info for {self.test_expression_pattern}") if __name__ == '__main__': diff --git a/src/test/test_flybase_combo_pubs.py b/src/test/test_flybase_combo_pubs.py index f3c1c6d2..55ab688c 100644 --- a/src/test/test_flybase_combo_pubs.py +++ b/src/test/test_flybase_combo_pubs.py @@ -115,5 +115,6 @@ def test_fbrf_is_a_visible_column(self): fbrf = result["headers"]["fbrf"] assert fbrf["type"] == "text" assert fbrf["order"] >= 0 - if result["rows"]: - assert result["rows"][0]["id"] == result["rows"][0]["fbrf"] + # FBco0000052 has publications, so an empty result is a defect. + assert result["rows"], "KNOWN_COMBO_ID should have publications" + assert result["rows"][0]["id"] == result["rows"][0]["fbrf"] diff --git a/src/test/test_flybase_stocks.py b/src/test/test_flybase_stocks.py index 0bf0be9b..ae7e6d0c 100644 --- a/src/test/test_flybase_stocks.py +++ b/src/test/test_flybase_stocks.py @@ -141,9 +141,11 @@ def test_filter_reduces_count(self): class TestFindStocksAllele: @pytest.mark.integration def test_known_allele(self): - # dpp[hr4] = FBal0000469 - stocks = find_stocks("FBal0000469") + # bcd[25] = FBal0034227, held in stocks (dpp[hr4]/FBal0000469 has none, + # so the old fixture made this test pass without checking anything). + stocks = find_stocks("FBal0034227") assert isinstance(stocks, list) + assert len(stocks) > 0, "bcd[25] (FBal0034227) should be held in at least one stock" class TestFindStocksInsertion: diff --git a/src/test/test_images_neurons.py b/src/test/test_images_neurons.py index 84f3bf6d..07df229f 100644 --- a/src/test/test_images_neurons.py +++ b/src/test/test_images_neurons.py @@ -37,30 +37,30 @@ def test_get_images_neurons_execution(self): # Check result type - handle both DataFrame and dict (from cache) import pandas as pd + # The antennal lobe (FBbt_00007401) has individual neuron images, so an + # empty result is a defect. A backend outage skips this upstream + # (conftest.py) rather than reaching here empty. if isinstance(result, pd.DataFrame): - # DataFrame result - if len(result) > 0: - print(f"\n✓ Found {len(result)} individual neuron images for {self.test_term}") - - # Verify DataFrame has expected columns - self.assertIn('id', result.columns, "Result should have 'id' column") - self.assertIn('label', result.columns, "Result should have 'label' column") - - # Print first few results for verification - print("\nSample results:") - for idx, row in result.head(3).iterrows(): - print(f" - {row.get('label', 'N/A')} ({row.get('id', 'N/A')})") - else: - print(f"\n⚠ No individual neuron images found for {self.test_term} (this may be expected)") + self.assertFalse(result.empty, f"{self.test_term} should have neuron images") + print(f"\n✓ Found {len(result)} individual neuron images for {self.test_term}") + + # Verify DataFrame has expected columns + self.assertIn('id', result.columns, "Result should have 'id' column") + self.assertIn('label', result.columns, "Result should have 'label' column") + + # Print first few results for verification + print("\nSample results:") + for idx, row in result.head(3).iterrows(): + print(f" - {row.get('label', 'N/A')} ({row.get('id', 'N/A')})") elif isinstance(result, dict): # Dict result (from cache) count = result.get('count', 0) rows = result.get('rows', []) + self.assertTrue(rows, f"{self.test_term} should have neuron images") print(f"\n✓ Found {count} total individual neuron images for {self.test_term} (showing {len(rows)})") - if rows: - print("\nSample results:") - for row in rows[:3]: - print(f" - {row.get('label', 'N/A')} ({row.get('id', 'N/A')})") + print("\nSample results:") + for row in rows[:3]: + print(f" - {row.get('label', 'N/A')} ({row.get('id', 'N/A')})") else: self.fail(f"Unexpected result type: {type(result)}") @@ -122,13 +122,11 @@ def test_images_neurons_preview(self): self.assertIn('headers', result, "Result should have 'headers' key") self.assertIn('count', result, "Result should have 'count' key") - if result['count'] > 0: - print(f"\n✓ Preview format validated") - print(f" Total count: {result['count']}") - print(f" Returned rows: {len(result['rows'])}") - print(f" Headers: {list(result['headers'].keys())}") - else: - print(f"\n⚠ No results in preview (this may be expected)") + self.assertGreater(result['count'], 0, f"{self.test_term} should have neuron images") + print(f"\n✓ Preview format validated") + print(f" Total count: {result['count']}") + print(f" Returned rows: {len(result['rows'])}") + print(f" Headers: {list(result['headers'].keys())}") def test_multiple_terms(self): """Test query with multiple synaptic neuropil terms""" diff --git a/src/test/test_images_that_develop_from.py b/src/test/test_images_that_develop_from.py index 723d270b..eed71571 100644 --- a/src/test/test_images_that_develop_from.py +++ b/src/test/test_images_that_develop_from.py @@ -39,33 +39,30 @@ def test_schema_generation(self): self.assertIn("thumbnail", schema.preview_columns) def test_get_images_that_develop_from_execution(self): - """Test that the query executes without errors.""" - try: - # Execute query with limit to keep test fast - result = get_images_that_develop_from(self.test_neuroblast, return_dataframe=True, limit=10) - - # Result should be either a DataFrame or dict - self.assertIsNotNone(result) - - # If we get results, check structure - if hasattr(result, 'empty'): # DataFrame - if not result.empty: - self.assertIn('id', result.columns) - self.assertIn('label', result.columns) - elif isinstance(result, dict): # Dict format - # Check for either 'data' or 'rows' key - self.assertTrue('data' in result or 'rows' in result, - "Result dict should have 'data' or 'rows' key") - - print(f"\n✅ ImagesThatDevelopFrom query executed successfully") - if isinstance(result, dict): - count = result.get('count', len(result.get('rows', result.get('data', [])))) - print(f" Result count: {count} neurons") - elif hasattr(result, 'shape'): - print(f" Result count: {len(result)} neurons") - - except Exception as e: - self.fail(f"Query execution failed: {str(e)}") + """Test that the query executes and returns results.""" + # No blanket try/except -> self.fail here: it would turn a backend + # connection failure into a hard failure, defeating the conftest.py + # skip-on-outage hook. Let exceptions propagate (connection -> skip, + # anything else -> a real error). + result = get_images_that_develop_from(self.test_neuroblast, return_dataframe=True, limit=10) + self.assertIsNotNone(result) + + # FBbt_00001419 (neuroblast MNB) has images that develop from it, so an + # empty result is a defect. + if hasattr(result, 'empty'): # DataFrame + self.assertFalse(result.empty, f"{self.test_neuroblast} should have images that develop from it") + self.assertIn('id', result.columns) + self.assertIn('label', result.columns) + elif isinstance(result, dict): # Dict format + self.assertTrue(result.get('rows') or result.get('data'), + f"{self.test_neuroblast} should have images that develop from it") + + print(f"\n✅ ImagesThatDevelopFrom query executed successfully") + if isinstance(result, dict): + count = result.get('count', len(result.get('rows', result.get('data', [])))) + print(f" Result count: {count} neurons") + elif hasattr(result, 'shape'): + print(f" Result count: {len(result)} neurons") def test_return_dataframe_parameter(self): """Test that return_dataframe parameter works correctly.""" diff --git a/src/test/test_individual_synonyms.py b/src/test/test_individual_synonyms.py new file mode 100644 index 00000000..6c5175b5 --- /dev/null +++ b/src/test/test_individual_synonyms.py @@ -0,0 +1,139 @@ +"""Regression tests: pub-attributed synonyms must be returned for Individuals. + +``term_info_parse_object`` gated the ``pub_syn`` block on +``"Class" in termInfo["SuperTypes"]``, so an Individual carrying a +pub-attributed synonym had it silently dropped — e.g. VFB_00101385, whose +"MEon JRC_FlyEM_Hemibrain" synonym never reached the term info. The gate is +gone; these lock that in, and check Classes did not regress with it. + +Deliberately offline: the input is a hand-built ``term_info`` document, so these +exercise the parsing branch itself rather than whatever the graph happens to +hold today. No backend means no skip path and no dependence on curation. +""" + +import dataclasses +import json +import unittest +import sys +import os + +sys.path.insert(0, os.path.join(os.path.dirname(__file__), '..')) + +from vfbquery.term_info_queries import VfbTerminfo +from vfbquery.vfb_queries import term_info_parse_object + + +class _FakeSolrResults: + """Minimal stand-in for the pysolr results object term_info_parse_object + consumes: it reads only ``.hits`` and ``.docs[0]['term_info'][0]``.""" + + def __init__(self, term_info_json): + self.hits = 1 + self.docs = [{"term_info": [term_info_json]}] + + +def _term_info(short_form, label, types, synonym_label=None): + """A `term_info` JSON document, optionally carrying one pub-attributed + synonym. + + Every top-level VfbTerminfo field is emitted (null where unused) because + dataclasses_json's ``from_json`` requires each key to be present — the real + SOLR documents are dense in the same way. Deriving the key list from the + dataclass keeps this fixture correct if a field is ever added. + """ + doc = { + "term": { + "core": { + "short_form": short_form, + "iri": f"http://virtualflybrain.org/reports/{short_form}", + "label": label, + "types": types, + "unique_facets": types, + "symbol": "", + }, + "description": [], + "comment": [], + }, + "version": "test", + } + if synonym_label is not None: + doc["pub_syn"] = [{ + "synonym": {"label": synonym_label, + "scope": "has_exact_synonym", + "type": ""}, + "pub": { + "core": { + "short_form": "FBrf0239540", + "iri": "http://flybase.org/reports/FBrf0239540", + "label": "Scheffer et al., 2020, eLife 9: e57443", + "types": ["Entity", "Individual", "pub"], + }, + "microref": "Scheffer et al., 2020", + }, + }] + for field in dataclasses.fields(VfbTerminfo): + doc.setdefault(field.name, None) + return json.dumps(doc) + + +INDIVIDUAL_TYPES = ["Entity", "Individual", "VFB", "Adult", "Anatomy", + "Nervous_system", "Synaptic_neuropil_domain", "has_image"] +CLASS_TYPES = ["Entity", "Class", "Anatomy", "Nervous_system"] + +SYNONYM = "MEon JRC_FlyEM_Hemibrain" + + +class IndividualSynonymsTest(unittest.TestCase): + + def _parse(self, term_info_json, short_form): + result = term_info_parse_object(_FakeSolrResults(term_info_json), + short_form) + self.assertIsNotNone(result, f"parse returned None for {short_form}") + return result, [s["label"] for s in result.get("Synonyms", [])] + + def test_individual_pub_synonym_is_returned(self): + """The regression: an Individual's pub_syn must survive parsing.""" + result, labels = self._parse( + _term_info("VFB_00101385", "ME(R) on JRC_FlyEM_Hemibrain", + INDIVIDUAL_TYPES, SYNONYM), + "VFB_00101385") + self.assertTrue(result["IsIndividual"], + "fixture should parse as an Individual, not a Class") + self.assertIn("Synonyms", result, + "an Individual with pub_syn must get a Synonyms block — " + "this was gated on 'Class' and silently dropped") + self.assertIn(SYNONYM, labels) + + def test_class_pub_synonym_still_returned(self): + """Removing the gate must not have cost Classes their synonyms.""" + result, labels = self._parse( + _term_info("FBbt_00003748", "medulla", CLASS_TYPES, "ME"), + "FBbt_00003748") + self.assertTrue(result["IsClass"]) + self.assertIn("ME", labels) + + def test_synonym_carries_its_publication(self): + """The merge step must keep the attributing publication, not just the + label — an unattributed synonym is much less useful.""" + result, _ = self._parse( + _term_info("VFB_00101385", "ME(R) on JRC_FlyEM_Hemibrain", + INDIVIDUAL_TYPES, SYNONYM), + "VFB_00101385") + entry = next(s for s in result["Synonyms"] if s["label"] == SYNONYM) + self.assertTrue(entry.get("publication"), + f"expected an attributing publication, got {entry}") + self.assertIn("FBrf0239540", entry["publication"]) + + def test_individual_without_pub_syn_has_no_synonyms(self): + """No pub_syn means no Synonyms key — the fix must not invent one.""" + result = term_info_parse_object( + _FakeSolrResults(_term_info("VFB_00101385", + "ME(R) on JRC_FlyEM_Hemibrain", + INDIVIDUAL_TYPES)), + "VFB_00101385") + self.assertIsNotNone(result) + self.assertNotIn("Synonyms", result) + + +if __name__ == "__main__": + unittest.main(verbosity=2) diff --git a/src/test/test_lineage_clones_in.py b/src/test/test_lineage_clones_in.py index d55ae00e..5c95661f 100644 --- a/src/test/test_lineage_clones_in.py +++ b/src/test/test_lineage_clones_in.py @@ -46,18 +46,16 @@ def test_query_execution(self): self.assertIsNotNone(result, "Query should return a result") self.assertIsInstance(result, dict, "Result should be a dictionary") - # Check for expected keys - if result: - print(f"Query returned {len(result.get('data', []))} results") - - # Validate data structure - if 'data' in result and len(result['data']) > 0: - first_result = result['data'][0] - self.assertIn('id', first_result, "Result should contain 'id' field") - self.assertIn('label', first_result, "Result should contain 'label' field") - print(f"First result: {first_result.get('label', 'N/A')} ({first_result.get('id', 'N/A')})") - else: - print("No results found (this is OK if no clones overlap this neuropil)") + # FBbt_00007401 (antennal lobe) has lineage clones overlapping it, so an + # empty result is a defect. (The old guard checked a 'data' key the query + # never returns — the key is 'rows' — so it passed vacuously regardless.) + rows = result.get('rows', []) + self.assertTrue(rows, "antennal lobe should have overlapping lineage clones") + print(f"Query returned {result.get('count', len(rows))} results") + first_result = rows[0] + self.assertIn('id', first_result, "Result should contain 'id' field") + self.assertIn('label', first_result, "Result should contain 'label' field") + print(f"First result: {first_result.get('label', 'N/A')} ({first_result.get('id', 'N/A')})") def test_schema_generation(self): """Test schema function generates correct structure""" @@ -76,7 +74,7 @@ def test_schema_generation(self): self.assertEqual(schema.preview, 5, "Preview should be 10") # Check preview columns - expected_columns = ["id", "label", "tags", "thumbnail"] + expected_columns = ["id", "label", "tags", "template", "technique", "thumbnail"] self.assertEqual(schema.preview_columns, expected_columns, f"Preview columns should be {expected_columns}") print(f"Schema generated successfully: {schema.label}") diff --git a/src/test/test_nblast_queries.py b/src/test/test_nblast_queries.py index fd806ed2..d3136f97 100644 --- a/src/test/test_nblast_queries.py +++ b/src/test/test_nblast_queries.py @@ -42,62 +42,64 @@ class NBLASTQueriesTest(unittest.TestCase): def setUp(self): """Set up test fixtures""" - self.nblast_term = 'VFB_00101567' # Has NBLAST matches - self.neuron_term = 'VFB_00050000' # Example neuron + # Real fixtures. The old values (VFB_00101567 / VFB_00050000) were + # TEMPLATES, not neurons, so every NBLAST query returned 0 and the + # len>0 guards below passed vacuously. Verified counts: NBLAST 215, + # NeuronBridge 15, NBLASTexp 20, reverse-NBLASTexp 14, NB-exp 40. + self.nblast_term = 'VFB_jrchk00s' # neuron with NBLAST + NeuronBridge matches + self.nblastexp_term = 'VFB_00016103' # neuron with NBLASTexp matches + self.exp_term = 'VFB_001012yj' # expression pattern with NBLASTexp + NeuronBridge def test_get_similar_morphology(self): """Test get_similar_morphology query""" - result = get_similar_morphology(self.nblast_term, return_dataframe=True, limit=5) - self.assertIsNotNone(result, "Result should not be None") - import pandas as pd - if isinstance(result, pd.DataFrame) and len(result) > 0: - print(f"\n✓ Found {len(result)} NBLAST matches for {self.nblast_term}") - self.assertIn('id', result.columns) - self.assertIn('label', result.columns) - self.assertIn('score', result.columns) - + result = get_similar_morphology(self.nblast_term, return_dataframe=True, limit=5) + self.assertIsInstance(result, pd.DataFrame) + self.assertFalse(result.empty, f"{self.nblast_term} should have NBLAST matches") + print(f"\n✓ Found {len(result)} NBLAST matches for {self.nblast_term}") + self.assertIn('id', result.columns) + self.assertIn('name', result.columns) + self.assertIn('score', result.columns) + def test_get_similar_morphology_formatted(self): """Test get_similar_morphology with formatted output""" result = get_similar_morphology(self.nblast_term, return_dataframe=False, limit=3) - self.assertIsNotNone(result) - - if isinstance(result, dict): - self.assertIn('headers', result) - self.assertIn('rows', result) - + self.assertIsInstance(result, dict) + self.assertIn('headers', result) + self.assertIn('rows', result) + self.assertTrue(result['rows'], f"{self.nblast_term} should have NBLAST matches") + def test_get_similar_morphology_part_of(self): """Test get_similar_morphology_part_of (NBLASTexp)""" - result = get_similar_morphology_part_of(self.neuron_term, return_dataframe=True, limit=5) - self.assertIsNotNone(result) - import pandas as pd - if isinstance(result, pd.DataFrame) and len(result) > 0: - print(f"\n✓ Found {len(result)} NBLASTexp matches for {self.neuron_term}") - + result = get_similar_morphology_part_of(self.nblastexp_term, return_dataframe=True, limit=5) + self.assertIsInstance(result, pd.DataFrame) + self.assertFalse(result.empty, f"{self.nblastexp_term} should have NBLASTexp matches") + print(f"\n✓ Found {len(result)} NBLASTexp matches for {self.nblastexp_term}") + def test_get_similar_morphology_part_of_exp(self): """Test get_similar_morphology_part_of_exp (reverse NBLASTexp)""" - result = get_similar_morphology_part_of_exp(self.neuron_term, return_dataframe=True, limit=5) - self.assertIsNotNone(result) - import pandas as pd - if isinstance(result, pd.DataFrame) and len(result) > 0: - print(f"\n✓ Found {len(result)} reverse NBLASTexp matches") - + result = get_similar_morphology_part_of_exp(self.exp_term, return_dataframe=True, limit=5) + self.assertIsInstance(result, pd.DataFrame) + self.assertFalse(result.empty, f"{self.exp_term} should have reverse-NBLASTexp matches") + print(f"\n✓ Found {len(result)} reverse NBLASTexp matches") + def test_get_similar_morphology_nb(self): """Test get_similar_morphology_nb (NeuronBridge)""" - result = get_similar_morphology_nb(self.neuron_term, return_dataframe=True, limit=5) - self.assertIsNotNone(result) - import pandas as pd - if isinstance(result, pd.DataFrame) and len(result) > 0: - print(f"\n✓ Found {len(result)} NeuronBridge matches") - self.assertIn('score', result.columns) - + result = get_similar_morphology_nb(self.nblast_term, return_dataframe=True, limit=5) + self.assertIsInstance(result, pd.DataFrame) + self.assertFalse(result.empty, f"{self.nblast_term} should have NeuronBridge matches") + print(f"\n✓ Found {len(result)} NeuronBridge matches") + self.assertIn('score', result.columns) + def test_get_similar_morphology_nb_exp(self): """Test get_similar_morphology_nb_exp (NeuronBridge for expression)""" - result = get_similar_morphology_nb_exp(self.neuron_term, return_dataframe=True, limit=5) - self.assertIsNotNone(result) + import pandas as pd + result = get_similar_morphology_nb_exp(self.exp_term, return_dataframe=True, limit=5) + self.assertIsInstance(result, pd.DataFrame) + self.assertFalse(result.empty, f"{self.exp_term} should have NeuronBridge-exp matches") def test_schema_functions_exist(self): """Test that all NBLAST schema functions exist and are callable""" diff --git a/src/test/test_neuron_classes_fasciculating.py b/src/test/test_neuron_classes_fasciculating.py index 477c62ff..4f48eb5a 100644 --- a/src/test/test_neuron_classes_fasciculating.py +++ b/src/test/test_neuron_classes_fasciculating.py @@ -46,16 +46,16 @@ def test_query_execution(self): self.assertIsNotNone(result, "Query should return a result") self.assertIsInstance(result, dict, "Result should be a dictionary") - # Check for expected keys - if result: - print(f"Query returned {len(result.get('data', []))} results") - - # Validate data structure - if 'data' in result and len(result['data']) > 0: - first_result = result['data'][0] - self.assertIn('id', first_result, "Result should contain 'id' field") - self.assertIn('label', first_result, "Result should contain 'label' field") - print(f"First result: {first_result.get('label', 'N/A')} ({first_result.get('id', 'N/A')})") + # FBbt_00003987 is innervated/fasciculated by neuron classes, so an empty + # result is a defect. (The old guard checked a 'data' key the query never + # returns — the key is 'rows' — so it passed vacuously regardless.) + rows = result.get('rows', []) + self.assertTrue(rows, f"{self.test_tract} should have fasciculating neuron classes") + print(f"Query returned {result.get('count', len(rows))} results") + first_result = rows[0] + self.assertIn('id', first_result, "Result should contain 'id' field") + self.assertIn('label', first_result, "Result should contain 'label' field") + print(f"First result: {first_result.get('label', 'N/A')} ({first_result.get('id', 'N/A')})") def test_schema_generation(self): """Test schema function generates correct structure""" @@ -159,19 +159,14 @@ def test_with_different_tracts(self): for tract_id, tract_name in test_tracts: print(f"\nTesting {tract_name} ({tract_id})...") - - try: - result = get_neuron_classes_fasciculating_here(tract_id, return_dataframe=False, limit=3) - - if result and 'data' in result: - print(f" ✓ Query successful, found {len(result['data'])} results") - else: - print(f" ✓ Query successful, no results found") - - except Exception as e: - print(f" ✗ Query failed: {str(e)}") - # Don't fail the test, just log the error - # raise + # No try/except swallow, and assert real content: each of these + # tracts has fasciculating neuron classes (counts 1 / 13 / 115), so + # an empty result is a defect. A backend outage is skipped upstream + # (conftest.py). The result key is 'rows', not 'data'. + result = get_neuron_classes_fasciculating_here(tract_id, return_dataframe=False, limit=3) + rows = result.get('rows', []) + self.assertTrue(rows, f"{tract_name} ({tract_id}) should have fasciculating neuron classes") + print(f" ✓ Query successful, found {result.get('count', len(rows))} results") def run_tests(): diff --git a/src/test/test_neuron_inputs.py b/src/test/test_neuron_inputs.py index 0fc90c86..14a375df 100644 --- a/src/test/test_neuron_inputs.py +++ b/src/test/test_neuron_inputs.py @@ -46,14 +46,15 @@ def test_query_execution(self): self.assertIsInstance(result, dict, "Result should be a dictionary") print(f"Query returned {result.get('count', 0)} total results") - if 'rows' in result and len(result['rows']) > 0: - first_result = result['rows'][0] - self.assertIn('id', first_result, "Result should contain 'id' field") - self.assertIn('Neurotransmitter', first_result, "Result should contain 'Neurotransmitter' field") - self.assertIn('Weight', first_result, "Result should contain 'Weight' field") - print(f"First result: {first_result.get('Neurotransmitter', 'N/A')} (weight: {first_result.get('Weight', 0)})") - else: - print("No input neurons found (this is OK if none exist)") + # VFB_jrchk00s (LPC1) is a known connectome neuron with inputs, so an + # empty result is a defect. A backend outage skips this upstream + # (conftest.py) rather than reaching here empty. + self.assertTrue(result.get('rows'), "Query for a neuron with known inputs returned no rows") + first_result = result['rows'][0] + self.assertIn('id', first_result, "Result should contain 'id' field") + self.assertIn('Neurotransmitter', first_result, "Result should contain 'Neurotransmitter' field") + self.assertIn('Weight', first_result, "Result should contain 'Weight' field") + print(f"First result: {first_result.get('Neurotransmitter', 'N/A')} (weight: {first_result.get('Weight', 0)})") def test_schema_generation(self): """Test that the schema function works correctly""" @@ -108,20 +109,18 @@ def test_preview_validation(self): limit=5 ) - if 'rows' in result and len(result['rows']) > 0: - # Check that all expected columns exist in the results - expected_columns = ['id', 'Neurotransmitter', 'Weight', 'Name'] - for item in result['rows']: - for col in expected_columns: - self.assertIn(col, item, f"Result should contain '{col}' field") - - print(f"✓ All {len(result['rows'])} results have required columns") - - # Print sample results - for i, item in enumerate(result['rows'][:3], 1): - print(f"{i}. {item.get('Name', 'N/A')} - {item.get('Neurotransmitter', 'N/A')} (weight: {item.get('Weight', 0)})") - else: - print("No preview data available (query returned no results)") + self.assertTrue(result.get('rows'), "Query for a neuron with known inputs returned no rows") + # Check that all expected columns exist in the results + expected_columns = ['id', 'Neurotransmitter', 'Weight', 'Name'] + for item in result['rows']: + for col in expected_columns: + self.assertIn(col, item, f"Result should contain '{col}' field") + + print(f"✓ All {len(result['rows'])} results have required columns") + + # Print sample results + for i, item in enumerate(result['rows'][:3], 1): + print(f"{i}. {item.get('Name', 'N/A')} - {item.get('Neurotransmitter', 'N/A')} (weight: {item.get('Weight', 0)})") def test_neurotransmitter_info(self): """Test that neurotransmitter information is included""" @@ -132,19 +131,17 @@ def test_neurotransmitter_info(self): limit=10 ) - if 'rows' in result and len(result['rows']) > 0: - # Check that neurotransmitter field exists and has values - neurotransmitters = set() - for row in result['rows']: - nt = row.get('Neurotransmitter', '') - if nt: - neurotransmitters.add(nt) - - print(f"✓ Found {len(neurotransmitters)} different neurotransmitter type(s)") - if neurotransmitters: - print(f" Types: {', '.join(list(neurotransmitters)[:5])}") - else: - print("No results to check neurotransmitter information") + self.assertTrue(result.get('rows'), "Query for a neuron with known inputs returned no rows") + # Check that neurotransmitter field exists and has values + neurotransmitters = set() + for row in result['rows']: + nt = row.get('Neurotransmitter', '') + if nt: + neurotransmitters.add(nt) + + print(f"✓ Found {len(neurotransmitters)} different neurotransmitter type(s)") + if neurotransmitters: + print(f" Types: {', '.join(list(neurotransmitters)[:5])}") def test_summary_mode(self): """Test that summary mode works correctly""" @@ -158,13 +155,11 @@ def test_summary_mode(self): self.assertIsNotNone(result, "Summary mode should return a result") self.assertIsInstance(result, dict, "Result should be a dictionary") - if 'rows' in result and len(result['rows']) > 0: - # In summary mode, results are grouped by neurotransmitter type - print(f"✓ Summary mode returned {len(result['rows'])} neurotransmitter types") - for i, item in enumerate(result['rows'][:3], 1): - print(f"{i}. {item.get('Neurotransmitter', 'N/A')} - Total weight: {item.get('Weight', 0)}") - else: - print("No summary data available") + self.assertTrue(result.get('rows'), "Summary mode for a neuron with known inputs returned no rows") + # In summary mode, results are grouped by neurotransmitter type + print(f"✓ Summary mode returned {len(result['rows'])} neurotransmitter types") + for i, item in enumerate(result['rows'][:3], 1): + print(f"{i}. {item.get('Neurotransmitter', 'N/A')} - Total weight: {item.get('Weight', 0)}") def test_dataframe_output(self): """Test that DataFrame output format works""" @@ -178,17 +173,14 @@ def test_dataframe_output(self): # Should return a pandas DataFrame import pandas as pd self.assertIsInstance(result, pd.DataFrame, "Should return DataFrame when return_dataframe=True") - - if not result.empty: - # Check for expected columns - expected_columns = ['id', 'Neurotransmitter', 'Weight'] - for col in expected_columns: - self.assertIn(col, result.columns, f"DataFrame should contain '{col}' column") - - print(f"✓ DataFrame has {len(result)} rows and {len(result.columns)} columns") - print(f" Columns: {list(result.columns)}") - else: - print("DataFrame is empty (no input neurons found)") + self.assertFalse(result.empty, "Query for a neuron with known inputs returned no rows") + # Check for expected columns + expected_columns = ['id', 'Neurotransmitter', 'Weight'] + for col in expected_columns: + self.assertIn(col, result.columns, f"DataFrame should contain '{col}' column") + + print(f"✓ DataFrame has {len(result)} rows and {len(result.columns)} columns") + print(f" Columns: {list(result.columns)}") if __name__ == '__main__': diff --git a/src/test/test_new_owlery_queries.py b/src/test/test_new_owlery_queries.py index bd6ef9b2..53c2ecc2 100644 --- a/src/test/test_new_owlery_queries.py +++ b/src/test/test_new_owlery_queries.py @@ -37,6 +37,9 @@ def test_neurons_synaptic_returns_results(self): self.assertIn('headers', result) self.assertIn('rows', result) self.assertIn('count', result) + # Known-populated term: an empty result is a defect (the empty branch + # still returns headers/rows/count, so key-presence alone passes vacuously). + self.assertGreater(result['count'], 0, "known-populated term should return results") def test_neurons_synaptic_has_expected_columns(self): """Test that result has expected column structure""" @@ -80,6 +83,9 @@ def test_neurons_presynaptic_returns_results(self): self.assertIn('headers', result) self.assertIn('rows', result) self.assertIn('count', result) + # Known-populated term: an empty result is a defect (the empty branch + # still returns headers/rows/count, so key-presence alone passes vacuously). + self.assertGreater(result['count'], 0, "known-populated term should return results") def test_neurons_presynaptic_has_expected_columns(self): """Test that result has expected column structure""" @@ -123,6 +129,9 @@ def test_neurons_postsynaptic_returns_results(self): self.assertIn('headers', result) self.assertIn('rows', result) self.assertIn('count', result) + # Known-populated term: an empty result is a defect (the empty branch + # still returns headers/rows/count, so key-presence alone passes vacuously). + self.assertGreater(result['count'], 0, "known-populated term should return results") def test_neurons_postsynaptic_has_expected_columns(self): """Test that result has expected column structure""" @@ -166,6 +175,9 @@ def test_components_of_returns_results(self): self.assertIn('headers', result) self.assertIn('rows', result) self.assertIn('count', result) + # Known-populated term: an empty result is a defect (the empty branch + # still returns headers/rows/count, so key-presence alone passes vacuously). + self.assertGreater(result['count'], 0, "known-populated term should return results") def test_components_of_has_expected_columns(self): """Test that result has expected column structure""" @@ -209,6 +221,9 @@ def test_parts_of_returns_results(self): self.assertIn('headers', result) self.assertIn('rows', result) self.assertIn('count', result) + # Known-populated term: an empty result is a defect (the empty branch + # still returns headers/rows/count, so key-presence alone passes vacuously). + self.assertGreater(result['count'], 0, "known-populated term should return results") def test_parts_of_has_expected_columns(self): """Test that result has expected column structure""" @@ -252,6 +267,9 @@ def test_subclasses_of_returns_results(self): self.assertIn('headers', result) self.assertIn('rows', result) self.assertIn('count', result) + # Known-populated term: an empty result is a defect (the empty branch + # still returns headers/rows/count, so key-presence alone passes vacuously). + self.assertGreater(result['count'], 0, "known-populated term should return results") def test_subclasses_of_has_expected_columns(self): """Test that result has expected column structure""" diff --git a/src/test/test_publication_transgene_queries.py b/src/test/test_publication_transgene_queries.py index bac121d6..866f521b 100644 --- a/src/test/test_publication_transgene_queries.py +++ b/src/test/test_publication_transgene_queries.py @@ -30,7 +30,10 @@ class PublicationTransgeneQueriesTest(unittest.TestCase): def setUp(self): """Set up test fixtures""" - self.pub_term = 'DOI_10_7554_eLife_04577' # Example publication + # FBrf0227179 = Aso et al. 2014, eLife 3:e04577 (86 terms). The previous + # value 'DOI_10_7554_eLife_04577' was not a node in the graph, so + # get_terms_for_pub returned 0 and the guards below passed vacuously. + self.pub_term = 'FBrf0227179' # a publication with referenced terms self.anatomy_term = 'FBbt_00003748' # mushroom body def test_get_terms_for_pub(self): @@ -39,38 +42,38 @@ def test_get_terms_for_pub(self): self.assertIsNotNone(result, "Result should not be None") import pandas as pd - if isinstance(result, pd.DataFrame) and len(result) > 0: - print(f"\n✓ Found {len(result)} terms for publication {self.pub_term}") - self.assertIn('id', result.columns) - self.assertIn('label', result.columns) + self.assertIsInstance(result, pd.DataFrame) + self.assertFalse(result.empty, f"{self.pub_term} should have referenced terms") + print(f"\n✓ Found {len(result)} terms for publication {self.pub_term}") + self.assertIn('id', result.columns) + self.assertIn('name', result.columns) def test_get_terms_for_pub_formatted(self): """Test get_terms_for_pub with formatted output""" result = get_terms_for_pub(self.pub_term, return_dataframe=False, limit=5) - self.assertIsNotNone(result) - - if isinstance(result, dict): - self.assertIn('headers', result) - self.assertIn('rows', result) - + self.assertIsInstance(result, dict) + self.assertIn('headers', result) + self.assertIn('rows', result) + self.assertTrue(result['rows'], f"{self.pub_term} should have referenced terms") + def test_get_transgene_expression_here(self): """Test get_transgene_expression_here query""" result = get_transgene_expression_here(self.anatomy_term, return_dataframe=True, limit=10) self.assertIsNotNone(result, "Result should not be None") - + import pandas as pd - if isinstance(result, pd.DataFrame) and len(result) > 0: - print(f"\n✓ Found {len(result)} transgene expressions in {self.anatomy_term}") - self.assertIn('id', result.columns) - + self.assertIsInstance(result, pd.DataFrame) + self.assertFalse(result.empty, f"{self.anatomy_term} should have transgene expression") + print(f"\n✓ Found {len(result)} transgene expressions in {self.anatomy_term}") + self.assertIn('id', result.columns) + def test_get_transgene_expression_formatted(self): """Test get_transgene_expression_here with formatted output""" result = get_transgene_expression_here(self.anatomy_term, return_dataframe=False, limit=5) - self.assertIsNotNone(result) - - if isinstance(result, dict): - self.assertIn('headers', result) - self.assertIn('rows', result) + self.assertIsInstance(result, dict) + self.assertIn('headers', result) + self.assertIn('rows', result) + self.assertTrue(result['rows'], f"{self.anatomy_term} should have transgene expression") def test_schema_functions_exist(self): """Test that publication/transgene schema functions exist and are callable""" @@ -85,10 +88,11 @@ def test_schema_functions_exist(self): def test_limit_parameter(self): """Test that limit parameter works correctly""" result = get_terms_for_pub(self.pub_term, return_dataframe=True, limit=3) - + import pandas as pd - if isinstance(result, pd.DataFrame) and len(result) > 0: - self.assertLessEqual(len(result), 3, "Result should respect limit parameter") + self.assertIsInstance(result, pd.DataFrame) + self.assertFalse(result.empty, f"{self.pub_term} should have referenced terms") + self.assertLessEqual(len(result), 3, "Result should respect limit parameter") def test_empty_results_handling(self): """Test that queries handle empty results gracefully""" diff --git a/src/test/test_query_count_rows_consistency.py b/src/test/test_query_count_rows_consistency.py new file mode 100644 index 00000000..3287c906 --- /dev/null +++ b/src/test/test_query_count_rows_consistency.py @@ -0,0 +1,79 @@ +"""Regression tests: query `count` must match the rows returned at limit=-1. + +These guard against the enrichment-CALL row-drop bug fixed alongside the +expression-pattern stock work: a thumbnail/image `CALL {}` subquery that ended +in `WHERE i IS NOT NULL` silently eliminated every result row with no aligned +image, so the table under-reported (or, for AnatomyExpressedIn, emptied out) +while `count` — computed by a separate, image-agnostic query — stayed correct. + +The pre-existing per-query tests missed this because they guard their +assertions behind `if not result.empty:` / `if rows:`, so a wrongly-empty +result skips every assertion and passes. Here we assert the opposite for known- +populated example terms deliberately chosen to have FEW results, so `limit=-1` +(which forces every row through the per-row enrichment CALLs) stays fast: + + - count > 0 and len(rows) > 0 (the term really does have data) + - count == len(rows) (no counted item was dropped from the table) + +A backend/connection failure skips rather than fails — handled centrally by +conftest.py — so these don't turn into false negatives without a live VFB +backend. An empty result while the backend IS reachable is a real failure. +""" + +import unittest +import sys +import os + +sys.path.insert(0, os.path.join(os.path.dirname(__file__), '..')) + +from vfbquery.vfb_queries import ( + get_transgene_expression_here, + get_expression_overlaps_here, + get_aligned_datasets, +) + + +class QueryCountRowsConsistencyTest(unittest.TestCase): + """Every counted item must appear as a row when nothing is limited.""" + + def _assert_count_equals_rows(self, fn, term, query_name): + # No try/except -> skipTest here: conftest.py turns a backend outage into + # a skip, and a real error must surface. An empty result for these + # known-populated terms is a defect, not a reason to skip. + result = fn(term, return_dataframe=False, limit=-1) + self.assertIsNotNone(result, f"{query_name}: no result for {term}") + + count = result.get('count', 0) + rows = result.get('rows', []) + self.assertGreater(count, 0, f"{query_name}: expected a non-zero count for {term}") + self.assertGreater(len(rows), 0, f"{query_name}: expected rows for {term}") + # A count that exceeds the rows returned at limit=-1 is the signature of + # an enrichment CALL dropping rows (the `WHERE i IS NOT NULL` bug). + self.assertEqual( + count, len(rows), + f"{query_name}: count ({count}) != rows returned ({len(rows)}) at " + f"limit=-1 for {term} — rows are being dropped after counting", + ) + + def test_transgene_expression_here_count_matches_rows(self): + # FBbt_00100253 (alpha'/beta' middle Kenyon cell): 11 expression + # patterns, 3 of them image-less splits that the bug dropped (11 -> 7). + self._assert_count_equals_rows( + get_transgene_expression_here, 'FBbt_00100253', 'TransgeneExpressionHere') + + def test_anatomy_expressed_in_count_matches_rows(self): + # A split with 2 overlapping anatomy classes. Anatomy classes rarely + # have the image path, so the bug emptied this table (2 -> 0). + self._assert_count_equals_rows( + get_expression_overlaps_here, 'VFBexp_FBtp0122383FBtp0119521', 'AnatomyExpressedIn') + + def test_aligned_datasets_count_matches_rows(self): + # VFB_00101384 (JRC_FlyEM_Hemibrain): 2 aligned datasets. Same + # enrichment-CALL shape; here the count path already requires an image, + # so this mainly locks the invariant against future regressions. + self._assert_count_equals_rows( + get_aligned_datasets, 'VFB_00101384', 'AlignedDatasets') + + +if __name__ == "__main__": + unittest.main(verbosity=2) diff --git a/src/test/test_query_performance.py b/src/test/test_query_performance.py index 2384b7cf..3a72021e 100644 --- a/src/test/test_query_performance.py +++ b/src/test/test_query_performance.py @@ -75,28 +75,47 @@ def setUp(self): self.results = [] - def _time_query(self, query_name, query_func, *args, **kwargs): - """Helper to time a query execution""" + @staticmethod + def _result_size(result): + """Row count for the various result shapes these queries return.""" + import pandas as pd + if isinstance(result, pd.DataFrame): + return len(result) + if isinstance(result, dict): + if 'rows' in result: + return len(result['rows']) + if 'count' in result: + return result['count'] + return 1 if result else 0 # e.g. a term_info dict + if isinstance(result, list): + return len(result) + return 1 if result is not None else 0 + + def _time_query(self, query_name, query_func, *args, allow_empty=False, **kwargs): + """Time a query AND assert it actually did its job. + + Exceptions are NOT swallowed: a backend outage is turned into a skip + upstream by conftest.py, and any other error fails the test — the old + try/except turned both into a silent success. Unless allow_empty=True, + the result must be non-empty, because a timing check alone let a query + that returned 0 rows (or None) pass. + """ start_time = time.time() - try: - result = query_func(*args, **kwargs) - duration = time.time() - start_time - success = result is not None - error = None - except Exception as e: - duration = time.time() - start_time - success = False - result = None - error = str(e) - + result = query_func(*args, **kwargs) + duration = time.time() - start_time + + self.assertIsNotNone(result, f"{query_name} returned None") + if not allow_empty: + self.assertGreater(self._result_size(result), 0, + f"{query_name} returned no rows") + self.results.append({ 'name': query_name, 'duration': duration, - 'success': success, - 'error': error + 'success': True, + 'error': None, }) - - return result, duration, success + return result, duration, True def test_01_term_info_queries(self): """Test term info query performance""" @@ -221,7 +240,8 @@ def test_04_anatomy_hierarchy_queries(self): result, duration, success = self._time_query( "SubclassesOf", get_subclasses_of, - test_term, + "FBbt_00048516", # wedge projection neuron has ~56 subclasses + # (mushroom body / test_term is a leaf with none, which returned 0) return_dataframe=False, limit=-1 # Enable caching for performance tests ) @@ -302,6 +322,9 @@ def test_05b_image_queries(self): "epFrag", get_expression_pattern_fragments, "FBtp0000001", # expression pattern example + allow_empty=True, # epFrag goes through the Owlery /instances endpoint, + # which is unreliable/slow for all inputs (see test_expression_pattern_ + # fragments.py) — this is a timing probe only, not a content check. return_dataframe=False, limit=-1 # Enable caching for performance tests ) @@ -527,57 +550,46 @@ def test_11_transcriptomics_queries(self): print(f" └─ Found {count} total clusters" + (", returned 10" if count > 10 else "")) self.assertLess(duration, self.THRESHOLD_SLOW, "anatScRNAseqQuery exceeded threshold") - # clusterExpression - test with a cluster ID (may return empty if cluster doesn't exist) - # Using a dummy ID - test will pass even with empty results - try: - result, duration, success = self._time_query( - "clusterExpression (example cluster)", - get_cluster_expression, - "FBlc0005370", # Example cluster ID - return_dataframe=False, - limit=10 - ) - print(f"clusterExpression: {duration:.4f}s {'✅' if success else '❌'}") - if success and result: - count = result.get('count', 0) - print(f" └─ Found {count} genes expressed" + (", returned 10" if count > 10 else "")) - self.assertLess(duration, self.THRESHOLD_SLOW, "clusterExpression exceeded threshold") - except Exception as e: - print(f"clusterExpression: Skipped (test data may not exist): {e}") - - # expressionCluster - test with a gene ID (may return empty if no scRNAseq data) - try: - result, duration, success = self._time_query( - "expressionCluster (example gene)", - get_expression_cluster, - "FBgn0034223", # Example gene ID - return_dataframe=False, - limit=10 - ) - print(f"expressionCluster: {duration:.4f}s {'✅' if success else '❌'}") - if success and result: - count = result.get('count', 0) - print(f" └─ Found {count} clusters expressing gene" + (", returned 10" if count > 10 else "")) - self.assertLess(duration, self.THRESHOLD_SLOW, "expressionCluster exceeded threshold") - except Exception as e: - print(f"expressionCluster: Skipped (test data may not exist): {e}") - - # scRNAdatasetData - test with a dataset ID (may return empty if dataset doesn't exist) - try: - result, duration, success = self._time_query( - "scRNAdatasetData (example dataset)", - get_scrnaseq_dataset_data, - "FBlc0005362", # Example dataset ID - return_dataframe=False, - limit=10 - ) - print(f"scRNAdatasetData: {duration:.4f}s {'✅' if success else '❌'}") - if success and result: - count = result.get('count', 0) - print(f" └─ Found {count} clusters in dataset" + (", returned 10" if count > 10 else "")) - self.assertLess(duration, self.THRESHOLD_SLOW, "scRNAdatasetData exceeded threshold") - except Exception as e: - print(f"scRNAdatasetData: Skipped (test data may not exist): {e}") + # clusterExpression - these FBlc/FBgn ids are real and return data + # (4588 / 9 / 13); no try/except-Skipped swallow, so a real failure or an + # empty result now fails (a backend outage is skipped upstream). + result, duration, success = self._time_query( + "clusterExpression (example cluster)", + get_cluster_expression, + "FBlc0005370", # cluster with expression data + return_dataframe=False, + limit=10 + ) + print(f"clusterExpression: {duration:.4f}s {'✅' if success else '❌'}") + count = result.get('count', 0) + print(f" └─ Found {count} genes expressed" + (", returned 10" if count > 10 else "")) + self.assertLess(duration, self.THRESHOLD_SLOW, "clusterExpression exceeded threshold") + + # expressionCluster + result, duration, success = self._time_query( + "expressionCluster (example gene)", + get_expression_cluster, + "FBgn0034223", # gene with scRNAseq expression clusters + return_dataframe=False, + limit=10 + ) + print(f"expressionCluster: {duration:.4f}s {'✅' if success else '❌'}") + count = result.get('count', 0) + print(f" └─ Found {count} clusters expressing gene" + (", returned 10" if count > 10 else "")) + self.assertLess(duration, self.THRESHOLD_SLOW, "expressionCluster exceeded threshold") + + # scRNAdatasetData + result, duration, success = self._time_query( + "scRNAdatasetData (example dataset)", + get_scrnaseq_dataset_data, + "FBlc0005362", # scRNAseq dataset with clusters + return_dataframe=False, + limit=10 + ) + print(f"scRNAdatasetData: {duration:.4f}s {'✅' if success else '❌'}") + count = result.get('count', 0) + print(f" └─ Found {count} clusters in dataset" + (", returned 10" if count > 10 else "")) + self.assertLess(duration, self.THRESHOLD_SLOW, "scRNAdatasetData exceeded threshold") def test_12_nblast_queries(self): """Test NBLAST similarity queries""" @@ -620,7 +632,7 @@ def test_12_nblast_queries(self): result, duration, success = self._time_query( "SimilarMorphologyToPartOf", get_similar_morphology_part_of, - "VFB_jrchjwmw", + "VFB_00016103", # neuron with NBLASTexp matches (VFB_jrchjwmw had none) return_dataframe=False, limit=10 ) @@ -634,7 +646,7 @@ def test_12_nblast_queries(self): result, duration, success = self._time_query( "SimilarMorphologyToPartOfexp", get_similar_morphology_part_of_exp, - "VFB_jrchjwmw", + "VFB_001012yj", # expression pattern with reverse-NBLASTexp matches return_dataframe=False, limit=10 ) diff --git a/src/test/test_similar_morphology.py b/src/test/test_similar_morphology.py index 9ebbc1f9..1e1efef5 100644 --- a/src/test/test_similar_morphology.py +++ b/src/test/test_similar_morphology.py @@ -48,14 +48,15 @@ def test_query_execution(self): self.assertIsInstance(result, dict, "Result should be a dictionary") print(f"Query returned {result.get('count', 0)} total results") - if 'rows' in result and len(result['rows']) > 0: - first_result = result['rows'][0] - self.assertIn('id', first_result, "Result should contain 'id' field") - self.assertIn('name', first_result, "Result should contain 'name' field") - self.assertIn('score', first_result, "Result should contain 'score' field") - print(f"First result: {first_result.get('name', 'N/A')} (score: {first_result.get('score', 0)})") - else: - print("No similar neurons found (this is OK if none exist)") + # VFB_jrchk00s (LPC1) is documented as having NBLAST data, so an empty + # result is a defect. A backend outage skips this upstream (conftest.py) + # rather than reaching here empty. + self.assertTrue(result.get('rows'), "Query for a neuron with known NBLAST data returned no rows") + first_result = result['rows'][0] + self.assertIn('id', first_result, "Result should contain 'id' field") + self.assertIn('name', first_result, "Result should contain 'name' field") + self.assertIn('score', first_result, "Result should contain 'score' field") + print(f"First result: {first_result.get('name', 'N/A')} (score: {first_result.get('score', 0)})") def test_schema_generation(self): """Test that the schema function works correctly""" @@ -107,20 +108,18 @@ def test_preview_validation(self): limit=5 ) - if 'rows' in result and len(result['rows']) > 0: - # Check that all preview columns exist in the results - expected_columns = ['id', 'name', 'score', 'tags'] - for item in result['rows']: - for col in expected_columns: - self.assertIn(col, item, f"Result should contain '{col}' field") - - print(f"✓ All {len(result['rows'])} results have required preview columns") - - # Print sample results - for i, item in enumerate(result['rows'][:3], 1): - print(f"{i}. {item.get('name', 'N/A')} - Score: {item.get('score', 0)}") - else: - print("No preview data available (query returned no results)") + self.assertTrue(result.get('rows'), "Query for a neuron with known NBLAST data returned no rows") + # Check that all preview columns exist in the results + expected_columns = ['id', 'name', 'score', 'tags'] + for item in result['rows']: + for col in expected_columns: + self.assertIn(col, item, f"Result should contain '{col}' field") + + print(f"✓ All {len(result['rows'])} results have required preview columns") + + # Print sample results + for i, item in enumerate(result['rows'][:3], 1): + print(f"{i}. {item.get('name', 'N/A')} - Score: {item.get('score', 0)}") def test_score_ordering(self): """Test that results are ordered by score descending""" @@ -132,20 +131,19 @@ def test_score_ordering(self): limit=10 ) - if 'rows' in result and len(result['rows']) > 1: - scores = [float(row.get('score', 0)) for row in result['rows']] - # Check that scores are in descending order - for i in range(len(scores) - 1): - self.assertGreaterEqual( - scores[i], - scores[i + 1], - f"Scores should be in descending order: {scores[i]} >= {scores[i+1]}" - ) - print(f"✓ Scores are properly ordered (descending)") - print(f" Highest score: {scores[0]}") - print(f" Lowest score: {scores[-1]}") - else: - print("Not enough results to test ordering") + self.assertTrue(result.get('rows'), "Query for a neuron with known NBLAST data returned no rows") + scores = [float(row.get('score', 0)) for row in result['rows']] + # Check that scores are in descending order (a single-row result trivially + # satisfies this; the query for this neuron returns many). + for i in range(len(scores) - 1): + self.assertGreaterEqual( + scores[i], + scores[i + 1], + f"Scores should be in descending order: {scores[i]} >= {scores[i+1]}" + ) + print(f"✓ Scores are properly ordered (descending)") + print(f" Highest score: {scores[0]}") + print(f" Lowest score: {scores[-1]}") def test_dataframe_output(self): """Test that DataFrame output format works""" @@ -160,17 +158,14 @@ def test_dataframe_output(self): # Should return a pandas DataFrame import pandas as pd self.assertIsInstance(result, pd.DataFrame, "Should return DataFrame when return_dataframe=True") - - if not result.empty: - # Check for expected columns - expected_columns = ['id', 'name', 'score', 'tags'] - for col in expected_columns: - self.assertIn(col, result.columns, f"DataFrame should contain '{col}' column") - - print(f"✓ DataFrame has {len(result)} rows and {len(result.columns)} columns") - print(f" Columns: {list(result.columns)}") - else: - print("DataFrame is empty (no similar neurons found)") + self.assertFalse(result.empty, "Query for a neuron with known NBLAST data returned no rows") + # Check for expected columns + expected_columns = ['id', 'name', 'score', 'tags'] + for col in expected_columns: + self.assertIn(col, result.columns, f"DataFrame should contain '{col}' column") + + print(f"✓ DataFrame has {len(result)} rows and {len(result.columns)} columns") + print(f" Columns: {list(result.columns)}") if __name__ == '__main__': diff --git a/src/test/test_solr_cache_failover.py b/src/test/test_solr_cache_failover.py index 0972def1..99ecdaef 100644 --- a/src/test/test_solr_cache_failover.py +++ b/src/test/test_solr_cache_failover.py @@ -43,12 +43,21 @@ def test_disable_and_reenable_on_solr_failure(self): def test_cache_invalidated_on_major_version_change(self): cache = SolrResultCache() cache._solr_available = MagicMock(return_value=True) - cache._package_version = "1.8.1" + # current 2.0.0 vs cached 1.8.0 is a genuine major.minor change (the old + # 1.8.1-vs-1.8.0 both normalise to 1.8 — a patch change that is correctly + # RETAINED, so this test only "passed" via the swallowed-TypeError bug). + cache._package_version = "2.0.0" cached_data = { "result": {"foo": "bar"}, - "cached_at": datetime.now().isoformat(), - "expires_at": (datetime.now() + timedelta(hours=1)).isoformat(), + # Timezone-AWARE, matching what the cache actually stores + # (solr_result_cache stores datetime.now().astimezone()). A naive + # timestamp here made get_cached_result raise TypeError comparing + # naive vs aware, which was swallowed to None — silently breaking the + # "cache hit" tests (and letting the invalidation tests pass for the + # wrong reason). + "cached_at": datetime.now().astimezone().isoformat(), + "expires_at": (datetime.now().astimezone() + timedelta(hours=1)).isoformat(), "params": {"limit": -1}, "hit_count": 0, "cache_version": "1.0", @@ -71,8 +80,14 @@ def test_cache_retained_on_patch_version_change(self): cached_data = { "result": {"foo": "bar"}, - "cached_at": datetime.now().isoformat(), - "expires_at": (datetime.now() + timedelta(hours=1)).isoformat(), + # Timezone-AWARE, matching what the cache actually stores + # (solr_result_cache stores datetime.now().astimezone()). A naive + # timestamp here made get_cached_result raise TypeError comparing + # naive vs aware, which was swallowed to None — silently breaking the + # "cache hit" tests (and letting the invalidation tests pass for the + # wrong reason). + "cached_at": datetime.now().astimezone().isoformat(), + "expires_at": (datetime.now().astimezone() + timedelta(hours=1)).isoformat(), "params": {"limit": -1}, "hit_count": 0, "cache_version": "1.0", @@ -95,8 +110,14 @@ def test_cache_invalidated_when_cached_version_missing(self): cached_data = { "result": {"foo": "bar"}, - "cached_at": datetime.now().isoformat(), - "expires_at": (datetime.now() + timedelta(hours=1)).isoformat(), + # Timezone-AWARE, matching what the cache actually stores + # (solr_result_cache stores datetime.now().astimezone()). A naive + # timestamp here made get_cached_result raise TypeError comparing + # naive vs aware, which was swallowed to None — silently breaking the + # "cache hit" tests (and letting the invalidation tests pass for the + # wrong reason). + "cached_at": datetime.now().astimezone().isoformat(), + "expires_at": (datetime.now().astimezone() + timedelta(hours=1)).isoformat(), "params": {"limit": -1}, "hit_count": 0, "cache_version": "1.0", diff --git a/src/test/test_tracts_nerves_innervating.py b/src/test/test_tracts_nerves_innervating.py index 789a5311..eb104561 100644 --- a/src/test/test_tracts_nerves_innervating.py +++ b/src/test/test_tracts_nerves_innervating.py @@ -46,16 +46,16 @@ def test_query_execution(self): self.assertIsNotNone(result, "Query should return a result") self.assertIsInstance(result, dict, "Result should be a dictionary") - # Check for expected keys - if result: - print(f"Query returned {len(result.get('data', []))} results") - - # Validate data structure - if 'data' in result and len(result['data']) > 0: - first_result = result['data'][0] - self.assertIn('id', first_result, "Result should contain 'id' field") - self.assertIn('label', first_result, "Result should contain 'label' field") - print(f"First result: {first_result.get('label', 'N/A')} ({first_result.get('id', 'N/A')})") + # FBbt_00007401 (antennal lobe) is innervated by tracts/nerves, so an + # empty result is a defect. (The old guard checked a 'data' key the query + # never returns — the key is 'rows' — so it passed vacuously regardless.) + rows = result.get('rows', []) + self.assertTrue(rows, "antennal lobe should have innervating tracts/nerves") + print(f"Query returned {result.get('count', len(rows))} results") + first_result = rows[0] + self.assertIn('id', first_result, "Result should contain 'id' field") + self.assertIn('label', first_result, "Result should contain 'label' field") + print(f"First result: {first_result.get('label', 'N/A')} ({first_result.get('id', 'N/A')})") def test_schema_generation(self): """Test schema function generates correct structure""" @@ -74,7 +74,7 @@ def test_schema_generation(self): self.assertEqual(schema.preview, 5, "Preview should be 5") # Check preview columns - expected_columns = ["id", "label", "tags", "thumbnail"] + expected_columns = ["id", "label", "tags", "template", "technique", "thumbnail"] self.assertEqual(schema.preview_columns, expected_columns, f"Preview columns should be {expected_columns}") print(f"Schema generated successfully: {schema.label}") diff --git a/src/test/test_transcriptomics.py b/src/test/test_transcriptomics.py index 55543046..3a2d22ed 100644 --- a/src/test/test_transcriptomics.py +++ b/src/test/test_transcriptomics.py @@ -20,29 +20,30 @@ class TranscriptomicsQueriesTest(unittest.TestCase): # Test data - known terms with scRNAseq data # These are examples from the VFB knowledge base - ANATOMY_WITH_SCRNASEQ = "FBbt_00003982" # adult brain - should have scRNAseq data - CLUSTER_ID = "VFBc_00101567" # Example cluster ID (may need to be updated with real data) - GENE_ID = "FBgn_00000024" # Example gene ID (may need to be updated with real data) - DATASET_ID = "VFBds_00001234" # Example dataset ID (may need to be updated with real data) + # Real fixtures (the previous values were placeholders / a Channel node, so + # every query returned 0 or raised, and the guards below passed vacuously or + # skipped). Verified counts: anatomy 3, cluster 2647, gene 3290, dataset 555. + ANATOMY_WITH_SCRNASEQ = "FBbt_00100163" # a cell type with scRNAseq clusters + CLUSTER_ID = "FBlc0006181" # an scRNAseq cluster + GENE_ID = "FBgn0283521" # a gene expressed across clusters + DATASET_ID = "FBlc0004785" # an scRNAseq dataset def test_anatomy_scrnaseq_basic_dataframe(self): """Test anatScRNAseqQuery returns DataFrame""" result = vfb.get_anatomy_scrnaseq(self.ANATOMY_WITH_SCRNASEQ, return_dataframe=True) self.assertIsInstance(result, pd.DataFrame) - - # If data exists, check structure - if not result.empty: - self.assertIn('id', result.columns) - self.assertIn('name', result.columns) - self.assertIn('tags', result.columns) - self.assertIn('dataset', result.columns) - self.assertIn('pubs', result.columns) - - # Check that all IDs start with expected prefix - for idx, row in result.iterrows(): - self.assertTrue(row['id'].startswith('VFB'), - f"Cluster ID should start with VFB, got: {row['id']}") + self.assertFalse(result.empty, f"{self.ANATOMY_WITH_SCRNASEQ} should have scRNAseq clusters") + self.assertIn('id', result.columns) + self.assertIn('name', result.columns) + self.assertIn('tags', result.columns) + self.assertIn('dataset', result.columns) + self.assertIn('pubs', result.columns) + + # Cluster IDs are FlyBase library-cluster ids (FBlc...). + for idx, row in result.iterrows(): + self.assertTrue(row['id'].startswith('FBlc'), + f"Cluster ID should start with FBlc, got: {row['id']}") def test_anatomy_scrnaseq_formatted_output(self): """Test anatScRNAseqQuery returns properly formatted dict""" @@ -64,120 +65,95 @@ def test_anatomy_scrnaseq_formatted_output(self): def test_anatomy_scrnaseq_limit(self): """Test anatScRNAseqQuery respects limit parameter""" result = vfb.get_anatomy_scrnaseq(self.ANATOMY_WITH_SCRNASEQ, return_dataframe=True, limit=5) - + self.assertIsInstance(result, pd.DataFrame) - if not result.empty: - self.assertLessEqual(len(result), 5) + self.assertFalse(result.empty, f"{self.ANATOMY_WITH_SCRNASEQ} should have scRNAseq clusters") + self.assertLessEqual(len(result), 5) def test_cluster_expression_basic_dataframe(self): """Test clusterExpression returns DataFrame""" - # Note: This test may need adjustment based on actual cluster IDs in the database - # For now, we'll just test that the function runs without error - try: - result = vfb.get_cluster_expression(self.CLUSTER_ID, return_dataframe=True) - self.assertIsInstance(result, pd.DataFrame) - - # If data exists, check structure - if not result.empty: - self.assertIn('id', result.columns) - self.assertIn('name', result.columns) - self.assertIn('tags', result.columns) - self.assertIn('expression_level', result.columns) - self.assertIn('expression_extent', result.columns) - self.assertIn('anatomy', result.columns) - except Exception as e: - # Skip test if cluster ID doesn't exist in current database - self.skipTest(f"Cluster ID {self.CLUSTER_ID} may not exist in database: {e}") + # No try/except -> skipTest here: with a real fixture, a raised error is + # a genuine problem (a backend outage is skipped upstream by conftest.py). + result = vfb.get_cluster_expression(self.CLUSTER_ID, return_dataframe=True) + self.assertIsInstance(result, pd.DataFrame) + self.assertFalse(result.empty, f"{self.CLUSTER_ID} should have expression data") + self.assertIn('id', result.columns) + self.assertIn('name', result.columns) + self.assertIn('tags', result.columns) + self.assertIn('expression_level', result.columns) + self.assertIn('expression_extent', result.columns) + self.assertIn('anatomy', result.columns) def test_cluster_expression_formatted_output(self): """Test clusterExpression returns properly formatted dict""" - try: - result = vfb.get_cluster_expression(self.CLUSTER_ID, return_dataframe=False) - - self.assertIsInstance(result, dict) - self.assertIn('headers', result) - self.assertIn('rows', result) - self.assertIn('count', result) - - # Check headers structure - headers = result['headers'] - self.assertIn('id', headers) - self.assertIn('name', headers) - self.assertIn('expression_level', headers) - self.assertIn('expression_extent', headers) - except Exception as e: - self.skipTest(f"Cluster ID {self.CLUSTER_ID} may not exist in database: {e}") + result = vfb.get_cluster_expression(self.CLUSTER_ID, return_dataframe=False) + self.assertIsInstance(result, dict) + self.assertIn('headers', result) + self.assertIn('rows', result) + self.assertIn('count', result) + self.assertTrue(result['rows'], f"{self.CLUSTER_ID} should have expression data") + + # Check headers structure + headers = result['headers'] + self.assertIn('id', headers) + self.assertIn('name', headers) + self.assertIn('expression_level', headers) + self.assertIn('expression_extent', headers) def test_expression_cluster_basic_dataframe(self): """Test expressionCluster returns DataFrame""" - try: - result = vfb.get_expression_cluster(self.GENE_ID, return_dataframe=True) - self.assertIsInstance(result, pd.DataFrame) - - # If data exists, check structure - if not result.empty: - self.assertIn('id', result.columns) - self.assertIn('name', result.columns) - self.assertIn('tags', result.columns) - self.assertIn('expression_level', result.columns) - self.assertIn('expression_extent', result.columns) - self.assertIn('anatomy', result.columns) - except Exception as e: - self.skipTest(f"Gene ID {self.GENE_ID} may not exist in database: {e}") + result = vfb.get_expression_cluster(self.GENE_ID, return_dataframe=True) + self.assertIsInstance(result, pd.DataFrame) + self.assertFalse(result.empty, f"{self.GENE_ID} should have expression clusters") + self.assertIn('id', result.columns) + self.assertIn('name', result.columns) + self.assertIn('tags', result.columns) + self.assertIn('expression_level', result.columns) + self.assertIn('expression_extent', result.columns) + self.assertIn('anatomy', result.columns) def test_expression_cluster_formatted_output(self): """Test expressionCluster returns properly formatted dict""" - try: - result = vfb.get_expression_cluster(self.GENE_ID, return_dataframe=False) - - self.assertIsInstance(result, dict) - self.assertIn('headers', result) - self.assertIn('rows', result) - self.assertIn('count', result) - - # Check headers structure - headers = result['headers'] - self.assertIn('id', headers) - self.assertIn('name', headers) - self.assertIn('expression_level', headers) - self.assertIn('expression_extent', headers) - except Exception as e: - self.skipTest(f"Gene ID {self.GENE_ID} may not exist in database: {e}") + result = vfb.get_expression_cluster(self.GENE_ID, return_dataframe=False) + self.assertIsInstance(result, dict) + self.assertIn('headers', result) + self.assertIn('rows', result) + self.assertIn('count', result) + self.assertTrue(result['rows'], f"{self.GENE_ID} should have expression clusters") + + # Check headers structure + headers = result['headers'] + self.assertIn('id', headers) + self.assertIn('name', headers) + self.assertIn('expression_level', headers) + self.assertIn('expression_extent', headers) def test_scrnaseq_dataset_basic_dataframe(self): """Test scRNAdatasetData returns DataFrame""" - try: - result = vfb.get_scrnaseq_dataset_data(self.DATASET_ID, return_dataframe=True) - self.assertIsInstance(result, pd.DataFrame) - - # If data exists, check structure - if not result.empty: - self.assertIn('id', result.columns) - self.assertIn('name', result.columns) - self.assertIn('tags', result.columns) - self.assertIn('anatomy', result.columns) - self.assertIn('pubs', result.columns) - except Exception as e: - self.skipTest(f"Dataset ID {self.DATASET_ID} may not exist in database: {e}") + result = vfb.get_scrnaseq_dataset_data(self.DATASET_ID, return_dataframe=True) + self.assertIsInstance(result, pd.DataFrame) + self.assertFalse(result.empty, f"{self.DATASET_ID} should have scRNAseq data") + self.assertIn('id', result.columns) + self.assertIn('name', result.columns) + self.assertIn('tags', result.columns) + self.assertIn('anatomy', result.columns) + self.assertIn('pubs', result.columns) def test_scrnaseq_dataset_formatted_output(self): """Test scRNAdatasetData returns properly formatted dict""" - try: - result = vfb.get_scrnaseq_dataset_data(self.DATASET_ID, return_dataframe=False) - - self.assertIsInstance(result, dict) - self.assertIn('headers', result) - self.assertIn('rows', result) - self.assertIn('count', result) - - # Check headers structure - headers = result['headers'] - self.assertIn('id', headers) - self.assertIn('name', headers) - self.assertIn('anatomy', headers) - self.assertIn('pubs', headers) - except Exception as e: - self.skipTest(f"Dataset ID {self.DATASET_ID} may not exist in database: {e}") + result = vfb.get_scrnaseq_dataset_data(self.DATASET_ID, return_dataframe=False) + self.assertIsInstance(result, dict) + self.assertIn('headers', result) + self.assertIn('rows', result) + self.assertIn('count', result) + self.assertTrue(result['rows'], f"{self.DATASET_ID} should have scRNAseq data") + + # Check headers structure + headers = result['headers'] + self.assertIn('id', headers) + self.assertIn('name', headers) + self.assertIn('anatomy', headers) + self.assertIn('pubs', headers) def test_anatomy_scrnaseq_empty_result(self): """Test anatScRNAseqQuery with anatomy that has no scRNAseq data""" diff --git a/src/vfbquery/vfb_queries.py b/src/vfbquery/vfb_queries.py index f95ad1b4..849af439 100644 --- a/src/vfbquery/vfb_queries.py +++ b/src/vfbquery/vfb_queries.py @@ -1543,10 +1543,13 @@ def term_info_parse_object(results, short_form): termInfo["Publications"] = publications - # Add Synonyms for Class entities. pub_syn holds one entry per - # (synonym, pub); get_merged_synonyms() collapses these to one entry per - # synonym with the combined refs and drops the Unattributed placeholder. - if termInfo["SuperTypes"] and "Class" in termInfo["SuperTypes"] and vfbTerm.pub_syn and len(vfbTerm.pub_syn) > 0: + # Add Synonyms from pub_syn — for Classes AND Individuals. An image + # individual can carry a pub-attributed synonym (e.g. VFB_00101385 = + # "MEon JRC_FlyEM_Hemibrain"); gating this on "Class" dropped those. + # pub_syn holds one entry per (synonym, pub); get_merged_synonyms() + # collapses these to one entry per synonym with the combined refs and + # drops the Unattributed placeholder. + if vfbTerm.pub_syn and len(vfbTerm.pub_syn) > 0: synonyms = vfbTerm.get_merged_synonyms() # Only add the synonyms if we found any if synonyms: @@ -3755,8 +3758,12 @@ def get_expression_overlaps_here(expression_pattern_short_form: str, return_data WITH anat OPTIONAL MATCH (anat)<-[:has_source|SUBCLASSOF|INSTANCEOF*]-(i:Individual)<-[:depicts]-(channel:Individual)-[irw:in_register_with]->(template:Individual)-[:depicts]->(template_anat:Individual) OPTIONAL MATCH (channel)-[:is_specified_output_of]->(technique:Class) - WITH anat, i, template_anat, technique, irw - WHERE i IS NOT NULL + // Do NOT filter `i IS NOT NULL` here: this is a CALL subquery, so an + // input anat for which it yields no rows is dropped entirely. Anatomy + // classes rarely have this image path, so the filter emptied the whole + // table (count 79 -> 0 rows) and desynced it from count_query. The + // all-null row is stripped to '' by the REPLACE below, so the anat + // still returns a row (empty thumbnail/template/technique). WITH anat, i, template_anat, technique, irw LIMIT 5 WITH anat, collect({{i: i, template_anat: template_anat, technique: technique, irw: irw}}) AS imgs WITH anat, imgs, head(imgs) AS rep @@ -6597,8 +6604,11 @@ def _dataset_enrichment_cypher(ds_var: str = "ds") -> str: WITH {ds_var} OPTIONAL MATCH ({ds_var})<-[:has_source]-(i:Individual)<-[:depicts]-(channel:Individual)-[irw:in_register_with]->(:Template)-[:depicts]->(templ:Template) OPTIONAL MATCH (channel)-[:is_specified_output_of]->(technique:Class) - WITH {ds_var}, i, templ, technique, irw - WHERE i IS NOT NULL + // Do NOT filter `i IS NOT NULL` here: this is a CALL subquery, so a + // {ds_var} for which it yields no rows is dropped entirely. A dataset + // with no aligned image would vanish from the results (and desync + // from any count); the all-null row is stripped to '' by the REPLACE + // below, so the dataset still returns a row (empty thumbnail). WITH {ds_var}, i, templ, technique, irw LIMIT 5 WITH {ds_var}, collect({{i: i, templ: templ, technique: technique, irw: irw}}) AS imgs WITH {ds_var}, imgs, head(imgs) AS rep @@ -6678,7 +6688,23 @@ def get_aligned_datasets(template_short_form: str, return_dataframe=True, limit: Imaging_Technique, Images, Image_count). Closes the v2 parity gap flagged in projects/geppetto-vfbquery-migration/V2_V2DEV_PARITY_SWEEP.md. """ - count_query = f"MATCH (ds:DataSet:Individual) WHERE NOT ds:Deprecated AND (:Template:Individual {{short_form:'{template_short_form}'}})<-[:depicts]-(:Template:Individual)-[:in_register_with]-(:Individual)-[:depicts]->(:Individual)-[:has_source]->(ds) RETURN count(ds) AS count" + # Anchor the MATCH on the template (indexed short_form) and walk OUT to the + # datasets, rather than `MATCH (ds:DataSet) WHERE `. + # The old dataset-anchored form let the planner enumerate every DataSet and + # verify the path per dataset — which walks that dataset's whole image set, + # so cost scaled with the graph's total aligned-image volume (tens of + # thousands) regardless of the handful of datasets that actually match. The + # template-anchored form only touches images aligned to THIS template: + # measured 6.6s -> 0.6s (count) and 22.4s -> 0.9s (main) on the Hemibrain, + # same rows. `count(DISTINCT ds)` because the path now expands to one row + # per aligned image, not one existence-check per dataset. + template_datasets_match = ( + f"MATCH (:Template:Individual {{short_form:'{template_short_form}'}})" + "<-[:depicts]-(:Template:Individual)-[:in_register_with]-(:Individual)" + "-[:depicts]->(:Individual)-[:has_source]->(ds:DataSet:Individual) " + "WHERE NOT ds:Deprecated" + ) + count_query = f"{template_datasets_match} RETURN count(DISTINCT ds) AS count" count_results = vc.nc.commit_list([count_query]) total_count = get_dict_cursor()(count_results)[0]['count'] if count_results else 0 @@ -6688,7 +6714,7 @@ def get_aligned_datasets(template_short_form: str, return_dataframe=True, limit: # the limit only trims afterwards. That blew past the THRESHOLD_MEDIUM # (3 s) perf-test budget on CI. limit_clause = f"LIMIT {limit}" if limit != -1 else "" - main_query = f"""MATCH (ds:DataSet:Individual) WHERE NOT ds:Deprecated AND (:Template:Individual {{short_form:'{template_short_form}'}})<-[:depicts]-(:Template:Individual)-[:in_register_with]-(:Individual)-[:depicts]->(:Individual)-[:has_source]->(ds) + main_query = f"""{template_datasets_match} WITH DISTINCT ds ORDER BY coalesce(ds.label, ds.short_form) {limit_clause} @@ -6990,8 +7016,15 @@ def get_transgene_expression_here(anatomy_short_form: str, return_dataframe=True WITH ep OPTIONAL MATCH (ep)<-[:has_source|SUBCLASSOF|INSTANCEOF*]-(i:Individual)<-[:depicts]-(channel:Individual)-[irw:in_register_with]->(:Template)-[:depicts]->(templ:Template) OPTIONAL MATCH (channel)-[:is_specified_output_of]->(technique:Class) - WITH ep, i, templ, technique, irw - WHERE i IS NOT NULL + // Do NOT filter `i IS NOT NULL` here: this is a CALL subquery, so an + // input ep for which it yields no rows is dropped from the result + // entirely. Filtering image-less EPs desynced the table from + // count_query (which has no image filter) and silently hid every + // expression pattern with no aligned image — e.g. splits targeting a + // neuron that carry no registered image. When there is no image the + // OPTIONAL MATCH yields one all-null row; the null placeholders are + // stripped to '' by the REPLACE below, so the ep still returns a row + // (empty thumbnail/template/technique) and stays in the results. WITH ep, i, templ, technique, irw LIMIT 5 WITH ep, collect({{i: i, templ: templ, technique: technique, irw: irw}}) AS imgs WITH ep, imgs, head(imgs) AS rep diff --git a/tests/requirements.txt b/tests/requirements.txt new file mode 100644 index 00000000..b34fca20 --- /dev/null +++ b/tests/requirements.txt @@ -0,0 +1,40 @@ +# Test-only dependencies. +# +# The three dependency sets in this repo, and what each is for: +# +# setup.py `install_requires` what `pip install vfbquery` pulls in — the +# library's actual runtime imports, nothing else. +# requirements.txt the same runtime set, pinned for CI and the +# Docker image build (Dockerfile COPYs it). +# tests/requirements.txt this file — tooling that only ever runs under +# CI or a local test run. +# +# Nothing listed here is imported by anything under src/vfbquery, so keeping it +# out of requirements.txt keeps test tooling out of the published package and +# out of the runtime container image. Previously pytest and pytest-timeout sat +# in requirements.txt and pytest-xdist / deepdiff / colorama were `pip install`ed +# ad hoc inside individual workflow steps, so no single file described what a +# test run needs. +# +# Install alongside the runtime set: +# +# pip install -r requirements.txt -r tests/requirements.txt +# +# Covers both test trees: src/test (library query tests) and tests (API/preview +# endpoint tests). + +pytest + +# Enforces the 300 s per-test ceiling configured in pyproject.toml. Without it +# pytest warns about an unknown option and carries on unbounded, so a hung +# upstream call takes the whole job down with nothing naming the stuck test. +pytest-timeout + +# Provides -n / --dist for the parallel test runs in the "Run Tests" and +# "Performance Test" workflows. +pytest-xdist + +# src/test/test_examples_diff.py only — structural comparison of the outputs +# produced by the README examples, run by the "Test VFBquery Examples" workflow. +deepdiff +colorama