Add CI: GitHub Actions, SQLite fixture, ruff lint#20
Merged
Conversation
For SQLite configs, connectInfo['tap_schema'] is the FILE PATH (passed to ATTACH DATABASE), not the schema name. The schema name used in queries is held in connectInfo['tap_schema_file'] (set as ATTACH AS in tapquery.py). TableValidator and dataDictionary both used connectInfo['tap_schema'] as the schema name when constructing SQL like: SELECT table_name FROM <schema>.tables which produced "near "/": syntax error" against SQLite because the generated SQL embedded the file path. Surfaced by the new CI SQLite fixture; would have prevented any SQLite- backed production deployment from passing table validation. Oracle/ Postgres/MySQL behavior is unchanged — those configs don't set tap_schema_file, so the existing tap_schema lookup remains the fallback. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Phase 1+2 of the CI plan. Establishes a GitHub Actions gate that runs on every PR and on push to develop/main, replacing the manual KOA/NEID/NEA testing loop with automated checks. What runs in CI: - ruff (E, F, W, I) — gated; codebase passes with the minimal ignore set documented in pyproject.toml - pytest tests/ — matrix over Python 3.9 / 3.10 / 3.11 / 3.12 on Ubuntu - python -m build --wheel — Ubuntu + macOS, verifies the C extension builds cross-platform and the wheel is installable Test infrastructure: - tests/conftest.py spins up an HTTP server on a free port backed by a custom nph-aware request handler (stdlib's CGIHTTPRequestHandler prepends its own status line, breaking nph- scripts that emit their own HTTP/1.1 status lines like nph-tap.py does) - tests/fixtures/build_test_db.py builds tap_schema.db (IVOA-shape) and test_data.db (3 public + 3 expired + 4 still-embargoed L1 rows). The still-embargoed rows are the key fixture: production test servers (vmneidtest, koa-test) have all-expired propints, which means propflag enforcement can't be observed there. CI now has the missing scenario. - tests/fixtures/stubs/spatial_index.py is a minimal stub for the four module-level constants TAP imports. The real spatial_index package on PyPI only ships wheels for Python ≤ 3.8, so modern-Python CI cannot install it. Tracking as upstream issue. - tests/test_http_endpoint.py is a smoke layer that exercises the full HTTP→CGI→TAP→SQLite→IPAC pipeline. Security/regression tests land in the follow-up PR (Phase 3+4). Cosmetic ruff autofixes touched 13 files (whitespace, f-strings, import order) — no semantic changes. Deferred to follow-up PRs: - F821 ignored in ruff for now; 6 pre-existing undefined-name bugs to address in TAP/taputil.py (SQLite/MySQL connection branches) and TAP/tap.py:2402 (debug-log typo `result` → `results`) - spatial_index Python 3.9+ wheel situation upstream - Postgres/MySQL/Oracle CI matrix (SQLite-only for first cut) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…index The wheel-build verification and the test-job install both failed on the first CI run because pip can't resolve spatial_index on Python 3.9+: the PyPI package only ships wheels for 2.7 / 3.6 / 3.7 / 3.8 and has no source distribution. Workaround in CI: - Test job: install nexsciTAP and ADQL with --no-deps. The test fixture stub in tests/fixtures/stubs/spatial_index.py supplies the four module-level constants TAP uses at import time. configobj (the only other actually-needed runtime dep) is in requirements-test.txt. - Wheel-platforms job: drop the post-build pip install; instead verify the .so artifact is present in the wheel. The wheel itself builds fine — the issue is only at install-resolve time. This is purely a CI workaround. The deeper fix is upstream in spatial_index (publish modern wheels or an sdist). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Surfaced by the second CI run: TAP/tablenames.py imports sqlparse, TAP/datadictionary.py imports astropy.io.ascii, and other TAP modules import bs4 and xmltodict. None of these are declared in setup.py's install_requires (a pre-existing bug — declaring them in install_requires is a separate cleanup since the CI install path uses --no-deps anyway). Add them to requirements-test.txt so CI installs the actual runtime surface needed to run the tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 1 + 2 of the CI plan agreed on 2026-05-20. Replaces the manual KOA/NEID/NEA testing loop with an automated GitHub Actions gate.
What runs in CI
What's covered today
Test infrastructure highlights
Pre-existing bugs surfaced and fixed
Two real bugs in TableValidator and dataDictionary used `connectInfo['tap_schema']` as a schema name in SQL — but for SQLite that's actually the file path (passed to `ATTACH DATABASE`). The schema name for queries is held in `connectInfo['tap_schema_file']`. Fix prefers `tap_schema_file` when present; Oracle/Postgres/MySQL behavior is unchanged. SQLite-backed deployments were silently broken before this fix. Each is a one-line change, isolated in commit 4b35392.
Deferred to follow-up PRs
Verifying CI works as a gate
After merge, CI runs on every PR. To confirm it actually blocks regressions, the suggested smoke test is to open a throwaway PR that comments out the propflag guard at `TAP/tap.py:1582` and confirm the security-e2e tests in the follow-up PR turn red on it.
🤖 Generated with Claude Code