diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS new file mode 100644 index 000000000..67b450a4c --- /dev/null +++ b/.github/CODEOWNERS @@ -0,0 +1,8 @@ +# Rule machinery: changes need review from a core developer other than the +# author. Deliberately narrow: pyproject.toml and workflow files are not owned, +# so dependency bots and everyday CI edits are not blocked. +/rules/ @PolicyEngine/core-developers +/scripts/rules/ @PolicyEngine/core-developers +/scripts/guards/ @PolicyEngine/core-developers +/scripts/run_quality_guards.py @PolicyEngine/core-developers +/.github/CODEOWNERS @PolicyEngine/core-developers diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md new file mode 100644 index 000000000..daaae976d --- /dev/null +++ b/.github/pull_request_template.md @@ -0,0 +1,9 @@ +Fixes # + +## Summary + +- + +## Testing + +- diff --git a/.github/scripts/check_ci_complete.sh b/.github/scripts/check_ci_complete.sh new file mode 100644 index 000000000..bcfb70a92 --- /dev/null +++ b/.github/scripts/check_ci_complete.sh @@ -0,0 +1,39 @@ +#!/usr/bin/env bash + +# Gate behind the "CI complete" check in pr.yml. +# +# In a stack of pull requests, only the lowest unmerged one runs the full +# workflow; the jobs of the others are skipped to save runners. A skipped job +# counts as passing for a required check, so this gate fails on those pull +# requests instead. On every other pull request it passes only when every job +# it needs passed. +# +# Environment: +# JOB_RESULTS toJSON(needs) for the gate job +# PULL_REQUEST_BASE_REF github.event.pull_request.base.ref +# STACK_BASE_REF github.event.pull_request.stack.base.ref, empty when +# the pull request isn't in a stack + +set -euo pipefail + +job_results="${JOB_RESULTS:?JOB_RESULTS is required}" +pull_request_base_ref="${PULL_REQUEST_BASE_REF:?PULL_REQUEST_BASE_REF is required}" +stack_base_ref="${STACK_BASE_REF:-}" + +if [[ -n "${stack_base_ref}" && "${stack_base_ref}" != "${pull_request_base_ref}" ]]; then + echo "::error::Full CI runs only on the lowest unmerged pull request in a stack. This one targets ${pull_request_base_ref}; it runs once the pull request below it merges and this one is rebased onto ${stack_base_ref}." + exit 1 +fi + +unsuccessful_jobs="$( + jq -r 'to_entries[] | select(.value.result != "success") | "\(.key): \(.value.result)"' <<<"${job_results}" +)" + +if [[ -n "${unsuccessful_jobs}" ]]; then + while IFS= read -r unsuccessful_job; do + echo "::error::Job did not pass: ${unsuccessful_job}" + done <<<"${unsuccessful_jobs}" + exit 1 +fi + +echo "Every job passed." diff --git a/.github/scripts/install_locked_ruff.sh b/.github/scripts/install_locked_ruff.sh new file mode 100644 index 000000000..a93e802c5 --- /dev/null +++ b/.github/scripts/install_locked_ruff.sh @@ -0,0 +1,30 @@ +#!/usr/bin/env bash + +# Install the ruff version pinned in uv.lock, so CI formats and lints with the +# same ruff as `uv sync --extra dev`. +# +# Usage: install_locked_ruff.sh [path/to/uv.lock] +# PYTHON selects the interpreter that reads the lock (Python 3.11+ for tomllib). + +set -euo pipefail + +lock_file="${1:-uv.lock}" +python_bin="${PYTHON:-python3}" + +version="$( + "${python_bin}" - "${lock_file}" <<'PY' +import sys +import tomllib + +lock_path = sys.argv[1] +with open(lock_path, "rb") as lock: + packages = tomllib.load(lock).get("package", []) +versions = [package["version"] for package in packages if package.get("name") == "ruff"] +if not versions: + sys.exit(f"ruff is not pinned in {lock_path}; add it to the dev extra and run uv lock") +print(versions[0]) +PY +)" + +pip install "ruff==${version}" +ruff --version diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index b2bfb28ed..1c99a021b 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -11,8 +11,46 @@ env: OLD_SIMULATION_GATEWAY_URL: ${{ vars.OLD_SIMULATION_GATEWAY_URL }} jobs: + ci-complete: + # The required check. Needs every other job: on a stacked pull request + # above the lowest unmerged one it fails, because those jobs were skipped + # and a skipped job would count as passing; otherwise it passes only when + # every job passed. See docs/engineering/skills/github-prs.md. + name: CI complete + if: ${{ always() }} + needs: + - ensure-policyengine-bundle-supported-by-simulation-api + - lint + - typecheck-v2 + - quality-guards + - alembic-v1-check + - alembic-v2-check + - v2-integration-check + - check-changelog + - test_container_builds + - test_cloud_run_container_builds + - test_env_vars + - test + runs-on: ubuntu-latest + permissions: + contents: read + steps: + - name: Checkout the gate script + uses: actions/checkout@v4 + with: + sparse-checkout: .github/scripts/check_ci_complete.sh + sparse-checkout-cone-mode: false + - name: Require full CI, with every job passing + env: + JOB_RESULTS: ${{ toJSON(needs) }} + PULL_REQUEST_BASE_REF: ${{ github.event.pull_request.base.ref }} + STACK_BASE_REF: ${{ github.event.pull_request.stack.base.ref }} + run: bash .github/scripts/check_ci_complete.sh + ensure-policyengine-bundle-supported-by-simulation-api: name: Ensure PolicyEngine bundle is supported by simulation API + # Full CI runs only on the lowest unmerged pull request in a stack. + if: ${{ !github.event.pull_request.stack || github.event.pull_request.stack.base.ref == github.event.pull_request.base.ref }} runs-on: ubuntu-latest environment: staging env: @@ -34,18 +72,24 @@ jobs: lint: name: Lint + # Full CI runs only on the lowest unmerged pull request in a stack. + if: ${{ !github.event.pull_request.stack || github.event.pull_request.stack.base.ref == github.event.pull_request.base.ref }} runs-on: ubuntu-latest steps: - name: Checkout repo uses: actions/checkout@v4 - name: Setup Python uses: actions/setup-python@v5 - - name: Install ruff - run: pip install ruff>=0.9.0 + with: + python-version: "3.12" + - name: Install ruff (version pinned in uv.lock) + run: bash .github/scripts/install_locked_ruff.sh - name: Format check with ruff run: ruff format --check . typecheck-v2: name: Type-check API v2 + # Full CI runs only on the lowest unmerged pull request in a stack. + if: ${{ !github.event.pull_request.stack || github.event.pull_request.stack.base.ref == github.event.pull_request.base.ref }} runs-on: ubuntu-latest steps: - name: Checkout repo @@ -62,6 +106,8 @@ jobs: run: uv run --frozen --extra dev mypy quality-guards: name: Quality guards + # Full CI runs only on the lowest unmerged pull request in a stack. + if: ${{ !github.event.pull_request.stack || github.event.pull_request.stack.base.ref == github.event.pull_request.base.ref }} runs-on: ubuntu-latest steps: - name: Checkout repo @@ -75,17 +121,25 @@ jobs: alembic-v1-check: name: Alembic v1 qualification + # Full CI runs only on the lowest unmerged pull request in a stack. + if: ${{ !github.event.pull_request.stack || github.event.pull_request.stack.base.ref == github.event.pull_request.base.ref }} uses: ./.github/workflows/alembic-v1-check.yml alembic-v2-check: name: Alembic v2 qualification + # Full CI runs only on the lowest unmerged pull request in a stack. + if: ${{ !github.event.pull_request.stack || github.event.pull_request.stack.base.ref == github.event.pull_request.base.ref }} uses: ./.github/workflows/alembic-v2-check.yml v2-integration-check: name: Cross-database integration tests + # Full CI runs only on the lowest unmerged pull request in a stack. + if: ${{ !github.event.pull_request.stack || github.event.pull_request.stack.base.ref == github.event.pull_request.base.ref }} uses: ./.github/workflows/v2-integration-check.yml secrets: CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} check-changelog: name: Check changelog fragment + # Full CI runs only on the lowest unmerged pull request in a stack. + if: ${{ !github.event.pull_request.stack || github.event.pull_request.stack.base.ref == github.event.pull_request.base.ref }} runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 diff --git a/.github/workflows/push.yml b/.github/workflows/push.yml index 9213f4fbd..b153d92ae 100644 --- a/.github/workflows/push.yml +++ b/.github/workflows/push.yml @@ -23,8 +23,10 @@ jobs: uses: actions/checkout@v4 - name: Setup Python uses: actions/setup-python@v5 - - name: Install ruff - run: pip install ruff>=0.9.0 + with: + python-version: "3.12" + - name: Install ruff (version pinned in uv.lock) + run: bash .github/scripts/install_locked_ruff.sh - name: Format check with ruff run: ruff format --check . diff --git a/AGENTS.md b/AGENTS.md index f3034a66d..82b5f1c4f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -35,7 +35,9 @@ When modifying the `Makefile`, read ## GitHub PRs Read `docs/engineering/skills/github-prs.md` before opening, replacing, or -sharing any pull request. +sharing any pull request. Every PR needs a tracking issue and +`Fixes #ISSUE_NUMBER` as the first line of its description. Agents must never +bypass branch rules (for example with `gh pr merge --admin`). Before creating or sharing a migration PR, run the focused migration guards: diff --git a/changelog.d/phase-0-enforcement.changed.md b/changelog.d/phase-0-enforcement.changed.md new file mode 100644 index 000000000..f2f681554 --- /dev/null +++ b/changelog.d/phase-0-enforcement.changed.md @@ -0,0 +1 @@ +Added CODEOWNERS for the rule machinery, pinned CI's ruff to the version in uv.lock with an explicit rule selection, made the v2 CRUD boundary tests discover modules so households and user_households are checked, and documented the tracking-issue, Fixes line and no-bypass PR rules. In a stack of pull requests, only the lowest unmerged one now runs the full PR workflow, behind a new "CI complete" gate job that fails on the others. diff --git a/docs/engineering/skills/github-prs.md b/docs/engineering/skills/github-prs.md index ddbc83d9a..dcb2ab7d8 100644 --- a/docs/engineering/skills/github-prs.md +++ b/docs/engineering/skills/github-prs.md @@ -2,6 +2,42 @@ Use this skill before opening, replacing, or sharing a pull request. +## Tracking Issues + +Before opening a PR, all developers and agents must: + +1. Open a GitHub issue for the work in `PolicyEngine/policyengine-api`, or + verify that an appropriate issue already exists. +2. Put `Fixes #ISSUE_NUMBER` as the first line of the PR description, using the + issue number from the previous step. The PR template starts with this line. + +## Branch Rules + +Agents must never bypass branch rules, even when their credentials have admin +rights: do not merge with `gh pr merge --admin`, and do not use GitHub's option +to bypass rules. If a required check or review blocks a merge, report it to a +human instead. + +## Stacked PRs and CI + +In a stack of pull requests, only the lowest unmerged one runs the full PR +workflow (`.github/workflows/pr.yml`). The ones above it run only the +`CI complete` job, which fails and says so. A stack therefore uses one set of +runners instead of one per layer. + +- **`CI complete` is the required check.** It needs every other job. On a + pull request that isn't stacked, or is the lowest unmerged one, it passes + only when every job passed. Skipped jobs would otherwise count as passing, so + a stack can only merge bottom-up, and nothing merges untested. +- **When the pull request below merges,** GitHub rebases the next one onto + the trunk, and that push starts its full run. If no run starts, push again + (`gh stack rebase`, then `gh stack push`), or close and reopen the pull + request. Re-running the old workflow doesn't help: it reuses the old event, + which still describes the pull request as higher in the stack. +- **When you add a job to `pr.yml`,** add it to `CI complete`'s `needs`. If + the job has no `needs` of its own, also give it the same stack condition as + the other jobs. `tests/unit/test_ci_complete_gate.py` checks both. + ## Migration PRs Migration PRs should keep the user experience stable unless the PR explicitly diff --git a/pyproject.toml b/pyproject.toml index 54988ccad..04f8d065a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -79,6 +79,11 @@ packages = ["policyengine_api"] [tool.hatch.metadata] allow-direct-references = true +[tool.ruff.lint] +# Ruff's defaults as of 0.15, made explicit so a ruff upgrade cannot change +# which rules apply. Widen this deliberately, one rule family per PR. +select = ["E4", "E7", "E9", "F"] + [tool.ruff.lint.per-file-ignores] "migrations/*/versions/*.py" = ["F401"] diff --git a/tests/unit/test_alembic_workflows.py b/tests/unit/test_alembic_workflows.py index 1e2114154..81801d5c4 100644 --- a/tests/unit/test_alembic_workflows.py +++ b/tests/unit/test_alembic_workflows.py @@ -10,10 +10,24 @@ REPO = Path(__file__).resolve().parents[2] +STACKED_PR_CONDITION = ( + "${{ !github.event.pull_request.stack || " + "github.event.pull_request.stack.base.ref == github.event.pull_request.base.ref }}" +) + + def _workflow(name: str) -> str: return (REPO / ".github" / "workflows" / name).read_text(encoding="utf-8") +def _job_conditions(job_text: str) -> list[str]: + return [ + line.strip().removeprefix("if:").strip() + for line in job_text.splitlines() + if line.strip().startswith("if:") + ] + + def _long_inline_run_blocks() -> list[str]: offenders = [] block_markers = {"|", "|-", "|+", ">", ">-", ">+"} @@ -74,9 +88,13 @@ def test_pr_always_runs_reusable_alembic_and_v2_integration_checks(): assert "uses: ./.github/workflows/v2-integration-check.yml" in integration_job assert "CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}" in integration_job assert "needs:" not in alembic_job - assert "if:" not in alembic_job assert "needs:" not in integration_job - assert "if:" not in integration_job + # The only condition allowed is the stacked-PR one: these checks run on + # every PR that isn't stacked and on the lowest unmerged PR of a stack, and + # CI complete, which needs them, fails on the others + # (tests/unit/test_ci_complete_gate.py). + assert _job_conditions(alembic_job) == [STACKED_PR_CONDITION] + assert _job_conditions(integration_job) == [STACKED_PR_CONDITION] def test_push_requires_schema_and_v2_integration_checks_before_versioning(): diff --git a/tests/unit/test_ci_complete_gate.py b/tests/unit/test_ci_complete_gate.py new file mode 100644 index 000000000..4905be87a --- /dev/null +++ b/tests/unit/test_ci_complete_gate.py @@ -0,0 +1,167 @@ +"""Tests for the stacked-PR CI gate: pr.yml's shape and check_ci_complete.sh. + +Only the lowest unmerged pull request in a stack runs the full PR workflow. +The "CI complete" job is the required check: it needs every other job, fails on +the pull requests above the bottom of a stack, and otherwise passes only when +every job passed. +""" + +from __future__ import annotations + +import json +import os +import subprocess +from pathlib import Path + +import yaml + +REPO = Path(__file__).resolve().parents[2] +PR_WORKFLOW = REPO / ".github" / "workflows" / "pr.yml" +GATE_SCRIPT = ".github/scripts/check_ci_complete.sh" +GATE_JOB = "ci-complete" +GATE_CONDITION = "${{ always() }}" +FULL_CI_CONDITION = ( + "${{ !github.event.pull_request.stack || " + "github.event.pull_request.stack.base.ref == github.event.pull_request.base.ref }}" +) + + +def gate_violations(workflow: dict) -> list[str]: + """Return every way a job in ``workflow`` could escape the CI-complete gate.""" + jobs = workflow.get("jobs") or {} + gate = jobs.get(GATE_JOB) + if gate is None: + return [f"the workflow has no {GATE_JOB} job"] + + violations = [] + if gate.get("if") != GATE_CONDITION: + violations.append(f"{GATE_JOB} must have if: {GATE_CONDITION}") + + gated_jobs = gate.get("needs") or [] + if isinstance(gated_jobs, str): + gated_jobs = [gated_jobs] + for job_id in sorted(set(jobs) - {GATE_JOB} - set(gated_jobs)): + violations.append(f"{GATE_JOB} does not need {job_id}") + + for job_id, job in jobs.items(): + if job_id == GATE_JOB: + continue + condition = job.get("if") + if job.get("needs"): + # A job that needs a skipped job is skipped too, unless its own + # condition overrides that. + if condition is not None and condition != FULL_CI_CONDITION: + violations.append(f"{job_id} has its own condition: {condition}") + elif condition != FULL_CI_CONDITION: + violations.append(f"{job_id} must have if: {FULL_CI_CONDITION}") + return violations + + +def test_pr_workflow_is_gated() -> None: + workflow = yaml.safe_load(PR_WORKFLOW.read_text()) + + assert gate_violations(workflow) == [] + + +def test_gate_check_rejects_an_ungated_job() -> None: + workflow = { + "jobs": { + GATE_JOB: {"if": GATE_CONDITION, "needs": ["lint"]}, + "lint": {"if": FULL_CI_CONDITION}, + "test": {}, + } + } + + assert gate_violations(workflow) == [ + f"{GATE_JOB} does not need test", + f"test must have if: {FULL_CI_CONDITION}", + ] + + +def _run_gate( + job_results: dict[str, str], + *, + pull_request_base_ref: str = "master", + stack_base_ref: str = "", +) -> subprocess.CompletedProcess[str]: + return subprocess.run( + ["bash", GATE_SCRIPT], + cwd=REPO, + env={ + "HOME": os.environ.get("HOME", ""), + "PATH": os.environ["PATH"], + "JOB_RESULTS": json.dumps( + { + job_id: {"result": result, "outputs": {}} + for job_id, result in job_results.items() + } + ), + "PULL_REQUEST_BASE_REF": pull_request_base_ref, + "STACK_BASE_REF": stack_base_ref, + }, + text=True, + capture_output=True, + check=False, + ) + + +def test_gate_passes_an_unstacked_pull_request_when_every_job_passed() -> None: + result = _run_gate({"lint": "success", "test": "success"}) + + assert result.returncode == 0, result.stdout + assert result.stdout == "Every job passed.\n" + + +def test_gate_passes_the_lowest_unmerged_pull_request_in_a_stack() -> None: + result = _run_gate( + {"lint": "success", "test": "success"}, + pull_request_base_ref="master", + stack_base_ref="master", + ) + + assert result.returncode == 0, result.stdout + + +def test_gate_fails_a_pull_request_higher_in_a_stack() -> None: + result = _run_gate( + {"lint": "skipped", "test": "skipped"}, + pull_request_base_ref="ruff-annotations", + stack_base_ref="master", + ) + + assert result.returncode == 1 + assert "lowest unmerged pull request in a stack" in result.stdout + assert "targets ruff-annotations" in result.stdout + + +def test_gate_fails_and_names_every_job_that_did_not_pass() -> None: + result = _run_gate( + {"lint": "success", "test": "failure", "typecheck-v2": "cancelled"}, + ) + + assert result.returncode == 1 + assert result.stdout.splitlines() == [ + "::error::Job did not pass: test: failure", + "::error::Job did not pass: typecheck-v2: cancelled", + ] + + +def test_gate_fails_when_a_job_was_skipped_on_full_ci() -> None: + result = _run_gate({"lint": "success", "test": "skipped"}, stack_base_ref="master") + + assert result.returncode == 1 + assert "::error::Job did not pass: test: skipped" in result.stdout + + +def test_gate_requires_the_job_results() -> None: + result = subprocess.run( + ["bash", GATE_SCRIPT], + cwd=REPO, + env={"PATH": os.environ["PATH"], "PULL_REQUEST_BASE_REF": "master"}, + text=True, + capture_output=True, + check=False, + ) + + assert result.returncode != 0 + assert "JOB_RESULTS is required" in result.stderr diff --git a/tests/unit/test_install_locked_ruff_script.py b/tests/unit/test_install_locked_ruff_script.py new file mode 100644 index 000000000..78ba3aae6 --- /dev/null +++ b/tests/unit/test_install_locked_ruff_script.py @@ -0,0 +1,94 @@ +"""Tests for .github/scripts/install_locked_ruff.sh.""" + +from __future__ import annotations + +import os +import subprocess +import sys +import tomllib +from pathlib import Path + +REPO = Path(__file__).resolve().parents[2] +SCRIPT = REPO / ".github/scripts/install_locked_ruff.sh" + + +def _fake_commands(tmp_path: Path, *, pip_exit: int = 0) -> tuple[Path, Path]: + """Fake pip and ruff that record their arguments instead of installing.""" + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + calls = tmp_path / "calls.log" + for name, exit_code in (("pip", pip_exit), ("ruff", 0)): + command = bin_dir / name + command.write_text( + f'#!/usr/bin/env bash\necho "{name} $*" >> "{calls}"\nexit {exit_code}\n' + ) + command.chmod(0o755) + return bin_dir, calls + + +def _run( + tmp_path: Path, *args: str, pip_exit: int = 0 +) -> tuple[subprocess.CompletedProcess[str], list[str]]: + bin_dir, calls = _fake_commands(tmp_path, pip_exit=pip_exit) + result = subprocess.run( + ["bash", str(SCRIPT), *args], + cwd=REPO, + env={ + "PATH": f"{bin_dir}{os.pathsep}{os.environ['PATH']}", + "PYTHON": sys.executable, + }, + text=True, + capture_output=True, + check=False, + ) + logged = calls.read_text().splitlines() if calls.exists() else [] + return result, logged + + +def _lock(tmp_path: Path, packages: str) -> Path: + lock = tmp_path / "uv.lock" + lock.write_text(f"version = 1\n{packages}") + return lock + + +def test_installs_the_ruff_version_pinned_in_the_lock(tmp_path: Path) -> None: + lock = _lock( + tmp_path, + '[[package]]\nname = "pytest"\nversion = "8.0.0"\n\n' + '[[package]]\nname = "ruff"\nversion = "0.15.12"\n', + ) + + result, calls = _run(tmp_path, str(lock)) + + assert result.returncode == 0, result.stderr + assert calls == ["pip install ruff==0.15.12", "ruff --version"] + + +def test_defaults_to_the_repository_lock(tmp_path: Path) -> None: + with (REPO / "uv.lock").open("rb") as lock: + packages = tomllib.load(lock)["package"] + locked = next(p["version"] for p in packages if p["name"] == "ruff") + + result, calls = _run(tmp_path) + + assert result.returncode == 0, result.stderr + assert calls[0] == f"pip install ruff=={locked}" + + +def test_fails_without_installing_when_ruff_is_not_locked(tmp_path: Path) -> None: + lock = _lock(tmp_path, '[[package]]\nname = "pytest"\nversion = "8.0.0"\n') + + result, calls = _run(tmp_path, str(lock)) + + assert result.returncode != 0 + assert "ruff is not pinned" in result.stderr + assert calls == [] + + +def test_stops_when_pip_fails(tmp_path: Path) -> None: + lock = _lock(tmp_path, '[[package]]\nname = "ruff"\nversion = "0.15.12"\n') + + result, calls = _run(tmp_path, str(lock), pip_exit=1) + + assert result.returncode != 0 + assert calls == ["pip install ruff==0.15.12"] diff --git a/tests/unit/v2/test_data_crud_boundaries.py b/tests/unit/v2/test_data_crud_boundaries.py index 44b49ff3c..4e08780f6 100644 --- a/tests/unit/v2/test_data_crud_boundaries.py +++ b/tests/unit/v2/test_data_crud_boundaries.py @@ -38,55 +38,63 @@ def _imported_modules(path: Path) -> set[str]: return modules -@pytest.mark.parametrize( - "relative_path", - ( - "policies/database_connectors/creates.py", - "comparison_runs/database_connectors/creates.py", - "user_policies/database_connectors/creates.py", - "comparison_runs/database_connectors/updates.py", - "user_policies/database_connectors/updates.py", - "user_policies/database_connectors/deletes.py", - ), +def _discover(*patterns: str) -> list[str]: + """Modules under SERVICES_ROOT matching any pattern, as sorted relative paths. + + Discovering modules (rather than listing them) means a new resource package + is checked automatically. An empty result fails at collection, because + pytest silently skips a test whose parameter list is empty. + """ + paths = sorted( + { + path.relative_to(SERVICES_ROOT).as_posix() + for pattern in patterns + for path in SERVICES_ROOT.glob(pattern) + } + ) + assert paths, f"no modules under {SERVICES_ROOT} match {patterns}" + return paths + + +MUTATION_CONNECTORS = _discover( + "*/database_connectors/creates*.py", + "*/database_connectors/updates*.py", + "*/database_connectors/deletes*.py", ) +READ_CONNECTORS = _discover("*/database_connectors/reads*.py") +VALIDATION_AND_TRANSFORMATION_MODULES = _discover( + "*/validators.py", "*/transformations.py" +) +DATABASE_SESSION_MODULES = _discover("*/database_session.py") + + +def test_every_connector_module_is_classified() -> None: + connectors = { + path + for path in _discover("*/database_connectors/*.py") + if not path.endswith("/__init__.py") + } + classified = set(MUTATION_CONNECTORS) | set(READ_CONNECTORS) + assert connectors - classified == set(), ( + "connector modules must be named creates, reads, updates or deletes " + "(optionally with a _suffix) so the boundary checks below cover them" + ) + + +@pytest.mark.parametrize("relative_path", MUTATION_CONNECTORS) def test_mutation_connectors_contain_no_database_reads(relative_path: str) -> None: calls = _called_names(SERVICES_ROOT / relative_path) assert "select" not in calls assert "get" not in calls -@pytest.mark.parametrize( - "relative_path", - ( - "policies/database_connectors/reads.py", - "comparison_runs/database_connectors/reads.py", - "user_policies/database_connectors/reads.py", - "metadata/database_connectors/reads.py", - "metadata/database_connectors/reads_datasets.py", - "metadata/database_connectors/reads_parameter_tree.py", - "metadata/database_connectors/reads_parameters.py", - "metadata/database_connectors/reads_regions.py", - "metadata/database_connectors/reads_variables.py", - ), -) +@pytest.mark.parametrize("relative_path", READ_CONNECTORS) def test_read_connectors_contain_no_database_mutations(relative_path: str) -> None: calls = _called_names(SERVICES_ROOT / relative_path) assert calls.isdisjoint({"insert", "add", "add_all", "delete"}) -@pytest.mark.parametrize( - "relative_path", - ( - "policies/validators.py", - "policies/transformations.py", - "metadata/validators.py", - "metadata/transformations.py", - "comparison_runs/validators.py", - "comparison_runs/transformations.py", - "user_policies/validators.py", - "user_policies/transformations.py", - ), -) +@pytest.mark.parametrize("relative_path", VALIDATION_AND_TRANSFORMATION_MODULES) def test_validation_and_transformation_modules_have_no_database_query_dependency( relative_path: str, ) -> None: @@ -100,15 +108,7 @@ def test_validation_and_transformation_modules_have_no_database_query_dependency ) -@pytest.mark.parametrize( - "relative_path", - ( - "policies/database_session.py", - "comparison_runs/database_session.py", - "metadata/database_session.py", - "user_policies/database_session.py", - ), -) +@pytest.mark.parametrize("relative_path", DATABASE_SESSION_MODULES) def test_database_session_modules_do_not_construct_queries( relative_path: str, ) -> None: