From 77dae439e7e9b9d4ac47efedee32b5dbbcde799f Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sat, 10 Oct 2026 16:59:48 -0400 Subject: [PATCH 1/2] Split the Test job into a YAML job and three shards of the Python tests The Test job took 59 minutes (run 38075384588): 2.6 minutes of YAML tests, then 55.8 minutes of pytest on four balanced xdist workers, 220 worker-minutes in all. No test dominates: 51 property-test files take 74% of it and the slowest test takes 3.5%. So the job is split rather than one test fixed. - `--shard K/N` (tests/conftest.py) keeps the tests whose node id hashes to shard K of N. The shard is a function of the node id alone, so every collected test is in exactly one shard, including tests added later. - The workflows run the YAML tests in one job and the Python tests as a matrix of three shards, N taken from the size of the matrix. - A final `Test` job runs .github/check_test_shards.py over the shards' JUnit files and an unsharded `pytest --collect-only -q` listing. It fails unless every collected test ran in exactly one shard and none failed. - Coverage is measured with sys.monitoring (COVERAGE_CORE=sysmon) instead of the C trace function. On one property-test file the two report the same 12,262 of 18,401 lines, and sys.monitoring adds 6% to the CPU time where the trace function adds 33%. Refs #2099 Co-Authored-By: Claude Opus 5.5 --- .github/check_test_shards.py | 195 ++++++++++ .github/workflows/code_changes.yaml | 114 +++++- .github/workflows/pr_code_changes.yaml | 114 +++++- Makefile | 6 +- changelog.d/ci-test-runtime-2099.fixed.md | 1 + docs/engineering/skills/testing.md | 21 ++ .../tests/code_health/test_ci_test_shards.py | 332 ++++++++++++++++++ policyengine_uk/tests/conftest.py | 57 +++ 8 files changed, 817 insertions(+), 23 deletions(-) create mode 100644 .github/check_test_shards.py create mode 100644 changelog.d/ci-test-runtime-2099.fixed.md create mode 100644 policyengine_uk/tests/code_health/test_ci_test_shards.py diff --git a/.github/check_test_shards.py b/.github/check_test_shards.py new file mode 100644 index 0000000000..69f40fa83f --- /dev/null +++ b/.github/check_test_shards.py @@ -0,0 +1,195 @@ +"""Check that the shards of the Test job ran every test exactly once. + +CI splits ``pytest policyengine_uk/tests`` over several runners with +``--shard K/N`` (see policyengine_uk/tests/conftest.py). This script compares +what the shards ran with what an unsharded ``pytest --collect-only -q`` lists: + + python .github/check_test_shards.py \ + --collected collected.txt --junit junit-1.xml junit-2.xml junit-3.xml + +It fails if a collected test is missing from every shard, ran in more than one, +ran without being collected, or failed. So splitting the suite cannot drop a +test without the Test job going red. + +Standard library only: the job that runs it does not install the package. +""" + +import argparse +import re +import sys +import xml.etree.ElementTree as ElementTree +from collections import Counter +from pathlib import Path + +_COLLECTED_SUMMARY = re.compile(r"^(\d+) tests? collected\b(.*)$") +_NOTHING_COLLECTED = re.compile(r"^no tests collected\b") + + +class ShardCheckError(Exception): + """An input that cannot be checked, as opposed to a check that failed.""" + + +def junit_key(nodeid: str) -> tuple[str, str]: + """The ``(classname, name)`` pytest gives ``nodeid`` in JUnit XML. + + Follows _pytest.junitxml.mangle_test_address; a test compares the two. + pytest also rewrites characters that XML does not allow, such as control + characters. Its node ids do not contain them, because it escapes them in + parameter ids. If one ever did, the names would differ and the check + would fail, not pass. + """ + path, bracket, params = nodeid.partition("[") + names = path.split("::") + names[0] = re.sub(r"\.py$", "", names[0].replace("/", ".")) + names[-1] += bracket + params + return ".".join(names[:-1]), names[-1] + + +def read_collected(text: str) -> list[str]: + """Node ids from the output of ``pytest --collect-only -q``. + + The listing is one node id per line, a blank line, then any warnings and + ``N tests collected``. The ids found must number N, so a listing this + cannot parse is an error, never a shorter list. + """ + ids = [] + total = None + listing_ended = False + for line in text.splitlines(): + summary = _COLLECTED_SUMMARY.match(line) + if summary: + if "error" in summary.group(2): + raise ShardCheckError(f"collection reported errors: {line!r}") + total = int(summary.group(1)) + elif _NOTHING_COLLECTED.match(line): + total = 0 + elif not line.strip(): + listing_ended = listing_ended or bool(ids) + elif "::" in line and not listing_ended: + ids.append(line) + if total is None: + raise ShardCheckError("no 'N tests collected' line in the collection listing") + if len(ids) != total: + raise ShardCheckError( + f"the collection listing says {total} tests but lists {len(ids)}" + ) + if len(set(ids)) != len(ids): + raise ShardCheckError("the collection listing repeats a node id") + return ids + + +def read_junit(path: Path) -> list[tuple[tuple[str, str], str]]: + """``((classname, name), outcome)`` for each test case in a JUnit file. + + The outcome is ``failed`` for a failure or an error, else ``skipped`` for a + skip or an expected failure, else ``passed``. + """ + cases = [] + for case in ElementTree.parse(path).getroot().iter("testcase"): + children = {child.tag for child in case} + if children & {"failure", "error"}: + outcome = "failed" + elif "skipped" in children: + outcome = "skipped" + else: + outcome = "passed" + cases.append(((case.get("classname", ""), case.get("name", "")), outcome)) + return cases + + +def check(collected: list[str], shards: list[list[tuple[tuple[str, str], str]]]): + """Compare the collected node ids with what each shard ran. + + Returns ``(problems, summary)``: ``problems`` is empty when every collected + test ran in exactly one shard and none failed. + """ + expected = {} + for nodeid in collected: + key = junit_key(nodeid) + if key in expected: + raise ShardCheckError( + f"{nodeid!r} and {expected[key]!r} have the same JUnit name" + ) + expected[key] = nodeid + + runs = Counter() + outcomes = Counter() + failed = [] + for number, cases in enumerate(shards, start=1): + for key, outcome in cases: + runs[key] += 1 + outcomes[outcome] += 1 + if outcome == "failed": + failed.append((number, key)) + + def name(key): + return expected.get(key) or f"{key[0]}::{key[1]}" + + problems = [] + missing = sorted(expected[key] for key in expected if key not in runs) + if missing: + problems.append((f"{len(missing)} collected tests ran in no shard", missing)) + repeated = sorted(name(key) for key, count in runs.items() if count > 1) + if repeated: + problems.append((f"{len(repeated)} tests ran more than once", repeated)) + extra = sorted(name(key) for key in runs if key not in expected) + if extra: + problems.append((f"{len(extra)} tests ran but were not collected", extra)) + if failed: + problems.append( + ( + f"{len(failed)} tests failed", + [f"shard {number}: {name(key)}" for number, key in failed], + ) + ) + + sizes = "/".join(str(len(cases)) for cases in shards) + summary = ( + f"{len(collected)} tests collected; {len(shards)} shards ran " + f"{sizes} = {sum(len(cases) for cases in shards)} " + f"({outcomes['passed']} passed, {outcomes['skipped']} skipped or " + f"expected failures, {outcomes['failed']} failed)" + ) + return problems, summary + + +def main(argv=None) -> int: + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + parser.add_argument( + "--collected", + required=True, + type=Path, + help="output of an unsharded `pytest --collect-only -q`", + ) + parser.add_argument( + "--junit", + required=True, + nargs="+", + type=Path, + help="the JUnit XML file of each shard", + ) + args = parser.parse_args(argv) + + try: + collected = read_collected(args.collected.read_text(encoding="utf-8")) + shards = [read_junit(path) for path in args.junit] + problems, summary = check(collected, shards) + except (ShardCheckError, OSError, ElementTree.ParseError) as error: + print(f"Cannot check the shards: {error}", file=sys.stderr) + return 2 + + print(summary) + for title, names in problems: + print(f"\n{title}:") + for line in names[:50]: + print(f" {line}") + if len(names) > 50: + print(f" ... and {len(names) - 50} more") + if problems: + return 1 + print("Every collected test ran in exactly one shard.") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/workflows/code_changes.yaml b/.github/workflows/code_changes.yaml index 5ba890a038..bd414330c1 100644 --- a/.github/workflows/code_changes.yaml +++ b/.github/workflows/code_changes.yaml @@ -31,19 +31,79 @@ jobs: uses: astral-sh/setup-uv@v8.1.0 - name: Check formatting run: uvx ruff format --check . - Test: + # The tests run as four jobs: the YAML policy tests, and the Python tests + # split over three runners. As one job they took 59 minutes on 10 October + # 2026 (run 38075384588: 2.6 minutes of YAML tests, then 55.8 minutes of + # pytest on four workers), and runs were cancelled at a 60-minute limit + # (issues #2099 and #2257). + Test-YAML: + name: Test (YAML) # Linux arm64 keeps the CPU architecture of the macOS runners this job # used to run on. GitHub-hosted macOS runners are scarce, so PR runs # queued for hours; the parameter tree and every test result are # identical on both. runs-on: ubuntu-24.04-arm - # The whole job took about 18 minutes when this limit was set; by - # October 2026 it takes about an hour (57 to 60 minutes on 9 October, - # and every run on 10 October was cancelled at a 60-minute limit, one - # at 99% with every test passing; issue #2257). 120 minutes lets a - # slow run finish and still stops a hung one long before the 6-hour - # GitHub limit. The suite's runtime needs its own attention. - timeout-minutes: 120 + timeout-minutes: 30 + permissions: + contents: "read" + id-token: "write" + steps: + - name: Checkout repo + uses: actions/checkout@v6 + - name: Install uv + uses: astral-sh/setup-uv@v8.1.0 + + - name: Set up Python + uses: actions/setup-python@v6 + with: + python-version: '3.13' + - uses: "google-github-actions/auth@v3" + with: + workload_identity_provider: "projects/322898545428/locations/global/workloadIdentityPools/policyengine-research-id-pool/providers/prod-github-provider" + service_account: "policyengine-research@policyengine-research.iam.gserviceaccount.com" + + - name: Install package + run: uv pip install -e .[dev] --system + - name: Install policyengine + run: uv pip install policyengine --system + - name: Install JB + run: uv pip install "jupyter-book>=2.0.0a0" --system + - name: UV sync + run: uv sync --extra dev + - name: Run YAML tests + run: make test-yaml + - name: List the Python tests + # Every Python test, collected without --shard. The Test job + # checks that the shards below ran each of these exactly once. + run: | + # Only export token if non-empty (defensive against missing secrets) + if [ -n "$HF_TOKEN_VALUE" ]; then + export HUGGING_FACE_TOKEN="$HF_TOKEN_VALUE" + fi + pytest policyengine_uk/tests --collect-only -q -p no:cacheprovider > collected.txt + tail -n 1 collected.txt + env: + HF_TOKEN_VALUE: ${{ secrets.HUGGING_FACE_TOKEN }} + - name: Upload the list of Python tests + uses: actions/upload-artifact@v7 + with: + name: test-collected + path: collected.txt + if-no-files-found: error + overwrite: true + retention-days: 7 + Test-Python: + # Shard K of N runs the tests whose node id hashes to K (--shard in + # policyengine_uk/tests/conftest.py), so each test is in exactly one + # shard. N is the number of jobs this matrix makes: to add a shard, add + # a number to the list. + name: Test (Python ${{ matrix.shard }}/${{ strategy.job-total }}) + runs-on: ubuntu-24.04-arm + timeout-minutes: 60 + strategy: + fail-fast: false + matrix: + shard: [1, 2, 3] permissions: contents: "read" id-token: "write" @@ -61,7 +121,7 @@ jobs: with: workload_identity_provider: "projects/322898545428/locations/global/workloadIdentityPools/policyengine-research-id-pool/providers/prod-github-provider" service_account: "policyengine-research@policyengine-research.iam.gserviceaccount.com" - + - name: Install package run: uv pip install -e .[dev] --system - name: Install policyengine @@ -78,12 +138,46 @@ jobs: fi # Four pytest-xdist workers, one per core; together they peak at # about 9 GB of the runner's 16 GB. - make test PYTEST_ARGS="-n 4 --dist worksteal" + make test-python PYTEST_ARGS="-n 4 --dist worksteal --shard ${{ matrix.shard }}/${{ strategy.job-total }} --junitxml=junit-${{ matrix.shard }}.xml" env: HF_TOKEN_VALUE: ${{ secrets.HUGGING_FACE_TOKEN }} + # Measure coverage with sys.monitoring instead of the C trace + # function: the same lines are reported, at less cost per line. + COVERAGE_CORE: sysmon + - name: Upload the test results + if: ${{ !cancelled() }} + uses: actions/upload-artifact@v7 + with: + name: test-junit-${{ matrix.shard }} + path: junit-${{ matrix.shard }}.xml + if-no-files-found: error + overwrite: true + retention-days: 7 - name: Upload coverage to Codecov uses: codecov/codecov-action@v6 with: files: ./coverage.xml fail_ci_if_error: false verbose: true + Test: + # One result for the whole suite. It fails if any test job failed, or + # if the shards did not run every collected Python test exactly once. + needs: [Test-YAML, Test-Python] + if: ${{ !cancelled() }} + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - name: Checkout repo + uses: actions/checkout@v6 + - name: Download the test lists and results + uses: actions/download-artifact@v8 + with: + pattern: test-* + merge-multiple: true + - name: Check that every Python test ran in exactly one shard + run: python3 .github/check_test_shards.py --collected collected.txt --junit junit-*.xml + - name: Check that every test job passed + run: | + echo "YAML: ${{ needs.Test-YAML.result }}; Python: ${{ needs.Test-Python.result }}" + test "${{ needs.Test-YAML.result }}" = success + test "${{ needs.Test-Python.result }}" = success diff --git a/.github/workflows/pr_code_changes.yaml b/.github/workflows/pr_code_changes.yaml index 055741c182..26d410db9d 100644 --- a/.github/workflows/pr_code_changes.yaml +++ b/.github/workflows/pr_code_changes.yaml @@ -51,19 +51,79 @@ jobs: uses: astral-sh/setup-uv@v8.1.0 - name: Check formatting run: uvx ruff format --check . - Test: + # The tests run as four jobs: the YAML policy tests, and the Python tests + # split over three runners. As one job they took 59 minutes on 10 October + # 2026 (run 38075384588: 2.6 minutes of YAML tests, then 55.8 minutes of + # pytest on four workers), and runs were cancelled at a 60-minute limit + # (issues #2099 and #2257). + Test-YAML: + name: Test (YAML) # Linux arm64 keeps the CPU architecture of the macOS runners this job # used to run on. GitHub-hosted macOS runners are scarce, so PR runs # queued for hours; the parameter tree and every test result are # identical on both. runs-on: ubuntu-24.04-arm - # The whole job took about 18 minutes when this limit was set; by - # October 2026 it takes about an hour (57 to 60 minutes on 9 October, - # and every run on 10 October was cancelled at a 60-minute limit, one - # at 99% with every test passing; issue #2257). 120 minutes lets a - # slow run finish and still stops a hung one long before the 6-hour - # GitHub limit. The suite's runtime needs its own attention. - timeout-minutes: 120 + timeout-minutes: 30 + permissions: + contents: "read" + id-token: "write" + steps: + - name: Checkout repo + uses: actions/checkout@v6 + - name: Install uv + uses: astral-sh/setup-uv@v8.1.0 + + - name: Set up Python + uses: actions/setup-python@v6 + with: + python-version: '3.13' + - uses: "google-github-actions/auth@v3" + with: + workload_identity_provider: "projects/322898545428/locations/global/workloadIdentityPools/policyengine-research-id-pool/providers/prod-github-provider" + service_account: "policyengine-research@policyengine-research.iam.gserviceaccount.com" + + - name: Install package + run: uv pip install -e .[dev] --system + - name: Install policyengine + run: uv pip install policyengine --system + - name: UV sync + run: uv sync --extra dev + - name: Run YAML tests + run: make test-yaml + - name: List the Python tests + # Every Python test, collected without --shard. The Test job + # checks that the shards below ran each of these exactly once. + run: | + # Only export token if non-empty (avoids 'Bearer ' error for Dependabot PRs) + if [ -n "$HF_TOKEN_VALUE" ]; then + export HUGGING_FACE_TOKEN="$HF_TOKEN_VALUE" + fi + pytest policyengine_uk/tests --collect-only -q -p no:cacheprovider > collected.txt + tail -n 1 collected.txt + env: + HF_TOKEN_VALUE: ${{ secrets.HUGGING_FACE_TOKEN }} + - name: Upload the list of Python tests + uses: actions/upload-artifact@v7 + with: + name: test-collected + path: collected.txt + if-no-files-found: error + overwrite: true + retention-days: 7 + - name: Test documentation builds + run: make documentation + Test-Python: + # Shard K of N runs the tests whose node id hashes to K (--shard in + # policyengine_uk/tests/conftest.py), so each test is in exactly one + # shard. N is the number of jobs this matrix makes: to add a shard, add + # a number to the list. + name: Test (Python ${{ matrix.shard }}/${{ strategy.job-total }}) + runs-on: ubuntu-24.04-arm + timeout-minutes: 60 + strategy: + fail-fast: false + matrix: + shard: [1, 2, 3] permissions: contents: "read" id-token: "write" @@ -96,14 +156,46 @@ jobs: fi # Four pytest-xdist workers, one per core; together they peak at # about 9 GB of the runner's 16 GB. - make test PYTEST_ARGS="-n 4 --dist worksteal" + make test-python PYTEST_ARGS="-n 4 --dist worksteal --shard ${{ matrix.shard }}/${{ strategy.job-total }} --junitxml=junit-${{ matrix.shard }}.xml" env: HF_TOKEN_VALUE: ${{ secrets.HUGGING_FACE_TOKEN }} + # Measure coverage with sys.monitoring instead of the C trace + # function: the same lines are reported, at less cost per line. + COVERAGE_CORE: sysmon + - name: Upload the test results + if: ${{ !cancelled() }} + uses: actions/upload-artifact@v7 + with: + name: test-junit-${{ matrix.shard }} + path: junit-${{ matrix.shard }}.xml + if-no-files-found: error + overwrite: true + retention-days: 7 - name: Upload coverage to Codecov uses: codecov/codecov-action@v6 with: files: ./coverage.xml fail_ci_if_error: false verbose: true - - name: Test documentation builds - run: make documentation + Test: + # One result for the whole suite. It fails if any test job failed, or + # if the shards did not run every collected Python test exactly once. + needs: [Test-YAML, Test-Python] + if: ${{ !cancelled() }} + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - name: Checkout repo + uses: actions/checkout@v6 + - name: Download the test lists and results + uses: actions/download-artifact@v8 + with: + pattern: test-* + merge-multiple: true + - name: Check that every Python test ran in exactly one shard + run: python3 .github/check_test_shards.py --collected collected.txt --junit junit-*.xml + - name: Check that every test job passed + run: | + echo "YAML: ${{ needs.Test-YAML.result }}; Python: ${{ needs.Test-Python.result }}" + test "${{ needs.Test-YAML.result }}" = success + test "${{ needs.Test-Python.result }}" = success diff --git a/Makefile b/Makefile index 5723221068..528f7b9be3 100644 --- a/Makefile +++ b/Makefile @@ -19,8 +19,10 @@ test: test-yaml test-python test-yaml: policyengine-core test policyengine_uk/tests/policy -c policyengine_uk -# CI passes PYTEST_ARGS="-n 4 --dist worksteal" (pytest-xdist) to spread the -# tests over the runner's four cores. Without it this runs serially, as before. +# CI passes PYTEST_ARGS to spread the tests over a runner's four cores +# ("-n 4 --dist worksteal", pytest-xdist) and over several runners +# ("--shard K/N", policyengine_uk/tests/conftest.py). Without it this runs +# every test serially. test-python: pytest policyengine_uk/tests/ --cov=policyengine_uk --cov-report=xml --maxfail=0 -v $(PYTEST_ARGS) diff --git a/changelog.d/ci-test-runtime-2099.fixed.md b/changelog.d/ci-test-runtime-2099.fixed.md new file mode 100644 index 0000000000..413d63714e --- /dev/null +++ b/changelog.d/ci-test-runtime-2099.fixed.md @@ -0,0 +1 @@ +- Split the CI `Test` job into a YAML job and three parallel shards of the Python tests, with a check that every collected test ran in exactly one shard, and measured coverage with `sys.monitoring` (#2099). diff --git a/docs/engineering/skills/testing.md b/docs/engineering/skills/testing.md index 66cb54d3c4..6894b55b4a 100644 --- a/docs/engineering/skills/testing.md +++ b/docs/engineering/skills/testing.md @@ -58,3 +58,24 @@ make test-microsimulation ``` When a command is not run, record why. + +## How CI Runs The Tests + +The test jobs in `.github/workflows/pr_code_changes.yaml` and +`code_changes.yaml` run the YAML policy tests in one job and split the Python +tests over several runners. Each of those runs one shard: + +```bash +make test-python PYTEST_ARGS="-n 4 --dist worksteal --shard K/N" +``` + +`--shard K/N` is defined in `policyengine_uk/tests/conftest.py`. It keeps the +tests whose node id hashes to shard K of N, so every test is in exactly one +shard, including a test added later. The final `Test` job runs +`.github/check_test_shards.py`, which fails unless every collected test ran in +exactly one shard. + +- To rerun what one CI shard ran, pass the same `--shard K/N` locally. +- To make CI faster, add a shard to the `shard` matrix in both workflows. +- Do not split tests by file or directory in a workflow: a test outside the + listed paths would silently not run. diff --git a/policyengine_uk/tests/code_health/test_ci_test_shards.py b/policyengine_uk/tests/code_health/test_ci_test_shards.py new file mode 100644 index 0000000000..17e3d71586 --- /dev/null +++ b/policyengine_uk/tests/code_health/test_ci_test_shards.py @@ -0,0 +1,332 @@ +"""The Test job's shards run every test exactly once. + +CI runs ``pytest policyengine_uk/tests`` as several jobs, each with +``--shard K/N`` (conftest.py), and then runs ``.github/check_test_shards.py`` +over their JUnit files and an unsharded ``pytest --collect-only -q`` listing. +These tests hold the two halves to the invariants the split relies on: + +1. ``shard_of(nodeid, N)`` is between 1 and N and depends only on its + arguments, so it is the same in every process and on every runner. +2. For any set of node ids and any N, the N shards are disjoint and their + union is the whole set. +3. ``parse_shard`` accepts exactly ``K/N`` with 1 <= K <= N. +4. ``junit_key`` gives a node id the name pytest gives it in JUnit XML. +5. The check passes when every collected test ran in exactly one shard and + none failed, and reports a test that is missing, repeated, not collected + or failed. +6. With real pytest runs, the collection listing and the shards' JUnit files + pass the check, with and without pytest-xdist, and fail it if a shard's + file is left out. +""" + +import argparse +import importlib.util +import os +import shutil +import subprocess +import sys +import xml.etree.ElementTree as ElementTree +from pathlib import Path + +import pytest +from hypothesis import HealthCheck, example, given, settings +from hypothesis import strategies as st + +TESTS_DIR = Path(__file__).resolve().parents[1] +REPO_ROOT = TESTS_DIR.parents[1] +CHECKER = REPO_ROOT / ".github" / "check_test_shards.py" + +if not CHECKER.exists(): + pytest.skip( + "the .github folder is not part of an installed package", + allow_module_level=True, + ) + +_spec = importlib.util.spec_from_file_location("check_test_shards", CHECKER) +checker = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(checker) + +PROPERTY_SETTINGS = settings( + max_examples=200, + deadline=None, + derandomize=True, + suppress_health_check=[HealthCheck.too_slow, HealthCheck.function_scoped_fixture], +) + + +@pytest.fixture +def conftest(request): + path = TESTS_DIR / "conftest.py" + for plugin in request.config.pluginmanager.get_plugins(): + if getattr(plugin, "__file__", None) and Path(plugin.__file__) == path: + return plugin + raise AssertionError(f"{path} is not loaded as a plugin") + + +# Node ids as pytest writes them: a path, optional classes, a test name and +# optional parameters. pytest escapes non-ASCII and control characters in +# parameter ids, so the parameters here are any printable ASCII text, +# including "[", "]" and "::". +_identifier = st.from_regex(r"[A-Za-z_][A-Za-z0-9_]{0,12}", fullmatch=True) +_path = st.lists(_identifier, min_size=1, max_size=4).map( + lambda parts: "/".join(parts) + ".py" +) +_printable = st.characters(min_codepoint=0x20, max_codepoint=0x7E) +_params = st.one_of( + st.just(""), + st.text(alphabet=_printable, max_size=20).map(lambda text: f"[{text}]"), +) +nodeids = st.builds( + lambda path, classes, name, params: "::".join([path, *classes, name + params]), + _path, + st.lists(_identifier, max_size=2), + _identifier, + _params, +) +# Node ids that could come from one collection: distinct, and with distinct +# JUnit names, which pytest's own naming guarantees for real suites. +collections = st.lists(nodeids, max_size=30, unique_by=checker.junit_key) + + +def junit_cases(ids, outcome="passed"): + return [(checker.junit_key(nodeid), outcome) for nodeid in ids] + + +@PROPERTY_SETTINGS +@given(nodeid=nodeids, shards=st.integers(1, 64)) +@example(nodeid="policyengine_uk/tests/test_x.py::test_y", shards=3) +def test_shard_is_in_range_and_repeatable(conftest, nodeid, shards): + shard = conftest.shard_of(nodeid, shards) + assert 1 <= shard <= shards + assert shard == conftest.shard_of(nodeid, shards) + + +def test_shard_does_not_depend_on_the_process(conftest): + # Pinned values: every runner and pytest-xdist worker must agree, which + # Python's own hash, salted per process, would not give. + assert [ + conftest.shard_of("policyengine_uk/tests/test_x.py::test_y", shards) + for shards in (1, 2, 3, 4, 7) + ] == [1, 2, 1, 4, 7] + assert conftest.shard_of("policyengine_uk/tests/test_x.py::test_y[2026-a]", 3) == 2 + + +@PROPERTY_SETTINGS +@given(ids=st.lists(nodeids, max_size=40, unique=True), shards=st.integers(1, 8)) +def test_shards_partition_any_collection(conftest, ids, shards): + members = { + shard: [nodeid for nodeid in ids if conftest.shard_of(nodeid, shards) == shard] + for shard in range(1, shards + 1) + } + assert sorted(sum(members.values(), [])) == sorted(ids) + assert sum(len(group) for group in members.values()) == len(ids) + + +@PROPERTY_SETTINGS +@given(shards=st.integers(1, 200), data=st.data()) +def test_parse_shard_accepts_k_of_n(conftest, shards, data): + shard = data.draw(st.integers(1, shards)) + assert conftest.parse_shard(f"{shard}/{shards}") == (shard, shards) + + +@pytest.mark.parametrize( + "value", + [ + "0/3", + "4/3", + "1/0", + "0/0", + "3", + "1/3/5", + "a/b", + "1-3", + "", + " 1/3", + "1/3 ", + "-1/3", + ], +) +def test_parse_shard_rejects_anything_else(conftest, value): + with pytest.raises(argparse.ArgumentTypeError): + conftest.parse_shard(value) + + +@PROPERTY_SETTINGS +@given(nodeid=nodeids) +@example(nodeid="policyengine_uk/tests/test_x.py::TestA::test_y[a/b.yaml-1]") +@example(nodeid="policyengine_uk/tests/test_x.py::test_y[a::b[c]-d e]") +def test_junit_key_matches_pytest(nodeid): + # Differential test against pytest's own naming, written out to XML and + # read back as the check reads it. + junitxml = pytest.importorskip("_pytest.junitxml") + names = junitxml.mangle_test_address(nodeid) + element = ElementTree.Element( + "testcase", + classname=".".join(names[:-1]), + name=junitxml.bin_xml_escape(names[-1]), + ) + read_back = ElementTree.fromstring(ElementTree.tostring(element)) + assert checker.junit_key(nodeid) == ( + read_back.get("classname"), + read_back.get("name"), + ) + + +@PROPERTY_SETTINGS +@given(ids=collections, shards=st.integers(1, 5), data=st.data()) +def test_check_passes_exactly_when_every_test_ran_once(conftest, ids, shards, data): + ran = [ + junit_cases(i for i in ids if conftest.shard_of(i, shards) == shard) + for shard in range(1, shards + 1) + ] + problems, summary = checker.check(ids, ran) + assert problems == [] + assert summary.startswith(f"{len(ids)} tests collected") + + if not ids: + return + victim = data.draw(st.sampled_from(ids)) + key = checker.junit_key(victim) + home = conftest.shard_of(victim, shards) - 1 + + missing = [list(cases) for cases in ran] + missing[home].remove((key, "passed")) + problems, _ = checker.check(ids, missing) + assert [names for title, names in problems if "ran in no shard" in title] == [ + [victim] + ] + + repeated = [list(cases) for cases in ran] + repeated[data.draw(st.integers(0, shards - 1))].append((key, "passed")) + problems, _ = checker.check(ids, repeated) + assert [names for title, names in problems if "more than once" in title] == [ + [victim] + ] + + failed = [list(cases) for cases in ran] + failed[home][failed[home].index((key, "passed"))] = (key, "failed") + problems, _ = checker.check(ids, failed) + assert [names for title, names in problems if "failed" in title] == [ + [f"shard {home + 1}: {victim}"] + ] + + uncollected = [nodeid for nodeid in ids if nodeid != victim] + problems, _ = checker.check(uncollected, ran) + assert [names for title, names in problems if "not collected" in title] == [ + [f"{key[0]}::{key[1]}"] + ] + + +def test_read_collected_requires_the_listed_count(): + listing = "a/test_x.py::test_one\na/test_x.py::test_two[p]\n\n2 tests collected in 0.01s\n" + assert checker.read_collected(listing) == [ + "a/test_x.py::test_one", + "a/test_x.py::test_two[p]", + ] + assert checker.read_collected("\nno tests collected in 0.01s\n") == [] + # Warnings follow the listing and may mention a node id. + with_warnings = listing.replace( + "\n\n2 tests", + "\n\n=== warnings summary ===\na/test_x.py::test_one\n x\n2 tests", + ) + assert checker.read_collected(with_warnings) == checker.read_collected(listing) + for bad in ( + "a/test_x.py::test_one\n", + "a/test_x.py::test_one\n\n2 tests collected in 0.01s\n", + "a/test_x.py::test_one\na/test_x.py::test_one\n\n2 tests collected in 0.01s\n", + "a/test_x.py::test_one\n\n1 test collected, 1 error in 0.01s\n", + ): + with pytest.raises(checker.ShardCheckError): + checker.read_collected(bad) + + +SAMPLE_SUITE = """ +import pytest + + +@pytest.mark.parametrize("value", range(40)) +def test_number(value): + assert value >= 0 + + +@pytest.mark.parametrize( + "name", ["parameters/gov/a.yaml", "b c", "d[e]", "f::g", "h-1", "caf\\u00e9", "tab\\there"] +) +def test_name(name): + assert name + + +class TestGroup: + @pytest.mark.parametrize("value", ["x", "y"]) + def test_member(self, value): + assert value + + @pytest.mark.skip(reason="skipped tests still count as run") + def test_skipped(self): + raise AssertionError + + @pytest.mark.xfail(strict=True) + def test_expected_failure(self): + raise AssertionError +""" + + +def _pytest(cwd, *args): + # A clean run of pytest: no options, plugins or coverage settings carried + # over from the session that runs this test. + env = { + name: value + for name, value in os.environ.items() + if not name.startswith(("PYTEST_", "COV_CORE_", "COVERAGE_")) + } + return subprocess.run( + [sys.executable, "-m", "pytest", "-p", "no:cacheprovider", *args], + cwd=cwd, + env=env, + capture_output=True, + text=True, + timeout=600, + ) + + +@pytest.mark.parametrize("xdist", [(), ("-n", "2", "--dist", "worksteal")]) +def test_real_pytest_shards_pass_the_check(tmp_path, xdist): + suite = tmp_path / "suite" + suite.mkdir() + # The suite's real conftest.py, so this runs the --shard CI uses. + shutil.copy(TESTS_DIR / "conftest.py", suite / "conftest.py") + (suite / "test_sample.py").write_text(SAMPLE_SUITE, encoding="utf-8") + + listing = _pytest(tmp_path, "suite", "--collect-only", "-q") + assert listing.returncode == 0, listing.stdout + listing.stderr + collected = tmp_path / "collected.txt" + collected.write_text(listing.stdout, encoding="utf-8") + assert len(checker.read_collected(listing.stdout)) == 51 + + shards = 3 + junit = [] + for shard in range(1, shards + 1): + junit.append(tmp_path / f"junit-{shard}.xml") + run = _pytest( + tmp_path, + "suite", + "--shard", + f"{shard}/{shards}", + f"--junitxml={junit[-1]}", + *xdist, + ) + assert run.returncode == 0, run.stdout + run.stderr + assert f"shard: {shard} of {shards}" in run.stdout + + sizes = [len(checker.read_junit(path)) for path in junit] + assert sum(sizes) == 51 and all(sizes) + + def check(*paths): + return checker.main( + ["--collected", str(collected), "--junit", *map(str, paths)] + ) + + assert check(*junit) == 0 + assert check(*junit[:-1]) == 1 + assert check(*junit, junit[0]) == 1 + assert check(tmp_path / "absent.xml") == 2 diff --git a/policyengine_uk/tests/conftest.py b/policyengine_uk/tests/conftest.py index d611a0c1c5..ca783dd9ab 100644 --- a/policyengine_uk/tests/conftest.py +++ b/policyengine_uk/tests/conftest.py @@ -1,4 +1,6 @@ +import argparse import gc +import hashlib import os import re import sys @@ -29,7 +31,62 @@ def cloned_uk_tax_benefit_system(): yield +# CI splits the suite over several runners with ``--shard K/N``. The shard of a +# test is a function of its node id alone, so the shards partition whatever is +# collected: every test runs in exactly one of them, including a test added +# later. The Test job checks that on each run (.github/check_test_shards.py). +def shard_of(nodeid: str, shards: int) -> int: + """The shard, from 1 to ``shards``, that runs the test ``nodeid``. + + Uses SHA-256 rather than ``hash``, which Python salts per process: every + runner and every pytest-xdist worker must give the same answer. + """ + digest = hashlib.sha256(nodeid.encode("utf-8")).digest() + return int.from_bytes(digest[:8], "big") % shards + 1 + + +def parse_shard(value: str) -> tuple[int, int]: + """Parse ``K/N`` into ``(K, N)``, requiring 1 <= K <= N.""" + match = re.fullmatch(r"([0-9]+)/([0-9]+)", value) + if match: + shard, shards = int(match.group(1)), int(match.group(2)) + if 1 <= shard <= shards: + return shard, shards + raise argparse.ArgumentTypeError( + f"expected K/N with 1 <= K <= N, such as 1/3; got {value!r}" + ) + + +def pytest_addoption(parser): + parser.addoption( + "--shard", + type=parse_shard, + default=None, + metavar="K/N", + help="Run only shard K of N. Every test is in exactly one shard.", + ) + + +def pytest_report_header(config): + shard = config.getoption("shard", default=None) + if shard is not None: + return f"shard: {shard[0]} of {shard[1]}, chosen by node id" + + def pytest_collection_modifyitems(config, items): + shard = config.getoption("shard", default=None) + if shard is not None: + this_shard, shards = shard + selected, deselected = [], [] + for item in items: + if shard_of(item.nodeid, shards) == this_shard: + selected.append(item) + else: + deselected.append(item) + items[:] = selected + if deselected: + config.hook.pytest_deselected(items=deselected) + has_default_dataset = bool(os.environ.get("POLICYENGINE_UK_DEFAULT_DATASET")) if has_default_dataset: return From 841d2a8899000ea54773c878f3a83fbf810ea9b4 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sun, 11 Oct 2026 11:00:16 -0400 Subject: [PATCH 2/2] Run the final Test job on the shards' runners and report failed jobs first On 10 October the 5-second final Test job waited 32 minutes for an ubuntu-latest runner after the shards finished (run 38085920460). It now runs on ubuntu-24.04-arm like the shards. The job-result check runs first, and the shard check still runs after a failure so its report names the failed tests. Record the measured job times by the timeouts. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/code_changes.yaml | 24 ++++++++++++++++++------ .github/workflows/pr_code_changes.yaml | 24 ++++++++++++++++++------ 2 files changed, 36 insertions(+), 12 deletions(-) diff --git a/.github/workflows/code_changes.yaml b/.github/workflows/code_changes.yaml index bd414330c1..6d30dcf3b9 100644 --- a/.github/workflows/code_changes.yaml +++ b/.github/workflows/code_changes.yaml @@ -43,6 +43,7 @@ jobs: # queued for hours; the parameter tree and every test result are # identical on both. runs-on: ubuntu-24.04-arm + # About 4 minutes on 10 October (run 38085920460). timeout-minutes: 30 permissions: contents: "read" @@ -99,6 +100,9 @@ jobs: # a number to the list. name: Test (Python ${{ matrix.shard }}/${{ strategy.job-total }}) runs-on: ubuntu-24.04-arm + # The shards took 15, 20 and 15 minutes on 10 October (run + # 38085920460). 60 minutes leaves room for a slow runner and still + # stops a hung shard. timeout-minutes: 60 strategy: fail-fast: false @@ -164,20 +168,28 @@ jobs: # if the shards did not run every collected Python test exactly once. needs: [Test-YAML, Test-Python] if: ${{ !cancelled() }} - runs-on: ubuntu-latest + # The shards' runners, not ubuntu-latest: on 10 October this 5-second + # job waited 32 minutes for an ubuntu-latest runner after the shards + # had finished (run 38085920460). + runs-on: ubuntu-24.04-arm timeout-minutes: 10 steps: + - name: Check that every test job passed + run: | + echo "YAML: ${{ needs.Test-YAML.result }}; Python: ${{ needs.Test-Python.result }}" + test "${{ needs.Test-YAML.result }}" = success + test "${{ needs.Test-Python.result }}" = success - name: Checkout repo + if: ${{ !cancelled() }} uses: actions/checkout@v6 - name: Download the test lists and results + if: ${{ !cancelled() }} uses: actions/download-artifact@v8 with: pattern: test-* merge-multiple: true - name: Check that every Python test ran in exactly one shard + # Runs even when a test job failed, so its report names the + # failed tests. + if: ${{ !cancelled() }} run: python3 .github/check_test_shards.py --collected collected.txt --junit junit-*.xml - - name: Check that every test job passed - run: | - echo "YAML: ${{ needs.Test-YAML.result }}; Python: ${{ needs.Test-Python.result }}" - test "${{ needs.Test-YAML.result }}" = success - test "${{ needs.Test-Python.result }}" = success diff --git a/.github/workflows/pr_code_changes.yaml b/.github/workflows/pr_code_changes.yaml index 26d410db9d..2c8f588ba6 100644 --- a/.github/workflows/pr_code_changes.yaml +++ b/.github/workflows/pr_code_changes.yaml @@ -63,6 +63,7 @@ jobs: # queued for hours; the parameter tree and every test result are # identical on both. runs-on: ubuntu-24.04-arm + # About 4 minutes on 10 October (run 38085920460). timeout-minutes: 30 permissions: contents: "read" @@ -119,6 +120,9 @@ jobs: # a number to the list. name: Test (Python ${{ matrix.shard }}/${{ strategy.job-total }}) runs-on: ubuntu-24.04-arm + # The shards took 15, 20 and 15 minutes on 10 October (run + # 38085920460). 60 minutes leaves room for a slow runner and still + # stops a hung shard. timeout-minutes: 60 strategy: fail-fast: false @@ -182,20 +186,28 @@ jobs: # if the shards did not run every collected Python test exactly once. needs: [Test-YAML, Test-Python] if: ${{ !cancelled() }} - runs-on: ubuntu-latest + # The shards' runners, not ubuntu-latest: on 10 October this 5-second + # job waited 32 minutes for an ubuntu-latest runner after the shards + # had finished (run 38085920460). + runs-on: ubuntu-24.04-arm timeout-minutes: 10 steps: + - name: Check that every test job passed + run: | + echo "YAML: ${{ needs.Test-YAML.result }}; Python: ${{ needs.Test-Python.result }}" + test "${{ needs.Test-YAML.result }}" = success + test "${{ needs.Test-Python.result }}" = success - name: Checkout repo + if: ${{ !cancelled() }} uses: actions/checkout@v6 - name: Download the test lists and results + if: ${{ !cancelled() }} uses: actions/download-artifact@v8 with: pattern: test-* merge-multiple: true - name: Check that every Python test ran in exactly one shard + # Runs even when a test job failed, so its report names the + # failed tests. + if: ${{ !cancelled() }} run: python3 .github/check_test_shards.py --collected collected.txt --junit junit-*.xml - - name: Check that every test job passed - run: | - echo "YAML: ${{ needs.Test-YAML.result }}; Python: ${{ needs.Test-Python.result }}" - test "${{ needs.Test-YAML.result }}" = success - test "${{ needs.Test-Python.result }}" = success