diff --git a/.github/workflows/aggregate-impact.yaml b/.github/workflows/aggregate-impact.yaml index 7740c688a2..fe842b9637 100644 --- a/.github/workflows/aggregate-impact.yaml +++ b/.github/workflows/aggregate-impact.yaml @@ -1,6 +1,8 @@ # Measure a pull request's base and head on the certified dataset and post # the comparison: receipts by tax head, spending and caseloads, poverty, # inequality, deciles and the standard reform costings, 2025-26 to 2031-32. +# For a pull request, the head is GitHub's merge commit and the base is that +# commit's first parent, so the report is exactly what merging would change. # # Both sides run the pull request head's copy of the measurement tool # (policyengine_uk/utils/aggregate_impact), each in an environment synced @@ -66,16 +68,15 @@ jobs: id: commits env: EVENT: ${{ github.event_name }} - PR_BASE: ${{ github.event.pull_request.base.sha }} PR_BASE_REF: ${{ github.event.pull_request.base.ref }} PR_NUMBER: ${{ github.event.pull_request.number }} INPUT_BASE: ${{ inputs.base }} INPUT_HEAD: ${{ inputs.head }} run: | - # A pull request's head is its merge commit, whose first parent is - # the base, so the comparison is exactly what merging would change. + # A pull request's head is its merge commit. Its base is read from + # that commit once the head is checked out (Choose the base). if [ "$EVENT" = "pull_request" ]; then - base="$PR_BASE"; head="$GITHUB_SHA" + base=""; head="$GITHUB_SHA" base_name="$PR_BASE_REF"; head_name="#$PR_NUMBER merged into $PR_BASE_REF" else base="${INPUT_BASE:-main}"; head="${INPUT_HEAD:-$GITHUB_SHA}" @@ -93,11 +94,39 @@ jobs: with: ref: ${{ steps.commits.outputs.head }} path: head + # A pull request's merge commit and its parents, so that the next + # step can read the base from it. + fetch-depth: 2 + + - name: Choose the base + id: base + env: + EVENT: ${{ github.event_name }} + BASE: ${{ steps.commits.outputs.base }} + run: | + # A pull request's base is its merge commit's first parent: the tip + # of the branch it merges into, as GitHub made the merge commit for + # this run. The comparison is then exactly what merging would + # change. The event's pull_request.base.sha is not that commit: it + # lags the branch once the branch has moved on, and the report then + # counted the branch's newer merges as the pull request's impact + # (#2516's took in 26 commits from main). + if [ "$EVENT" = "pull_request" ]; then + # The merge commit, then its parents. + line=$(git -C head rev-list --parents -n 1 HEAD) + read -r -a commit <<< "$line" + if [ "${#commit[@]}" -ne 3 ]; then + echo "::error::The head ${commit[0]} has $((${#commit[@]} - 1)) parents, not 2, so it is not a pull request's merge commit and its base is unknown." + exit 1 + fi + BASE="${commit[1]}" + fi + echo "sha=$BASE" >> "$GITHUB_OUTPUT" - name: Check out the base uses: actions/checkout@v6 with: - ref: ${{ steps.commits.outputs.base }} + ref: ${{ steps.base.outputs.sha }} path: base - name: Install uv diff --git a/changelog.d/aggregate-impact-merge-parent.fixed.md b/changelog.d/aggregate-impact-merge-parent.fixed.md new file mode 100644 index 0000000000..09a3a244bd --- /dev/null +++ b/changelog.d/aggregate-impact-merge-parent.fixed.md @@ -0,0 +1 @@ +- The Aggregate impact report now compares a pull request's merge commit with that commit's first parent, the tip of the branch it merges into. It used the event's `pull_request.base.sha`, which lags that branch once it moves on, so newer merges to `main` were counted as the pull request's impact. diff --git a/policyengine_uk/tests/code_health/test_aggregate_impact_workflow.py b/policyengine_uk/tests/code_health/test_aggregate_impact_workflow.py index b9554b3e52..503f4a414e 100644 --- a/policyengine_uk/tests/code_health/test_aggregate_impact_workflow.py +++ b/policyengine_uk/tests/code_health/test_aggregate_impact_workflow.py @@ -1,17 +1,41 @@ -"""Data protection: only aggregates may leave the aggregate-impact job. +"""What the aggregate-impact job compares, and what may leave it. -The certified dataset is licensed survey microdata. The measurement keeps -household-level detail for the decile comparison under ``private/``; these -tests fail if the workflow could publish it, or could put the dataset where -other workflows can read it. +Comparison base. For a pull request the head is GitHub's merge commit and +the base is that commit's first parent, so the report is exactly what +merging would change. The event's ``pull_request.base.sha`` lags the base +branch once the branch moves on, and a report against it counted main's +newer merges as the pull request's impact (#2516's took in 26 commits from +main). These tests run the workflow's own step scripts on real histories, +with the head fetched as actions/checkout fetches it at the workflow's +fetch depth, and hold the job to: + +1. The base is the merge commit's first parent for every shape of history, + so no commit of the base branch is counted as the pull request's. +2. A head that is not a merge of two commits fails the job with an error + and sets no base. +3. No step reads ``pull_request.base.sha``, and the base checkout uses the + base the job chose. +4. A manual dispatch compares the commits it names, as before. + +Data protection. Only aggregates may leave the job. The certified dataset +is licensed survey microdata. The measurement keeps household-level detail +for the decile comparison under ``private/``; these tests fail if the +workflow could publish it, or could put the dataset where other workflows +can read it. """ import json +import os +import re +import subprocess +import tempfile from pathlib import Path import numpy as np import pytest import yaml +from hypothesis import HealthCheck, given, settings +from hypothesis import strategies as st from policyengine_uk.utils.aggregate_impact.measure import ( HOUSEHOLDS_FILE, @@ -95,3 +119,243 @@ def test_a_failed_run_always_replaces_an_earlier_comment(): assert post["if"] == "always() && github.event_name == 'pull_request'" assert "if [ ! -s report/comment.md ]" in post["run"] assert "FAIL" in post["run"] + + +# The comparison base. + +GIT_ENV = { + **os.environ, + "GIT_CONFIG_GLOBAL": os.devnull, + "GIT_CONFIG_NOSYSTEM": "1", + "GIT_AUTHOR_NAME": "t", + "GIT_AUTHOR_EMAIL": "t@example.com", + "GIT_COMMITTER_NAME": "t", + "GIT_COMMITTER_EMAIL": "t@example.com", +} +SHA = "0123456789abcdef0123456789abcdef01234567" + + +def step_named(name): + return next(s for s in steps() if s.get("name") == name) + + +def git(repo, *args): + return subprocess.run( + ["git", "-C", str(repo), *args], + check=True, + capture_output=True, + text=True, + env=GIT_ENV, + ).stdout.strip() + + +def run_step(name, workspace, **env): + """Run a step's script in ``workspace`` as Actions runs it. + + Each variable in the step's ``env`` is set, empty unless given, as an + expression with no value is. Returns the process and the outputs the + step wrote to ``GITHUB_OUTPUT``. + """ + step = step_named(name) + outputs = Path(workspace) / "github_output" + outputs.write_text("") + variables = {key: "" for key in step.get("env", {})} + variables.update(env) + process = subprocess.run( + ["bash", "--noprofile", "--norc", "-eo", "pipefail", "-c", step["run"]], + cwd=workspace, + env={**GIT_ENV, **variables, "GITHUB_OUTPUT": str(outputs)}, + capture_output=True, + text=True, + ) + written = dict(line.split("=", 1) for line in outputs.read_text().splitlines()) + return process, written + + +class Origin: + """A repository on GitHub's side, holding commits made directly.""" + + def __init__(self, path): + path.mkdir(parents=True) + git(path, "init", "-q") + # actions/checkout fetches the head by its SHA. + git(path, "config", "uploadpack.allowAnySHA1InWant", "true") + self.path = path + self.tree = git(path, "write-tree") + self.made = 0 + + def commit(self, *parents): + self.made += 1 + flags = [flag for parent in parents for flag in ("-p", parent)] + return git( + self.path, "commit-tree", self.tree, *flags, "-m", f"commit {self.made}" + ) + + def attributed(self, base, head): + """The commits a comparison of ``base`` with ``head`` counts.""" + return set(git(self.path, "rev-list", f"{base}..{head}").split()) + + +def check_out_the_head(workspace, origin, sha): + """Fetch ``sha`` into ``workspace/head`` as the workflow's checkout does.""" + depth = step_named("Check out the head")["with"].get("fetch-depth", 1) + head = Path(workspace) / "head" + head.mkdir(parents=True) + git(head, "init", "-q") + git(head, "fetch", "-q", "--no-tags", f"--depth={depth}", origin.path.as_uri(), sha) + git(head, "checkout", "-q", "--detach", sha) + + +def choose_the_base(workspace, origin, merge): + check_out_the_head(workspace, origin, merge) + return run_step("Choose the base", workspace, EVENT="pull_request") + + +def test_a_pull_request_is_compared_with_its_merge_commits_first_parent(tmp_path): + origin = Origin(tmp_path / "origin") + # The pull request branches from main at `forked`. The event's + # pull_request.base.sha can still name it after main has moved on. + forked = origin.commit(origin.commit()) + change = origin.commit(forked) + # Main moves on: two later merges. + tip = origin.commit(origin.commit(forked)) + # GitHub's merge commit for the run: main's tip, then the pull request. + merge = origin.commit(tip, change) + + process, outputs = choose_the_base(tmp_path / "runner", origin, merge) + + assert process.returncode == 0, process.stdout + process.stderr + assert outputs == {"sha": tip} + assert origin.attributed(outputs["sha"], merge) == {merge, change} + # Against `forked`, main's two later merges counted as the pull request's. + assert len(origin.attributed(forked, merge) - {merge, change}) == 2 + + +@pytest.mark.parametrize("parents", [0, 1, 3]) +def test_a_head_that_does_not_merge_two_commits_has_no_base(tmp_path, parents): + origin = Origin(tmp_path / "origin") + head = origin.commit(*[origin.commit() for _ in range(parents)]) + + process, outputs = choose_the_base(tmp_path / "runner", origin, head) + + assert process.returncode != 0 + assert f"::error::The head {head} has {parents} parents, not 2" in process.stdout + assert outputs == {} + + +def test_the_events_base_sha_is_not_the_comparison_base(): + expressions = re.findall(r"\$\{\{(.*?)\}\}", WORKFLOW.read_text(), re.DOTALL) + assert expressions + assert not [e for e in expressions if "pull_request.base.sha" in e] + names = [s.get("name") for s in steps()] + order = [ + names.index("Choose the commits"), + names.index("Check out the head"), + names.index("Choose the base"), + names.index("Check out the base"), + ] + assert order == sorted(order) + head = step_named("Check out the head")["with"] + assert head["ref"] == "${{ steps.commits.outputs.head }}" + choose = step_named("Choose the base") + assert choose["id"] == "base" + assert choose["env"]["BASE"] == "${{ steps.commits.outputs.base }}" + assert step_named("Check out the base")["with"]["ref"] == ( + "${{ steps.base.outputs.sha }}" + ) + + +def test_a_pull_request_head_is_its_merge_commit(tmp_path): + process, outputs = run_step( + "Choose the commits", + tmp_path, + EVENT="pull_request", + GITHUB_SHA=SHA, + PR_BASE_REF="main", + PR_NUMBER="2516", + ) + assert process.returncode == 0, process.stdout + process.stderr + assert outputs == { + "base": "", + "head": SHA, + "base_name": "main", + "head_name": "#2516 merged into main", + } + + +@pytest.mark.parametrize( + "inputs, expected", + [ + ( + {}, + {"base": "main", "head": SHA, "base_name": "main", "head_name": "dev"}, + ), + ( + {"INPUT_BASE": "2.139.0", "INPUT_HEAD": "fix"}, + { + "base": "2.139.0", + "head": "fix", + "base_name": "2.139.0", + "head_name": "fix", + }, + ), + ], +) +def test_a_dispatch_compares_the_commits_it_names(tmp_path, inputs, expected): + process, outputs = run_step( + "Choose the commits", + tmp_path, + EVENT="workflow_dispatch", + GITHUB_SHA=SHA, + GITHUB_REF_NAME="dev", + **inputs, + ) + assert process.returncode == 0, process.stdout + process.stderr + assert outputs == expected + # The base passes through, with no head to read it from. + process, outputs = run_step( + "Choose the base", tmp_path, EVENT="workflow_dispatch", BASE=expected["base"] + ) + assert process.returncode == 0, process.stdout + process.stderr + assert outputs == {"sha": expected["base"]} + + +@settings( + max_examples=25, + deadline=None, + derandomize=True, + suppress_health_check=[HealthCheck.too_slow], +) +@given( + before=st.integers(0, 2), + events=st.lists(st.sampled_from(["main", "pull request", "update"]), max_size=8), +) +def test_no_commit_of_the_base_branch_counts_as_the_pull_requests(before, events): + """Invariant 1, for any history: main's commits before and after the + fork, the pull request's commits, and merges of main into the pull + request ("Update branch").""" + with tempfile.TemporaryDirectory() as directory: + origin = Origin(Path(directory) / "origin") + main = origin.commit() + for _ in range(before): + main = origin.commit(main) + pull_request = origin.commit(main) + own = {pull_request} + merged = main + for event in events: + if event == "main": + main = origin.commit(main) + elif event == "pull request": + pull_request = origin.commit(pull_request) + own.add(pull_request) + elif main != merged: + pull_request = origin.commit(pull_request, main) + own.add(pull_request) + merged = main + merge = origin.commit(main, pull_request) + + process, outputs = choose_the_base(Path(directory) / "runner", origin, merge) + + assert process.returncode == 0, process.stdout + process.stderr + assert outputs == {"sha": main} + assert origin.attributed(outputs["sha"], merge) == own | {merge}