From 1a80daa28b615da9de0f9cca4a0a3fbb945d3dc5 Mon Sep 17 00:00:00 2001 From: Max Horn Date: Thu, 7 May 2026 21:43:01 +0100 Subject: [PATCH 1/2] ci: add GAP PR package test workflow Add a workflow that accepts a GAP pull request URL, resolves its head revision, runs the package test suite against that exact commit, and posts the resulting report back to the PR. Teach the shared test workflow and report tooling about ad hoc report keys, labels, baselines, and PR-specific failure summaries so these runs compare against the correct branch. AI assistance: Codex implemented the workflows, Python helpers, tests, and documentation updates. Co-authored-by: Codex --- .github/workflows/test-all.yml | 84 +++++++- .github/workflows/test-gap-pr.yml | 200 ++++++++++++++++++ README.md | 16 ++ tools/gap_pr_utils.py | 101 +++++++++ tools/generate_report.py | 5 + tools/generate_test_status.py | 18 +- tools/render_gap_pr_test_comment.py | 124 +++++++++++ tools/tests/test_gap_pr_utils.py | 92 ++++++++ tools/tests/test_generate_report.py | 23 ++ .../tests/test_render_gap_pr_test_comment.py | 89 ++++++++ 10 files changed, 738 insertions(+), 14 deletions(-) create mode 100644 .github/workflows/test-gap-pr.yml create mode 100644 tools/gap_pr_utils.py create mode 100644 tools/render_gap_pr_test_comment.py create mode 100644 tools/tests/test_gap_pr_utils.py create mode 100644 tools/tests/test_render_gap_pr_test_comment.py diff --git a/.github/workflows/test-all.yml b/.github/workflows/test-all.yml index 2eede6d3c..59af629bb 100644 --- a/.github/workflows/test-all.yml +++ b/.github/workflows/test-all.yml @@ -26,6 +26,31 @@ on: required: false type: string default: https://github.com/gap-system/gap.git + which-gap-sha: + description: 'Exact GAP commit SHA expected at the branch head' + required: false + type: string + default: "" + report-key: + description: 'Identifier used for artifacts and report paths' + required: false + type: string + default: "" + report-label: + description: 'Human-readable GAP label for the generated report' + required: false + type: string + default: "" + baseline-key: + description: 'Baseline key used for latest-* comparisons' + required: false + type: string + default: "" + job-name-prefix: + description: 'Reusable workflow job prefix override' + required: false + type: string + default: "" pkg-build-glob: description: 'Only build packages matching the given glob' required: false @@ -53,6 +78,31 @@ on: required: false type: string default: https://github.com/gap-system/gap.git + which-gap-sha: + description: 'Exact GAP commit SHA expected at the branch head' + required: false + type: string + default: "" + report-key: + description: 'Identifier used for artifacts and report paths' + required: false + type: string + default: "" + report-label: + description: 'Human-readable GAP label for the generated report' + required: false + type: string + default: "" + baseline-key: + description: 'Baseline key used for latest-* comparisons' + required: false + type: string + default: "" + job-name-prefix: + description: 'Reusable workflow job prefix override' + required: false + type: string + default: "" pkg-build-glob: description: 'Only build packages matching the given glob' required: false @@ -130,16 +180,25 @@ jobs: echo "::group::fetch" whichgap="${{ github.event.inputs.which-gap || inputs.which-gap }}" whichgaprepo="${{ github.event.inputs.which-gap-repo || inputs.which-gap-repo }}" - if [[ "${whichgap}" == 4.* ]]; then + whichgapsha="${{ github.event.inputs.which-gap-sha || inputs.which-gap-sha }}" + if [[ "${whichgap}" == 4.* && -z "${whichgapsha}" ]]; then # assume it is a tag wget --quiet https://github.com/gap-system/gap/releases/download/v${whichgap}/gap-${whichgap}-core.tar.gz tar xf gap-${whichgap}-core.tar.gz rm gap-${whichgap}-core.tar.gz mv gap-${whichgap} $HOME/gap else - git clone --depth=2 -b ${whichgap} ${whichgaprepo} $HOME/gap + git clone --depth=1 -b "${whichgap}" "${whichgaprepo}" $HOME/gap fi cd $HOME/gap + if [[ -n "${whichgapsha}" ]]; then + actualsha=$(git rev-parse HEAD) + if [[ "${actualsha}" != "${whichgapsha}" ]]; then + echo "Expected GAP branch head ${whichgapsha}, got ${actualsha}." + echo "The pull request changed after this run was requested." + exit 1 + fi + fi echo "::endgroup::" echo "::group::autogen" @@ -209,7 +268,8 @@ jobs: - name: "Compute suitable artifact basename" id: get-artifact-name run: | - echo "artifact=${{ github.event.inputs.which-gap || inputs.which-gap }}" | sed -e 's;/;-;g' >> $GITHUB_OUTPUT + artifact_key="${{ github.event.inputs.report-key || inputs.report-key || github.event.inputs.which-gap || inputs.which-gap }}" + echo "artifact=${artifact_key}" | sed -e 's;/;-;g' >> $GITHUB_OUTPUT - name: "Upload GAP with packages as artifact" uses: actions/upload-artifact@v7 @@ -369,15 +429,18 @@ jobs: - name: "Generate test-status.json" id: test-status run: | - # Is this a workflow_call? - if [ "${{inputs.which-gap}}" != "" ]; then - JOB_NAME_PREFIX="${{inputs.which-gap}} / " + REPORT_KEY="${{ github.event.inputs.report-key || inputs.report-key || github.event.inputs.which-gap || inputs.which-gap }}" + REPORT_LABEL="${{ github.event.inputs.report-label || inputs.report-label || github.event.inputs.which-gap || inputs.which-gap }}" + if [ "${{ inputs.job-name-prefix }}" != "" ]; then + JOB_NAME_PREFIX="${{ inputs.job-name-prefix }} / " + elif [ "${{ inputs.which-gap }}" != "" ]; then + JOB_NAME_PREFIX="${{ inputs.which-gap }} / " else JOB_NAME_PREFIX="" fi ROOT='data/reports' # relative path (with respect to ROOT) to generated test-status.json, i.e. the "id" entry of the json-file. - DIR_REL=$(tools/generate_test_status.py ${{secrets.GITHUB_TOKEN}} ${{ github.repository }} "$GITHUB_RUN_ID" "$GITHUB_SHA" ${{ github.event.inputs.which-gap || inputs.which-gap }} "$JOB_NAME_PREFIX") + DIR_REL=$(tools/generate_test_status.py ${{secrets.GITHUB_TOKEN}} ${{ github.repository }} "$GITHUB_RUN_ID" "$GITHUB_SHA" "${REPORT_KEY}" "${REPORT_LABEL}" "${JOB_NAME_PREFIX}") DIR="${ROOT}/${DIR_REL}" echo "dir=${DIR}" >> $GITHUB_OUTPUT echo "dir-rel=${DIR_REL}" >> $GITHUB_OUTPUT @@ -386,7 +449,8 @@ jobs: id: download-latest-report run: | ROOT="data/reports" - DIR_SYM_REL=latest-${{ needs.build.outputs.artifact }} + BASELINE_KEY="${{ github.event.inputs.baseline-key || inputs.baseline-key || github.event.inputs.report-key || inputs.report-key || github.event.inputs.which-gap || inputs.which-gap }}" + DIR_SYM_REL=latest-${BASELINE_KEY} DIR_SYM="${ROOT}/${DIR_SYM_REL}" URL_SYM="https://raw.githubusercontent.com/${{ github.repository }}/${DIR_SYM}" # Check if file at url exists, @@ -405,7 +469,9 @@ jobs: - name: "Generate report" id: report - run: tools/generate_report.py ${{ steps.test-status.outputs.dir-rel }} latest-${{ needs.build.outputs.artifact }} + run: | + BASELINE_KEY="${{ github.event.inputs.baseline-key || inputs.baseline-key || github.event.inputs.report-key || inputs.report-key || github.event.inputs.which-gap || inputs.which-gap }}" + tools/generate_report.py ${{ steps.test-status.outputs.dir-rel }} latest-${BASELINE_KEY} - name: "Upload report as artifact" uses: actions/upload-artifact@v7 diff --git a/.github/workflows/test-gap-pr.yml b/.github/workflows/test-gap-pr.yml new file mode 100644 index 000000000..331994bba --- /dev/null +++ b/.github/workflows/test-gap-pr.yml @@ -0,0 +1,200 @@ +# +# This workflow resolves a GAP pull request URL, runs the full PackageDistro +# test suite against the pull request head, and posts a summary comment back +# to the pull request. +# +name: "Test GAP pull request" + +on: + workflow_dispatch: + inputs: + gap-pr-url: + description: 'URL of a pull request on gap-system/gap' + required: true + type: string + pkg-build-glob: + description: 'Only build packages matching the given glob' + required: false + type: string + default: "*" + pkg-test-glob: + description: 'Only test packages matching the given glob' + required: false + type: string + default: "*" + callback-repo: + description: 'Optional repo to comment on, e.g. gap-system/gap' + required: false + type: string + default: "" + callback-pr-number: + description: 'Optional PR number override for callback comments' + required: false + type: string + default: "" + callback-comment-id: + description: 'Optional existing comment id to update' + required: false + type: string + default: "" + requester: + description: 'Optional login of the user who requested the run' + required: false + type: string + default: "" + command: + description: 'Optional command string that triggered the run' + required: false + type: string + default: "" + +concurrency: + group: ${{ github.workflow }}-${{ github.event.inputs.gap-pr-url }} + cancel-in-progress: true + +jobs: + resolve: + name: "Resolve GAP pull request" + runs-on: ubuntu-slim + outputs: + repo-owner: ${{ steps.resolve.outputs.repo-owner }} + repo-name: ${{ steps.resolve.outputs.repo-name }} + repo-full-name: ${{ steps.resolve.outputs.repo-full-name }} + pr-number: ${{ steps.resolve.outputs.pr-number }} + pr-url: ${{ steps.resolve.outputs.pr-url }} + base-ref: ${{ steps.resolve.outputs.base-ref }} + head-ref: ${{ steps.resolve.outputs.head-ref }} + head-sha: ${{ steps.resolve.outputs.head-sha }} + head-repo-clone-url: ${{ steps.resolve.outputs.head-repo-clone-url }} + report-key: ${{ steps.resolve.outputs.report-key }} + report-label: ${{ steps.resolve.outputs.report-label }} + report-artifact: ${{ steps.resolve.outputs.report-artifact }} + job-name-prefix: ${{ steps.resolve.outputs.job-name-prefix }} + callback-repo-owner: ${{ steps.resolve.outputs.callback-repo-owner }} + callback-repo-name: ${{ steps.resolve.outputs.callback-repo-name }} + callback-pr-number: ${{ steps.resolve.outputs.callback-pr-number }} + callback-comment-id: ${{ steps.resolve.outputs.callback-comment-id }} + requester: ${{ steps.resolve.outputs.requester }} + command: ${{ steps.resolve.outputs.command }} + steps: + - uses: actions/checkout@v6 + + - name: "Set up Python" + uses: actions/setup-python@v6 + with: + python-version: "3.11" + + - name: "Install package distribution tools" + run: python -m pip install -r tools/requirements.txt + + - name: "Resolve GAP PR metadata" + id: resolve + run: | + python tools/gap_pr_utils.py "${{ github.event.inputs.gap-pr-url }}" > resolved.json + + callback_repo="${{ github.event.inputs.callback-repo }}" + if [[ -n "${callback_repo}" ]]; then + callback_repo_owner="${callback_repo%%/*}" + callback_repo_name="${callback_repo##*/}" + else + callback_repo_owner=$(jq -r '.repo_owner' resolved.json) + callback_repo_name=$(jq -r '.repo_name' resolved.json) + fi + + callback_pr_number="${{ github.event.inputs.callback-pr-number }}" + if [[ -z "${callback_pr_number}" ]]; then + callback_pr_number=$(jq -r '.number' resolved.json) + fi + + echo "repo-owner=$(jq -r '.repo_owner' resolved.json)" >> $GITHUB_OUTPUT + echo "repo-name=$(jq -r '.repo_name' resolved.json)" >> $GITHUB_OUTPUT + echo "repo-full-name=$(jq -r '.repo_full_name' resolved.json)" >> $GITHUB_OUTPUT + echo "pr-number=$(jq -r '.number' resolved.json)" >> $GITHUB_OUTPUT + echo "pr-url=$(jq -r '.pr_url' resolved.json)" >> $GITHUB_OUTPUT + echo "base-ref=$(jq -r '.base_ref' resolved.json)" >> $GITHUB_OUTPUT + echo "head-ref=$(jq -r '.head_ref' resolved.json)" >> $GITHUB_OUTPUT + echo "head-sha=$(jq -r '.head_sha' resolved.json)" >> $GITHUB_OUTPUT + echo "head-repo-clone-url=$(jq -r '.head_repo_clone_url' resolved.json)" >> $GITHUB_OUTPUT + echo "report-key=$(jq -r '.report_key' resolved.json)" >> $GITHUB_OUTPUT + echo "report-label=$(jq -r '.report_label' resolved.json)" >> $GITHUB_OUTPUT + echo "report-artifact=$(jq -r '.report_key' resolved.json)" >> $GITHUB_OUTPUT + echo "job-name-prefix=$(jq -r '.job_name_prefix' resolved.json)" >> $GITHUB_OUTPUT + echo "callback-repo-owner=${callback_repo_owner}" >> $GITHUB_OUTPUT + echo "callback-repo-name=${callback_repo_name}" >> $GITHUB_OUTPUT + echo "callback-pr-number=${callback_pr_number}" >> $GITHUB_OUTPUT + echo "callback-comment-id=${{ github.event.inputs.callback-comment-id }}" >> $GITHUB_OUTPUT + echo "requester=${{ github.event.inputs.requester }}" >> $GITHUB_OUTPUT + echo "command=${{ github.event.inputs.command }}" >> $GITHUB_OUTPUT + + test-gap-pr: + name: ${{ needs.resolve.outputs.job-name-prefix }} + needs: resolve + uses: ./.github/workflows/test-all.yml + with: + which-gap: ${{ needs.resolve.outputs.head-ref }} + which-gap-repo: ${{ needs.resolve.outputs.head-repo-clone-url }} + which-gap-sha: ${{ needs.resolve.outputs.head-sha }} + report-key: ${{ needs.resolve.outputs.report-key }} + report-label: ${{ needs.resolve.outputs.report-label }} + baseline-key: ${{ needs.resolve.outputs.base-ref }} + job-name-prefix: ${{ needs.resolve.outputs.job-name-prefix }} + pkg-build-glob: ${{ github.event.inputs.pkg-build-glob }} + pkg-test-glob: ${{ github.event.inputs.pkg-test-glob }} + + comment: + name: "Comment on GAP pull request" + needs: [resolve, test-gap-pr] + if: always() + runs-on: ubuntu-slim + steps: + - uses: actions/checkout@v6 + + - name: "Set up Python" + uses: actions/setup-python@v6 + with: + python-version: "3.11" + + - name: "Install package distribution tools" + run: python -m pip install -r tools/requirements.txt + + - name: "Download report" + uses: actions/download-artifact@v8 + with: + name: report-${{ needs.resolve.outputs.report-artifact }} + path: _report + + - name: "Build comment body" + run: | + mkdir -p _comment + cat > _comment/metadata.json < _comment/body.md + + - uses: actions/create-github-app-token@v3 + id: generate-token + with: + client-id: ${{ secrets.APP_ID }} + private-key: ${{ secrets.APP_PRIVATE_KEY }} + owner: ${{ needs.resolve.outputs.callback-repo-owner }} + repositories: ${{ needs.resolve.outputs.callback-repo-name }} + + - name: "Create or update comment" + uses: peter-evans/create-or-update-comment@v5 + with: + token: ${{ steps.generate-token.outputs.token }} + issue-number: ${{ needs.resolve.outputs.callback-pr-number }} + comment-id: ${{ needs.resolve.outputs.callback-comment-id }} + body-path: _comment/body.md + edit-mode: replace diff --git a/README.md b/README.md index e75bd7149..700669a0d 100644 --- a/README.md +++ b/README.md @@ -82,6 +82,22 @@ a report is added to the PR which indicates whether the new package breaks somet in GAP or other packages, and whether its tests pass. If all looks good, the PR may be merged by any maintainer. +### Testing GAP pull requests against all packages + +Maintainers can also run the full PackageDistro test suite against a pull +request on `gap-system/gap`. + +1. Go to + +2. Click "Run workflow". +3. Paste the GAP pull request URL into the `gap-pr-url` field. +4. Optionally restrict the package build or test glob. + +The workflow resolves the pull request automatically, checks out the exact +head revision, compares the results against the latest report for the pull +request's target branch, and posts a summary comment back to the GAP pull +request. + ## How everything works diff --git a/tools/gap_pr_utils.py b/tools/gap_pr_utils.py new file mode 100644 index 000000000..ffc4d63ed --- /dev/null +++ b/tools/gap_pr_utils.py @@ -0,0 +1,101 @@ +#!/usr/bin/env python3 + +############################################################################# +## +## This file is part of GAP, a system for computational discrete algebra. +## +## Copyright of GAP belongs to its developers, whose names are too numerous +## to list here. Please refer to the COPYRIGHT file for details. +## +## SPDX-License-Identifier: GPL-2.0-or-later +## + +import json +import re +import sys +from typing import Any, Callable, Dict, Tuple + +import requests +from utils import error + +GAP_PR_URL_RE = re.compile( + r"^https://github\.com/(?P[^/]+)/(?P[^/]+)/pull/(?P\d+)" + r"(?:[/?#].*)?$" +) + + +def github_headers(token: str | None) -> Dict[str, str]: + headers = {"Accept": "application/vnd.github+json"} + if token: + headers["Authorization"] = f"Bearer {token}" + return headers + + +def parse_gap_pull_request_url(url: str) -> Tuple[str, str, int]: + match = GAP_PR_URL_RE.match(url) + if match is None: + raise ValueError("Expected a GAP pull request URL on github.com") + owner = match.group("owner") + repo = match.group("repo") + if (owner, repo) != ("gap-system", "gap"): + raise ValueError("Expected a GAP pull request URL for gap-system/gap") + return owner, repo, int(match.group("number")) + + +def build_report_key(owner: str, repo: str, number: int, head_sha: str) -> str: + return f"pr-{owner}-{repo}-{number}-{head_sha[:8]}".lower() + + +def build_report_label( + owner: str, repo: str, number: int, head_sha: str, base_ref: str +) -> str: + return f"{owner}/{repo}#{number} @ {head_sha[:8]} (base {base_ref})" + + +def resolve_gap_pull_request( + url: str, + token: str | None = None, + get: Callable[..., requests.Response] = requests.get, +) -> Dict[str, Any]: + owner, repo, number = parse_gap_pull_request_url(url) + api_url = f"https://api.github.com/repos/{owner}/{repo}/pulls/{number}" + response = get(api_url, headers=github_headers(token)) + response.raise_for_status() + payload = response.json() + + base_ref = payload["base"]["ref"] + head_sha = payload["head"]["sha"] + normalized = { + "pr_url": f"https://github.com/{owner}/{repo}/pull/{number}", + "api_url": api_url, + "repo_owner": owner, + "repo_name": repo, + "repo_full_name": f"{owner}/{repo}", + "number": number, + "title": payload["title"], + "base_ref": base_ref, + "base_repo_full_name": payload["base"]["repo"]["full_name"], + "base_repo_clone_url": payload["base"]["repo"]["clone_url"], + "head_ref": payload["head"]["ref"], + "head_sha": head_sha, + "head_repo_full_name": payload["head"]["repo"]["full_name"], + "head_repo_clone_url": payload["head"]["repo"]["clone_url"], + "head_repo_html_url": payload["head"]["repo"]["html_url"], + "report_key": build_report_key(owner, repo, number, head_sha), + "report_label": build_report_label(owner, repo, number, head_sha, base_ref), + "job_name_prefix": f"PR #{number}", + } + return normalized + + +def main(argv: list[str]) -> int: + if len(argv) not in (2, 3): + error("usage: gap_pr_utils.py GAP_PR_URL [GITHUB_TOKEN]") + token = argv[2] if len(argv) == 3 else None + resolved = resolve_gap_pull_request(argv[1], token=token) + print(json.dumps(resolved, ensure_ascii=False, indent=2)) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main(sys.argv)) diff --git a/tools/generate_report.py b/tools/generate_report.py index 7f7544faf..49144839d 100755 --- a/tools/generate_report.py +++ b/tools/generate_report.py @@ -100,6 +100,11 @@ and pkgs[pkg]["status"] == status ] +report_diff["failures_changed"] = sorted(pkgs_changed["failure"]) +report_diff["failures_current"] = sorted( + pkg for pkg, data in pkgs.items() if data["status"] == "failure" +) + def write_details_list( f: io.TextIOWrapper, pkgnames: List[str], pkgs: Dict[str, Any] diff --git a/tools/generate_test_status.py b/tools/generate_test_status.py index 200c9a26c..8de1847f8 100755 --- a/tools/generate_test_status.py +++ b/tools/generate_test_status.py @@ -90,15 +90,20 @@ def main(argv: List[str]) -> int: # Arguments num_args = len(argv) - if num_args != 7: + if num_args not in (7, 8): error("Unknown number of arguments") git_token = argv[1] repo = argv[2] run_id = argv[3] hash = argv[4] - which_gap = argv[5] - job_name_prefix = argv[6] + report_key = argv[5] + if num_args == 8: + gap_display_name = argv[6] + job_name_prefix = argv[7] + else: + gap_display_name = report_key + job_name_prefix = argv[6] ################################################################################ # Collect names of all packages @@ -175,8 +180,11 @@ def main(argv: List[str]) -> int: report["hash"] = hash date = str(datetime.now()).split(".")[0] report["date"] = date - report["gap_version"] = which_gap - report["id"] = os.path.join(which_gap, "%s-%s" % (date.replace(" ", "-"), hash[:8])) + report["gap_version"] = gap_display_name + report["gap_report_key"] = report_key + report["id"] = os.path.join( + report_key, "%s-%s" % (date.replace(" ", "-"), hash[:8]) + ) # Path root = "data/reports" diff --git a/tools/render_gap_pr_test_comment.py b/tools/render_gap_pr_test_comment.py new file mode 100644 index 000000000..5ddacad5c --- /dev/null +++ b/tools/render_gap_pr_test_comment.py @@ -0,0 +1,124 @@ +#!/usr/bin/env python3 + +############################################################################# +## +## This file is part of GAP, a system for computational discrete algebra. +## +## Copyright of GAP belongs to its developers, whose names are too numerous +## to list here. Please refer to the COPYRIGHT file for details. +## +## SPDX-License-Identifier: GPL-2.0-or-later +## + +import json +import sys +from typing import Any + +from utils import error + +COMMENT_MARKER = "" + + +def load_json(path: str) -> dict[str, Any]: + with open(path, "r") as f: + return json.load(f) + + +def load_text(path: str) -> str: + with open(path, "r") as f: + return f.read().strip() + + +def package_links( + package_names: list[str], test_status: dict[str, Any], limit: int = 10 +) -> str: + rows = [] + for name in package_names[:limit]: + pkg = test_status["pkgs"][name] + rows.append(f"- [{name} {pkg['version']}]({pkg['workflow_run']})") + if len(package_names) > limit: + rows.append(f"- ... and {len(package_names) - limit} more") + return "\n".join(rows) + + +def render_gap_pr_comment( + metadata: dict[str, Any], + test_status: dict[str, Any], + report_diff: dict[str, Any], + report_markdown: str, +) -> str: + repo_full_name = metadata["repo_full_name"] + number = metadata["number"] + head_sha = metadata["head_sha"][:8] + workflow_url = test_status["workflow"] + + lines = [ + COMMENT_MARKER, + f"## PackageDistro test for {repo_full_name}#{number}", + "", + f"- PR: {metadata['pr_url']}", + f"- Tested head SHA: `{head_sha}`", + f"- Workflow: [workflow run]({workflow_url})", + ( + f"- Summary: {test_status['success']} succeeded, " + f"{test_status['failure']} failed, {test_status['skipped']} skipped" + ), + ] + + requester = metadata.get("requester") + command = metadata.get("command") + if requester and command: + lines.append(f"- Requested by @{requester} via `{command}`.") + + lines.append("") + + failures_changed = report_diff.get("failures_changed", []) + failures_current = report_diff.get("failures_current", []) + if report_diff.get("failure_changed", 0) > 0 and failures_changed: + lines.append("### New failures vs baseline") + lines.append("") + lines.append(package_links(failures_changed, test_status)) + lines.append("") + elif test_status.get("failure", 0) > 0 and failures_current: + lines.append("### Current failing packages") + lines.append("") + lines.append(package_links(failures_current, test_status)) + lines.append("") + + lines.extend( + [ + "
", + "Full report", + "", + report_markdown.strip(), + "", + "
", + ] + ) + return "\n".join(lines).strip() + "\n" + + +def main(argv: list[str]) -> int: + if len(argv) != 5: + error( + "usage: render_gap_pr_test_comment.py " + "METADATA_JSON TEST_STATUS_JSON REPORT_DIFF_JSON REPORT_MD" + ) + metadata = load_json(argv[1]) + test_status = load_json(argv[2]) + report_diff = load_json(argv[3]) + report_markdown = load_text(argv[4]) + print( + render_gap_pr_comment( + metadata=metadata, + test_status=test_status, + report_diff=report_diff, + report_markdown=report_markdown, + ), + end="", + ) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main(sys.argv)) diff --git a/tools/tests/test_gap_pr_utils.py b/tools/tests/test_gap_pr_utils.py new file mode 100644 index 000000000..39d0d1cce --- /dev/null +++ b/tools/tests/test_gap_pr_utils.py @@ -0,0 +1,92 @@ +# -*- coding: utf-8 -*- +# pylint: disable=missing-function-docstring, invalid-name + +import importlib +import os +import sys + +sys.path.insert( + 0, "/".join(os.path.dirname(os.path.realpath(__file__)).split("/")[:-1]) +) + + +def test_parse_gap_pull_request_url_accepts_standard_urls(): + module = importlib.import_module("gap_pr_utils") + + assert module.parse_gap_pull_request_url( + "https://github.com/gap-system/gap/pull/1234" + ) == ("gap-system", "gap", 1234) + assert module.parse_gap_pull_request_url( + "https://github.com/gap-system/gap/pull/1234/" + ) == ("gap-system", "gap", 1234) + assert module.parse_gap_pull_request_url( + "https://github.com/gap-system/gap/pull/1234/files?foo=bar" + ) == ("gap-system", "gap", 1234) + + +def test_parse_gap_pull_request_url_rejects_invalid_urls(): + module = importlib.import_module("gap_pr_utils") + + for bad_url in [ + "https://github.com/gap-system/gap/issues/1234", + "https://github.com/gap-system/PackageDistro/pull/1234", + "https://example.com/gap-system/gap/pull/1234", + "not a url", + ]: + try: + module.parse_gap_pull_request_url(bad_url) + except ValueError as exc: + assert "GAP pull request URL" in str(exc) + else: + raise AssertionError(f"expected ValueError for {bad_url}") + + +def test_resolve_gap_pull_request_normalizes_metadata(): + module = importlib.import_module("gap_pr_utils") + + class Response: + def raise_for_status(self): + return None + + def json(self): + return { + "title": "Fix something", + "base": { + "ref": "master", + "repo": { + "full_name": "gap-system/gap", + "clone_url": "https://github.com/gap-system/gap.git", + }, + }, + "head": { + "ref": "feature/test-pr", + "sha": "0123456789abcdef0123456789abcdef01234567", + "repo": { + "full_name": "contrib/gap", + "clone_url": "https://github.com/contrib/gap.git", + "html_url": "https://github.com/contrib/gap", + }, + }, + } + + seen = {} + + def fake_get(url, headers): + seen["url"] = url + seen["headers"] = headers + return Response() + + resolved = module.resolve_gap_pull_request( + "https://github.com/gap-system/gap/pull/1234", token="TOKEN", get=fake_get + ) + + assert seen["url"] == "https://api.github.com/repos/gap-system/gap/pulls/1234" + assert seen["headers"]["Authorization"] == "Bearer TOKEN" + assert resolved["pr_url"] == "https://github.com/gap-system/gap/pull/1234" + assert resolved["base_ref"] == "master" + assert resolved["head_ref"] == "feature/test-pr" + assert resolved["head_sha"] == "0123456789abcdef0123456789abcdef01234567" + assert resolved["head_repo_clone_url"] == "https://github.com/contrib/gap.git" + assert resolved["report_key"] == "pr-gap-system-gap-1234-01234567" + assert resolved["report_label"] == "gap-system/gap#1234 @ 01234567 (base master)" + assert resolved["job_name_prefix"] == "PR #1234" diff --git a/tools/tests/test_generate_report.py b/tools/tests/test_generate_report.py index 1cbd5ab07..29599306d 100644 --- a/tools/tests/test_generate_report.py +++ b/tools/tests/test_generate_report.py @@ -6,6 +6,7 @@ This module contains some tests for the generate_report.py script """ +import json import os import shutil @@ -62,3 +63,25 @@ def test_report(): ) clear_files(root) + + +def test_report_diff_contains_failure_lists(): + root = "/".join(os.path.dirname(os.path.realpath(__file__)).split("/")[:-2]) + os.chdir(root) + + _, current_path = generate_report(root) + + with open(os.path.join(current_path, "test-status-diff.json"), "r") as f: + report_diff = json.load(f) + + assert report_diff["failures_changed"] == ["boogle", "zork"] + assert report_diff["failures_current"] == [ + "bar", + "boogle", + "foo", + "ham", + "quux", + "zork", + ] + + clear_files(root) diff --git a/tools/tests/test_render_gap_pr_test_comment.py b/tools/tests/test_render_gap_pr_test_comment.py new file mode 100644 index 000000000..9753893e4 --- /dev/null +++ b/tools/tests/test_render_gap_pr_test_comment.py @@ -0,0 +1,89 @@ +# -*- coding: utf-8 -*- +# pylint: disable=missing-function-docstring, invalid-name + +import importlib +import os +import sys + +sys.path.insert( + 0, "/".join(os.path.dirname(os.path.realpath(__file__)).split("/")[:-1]) +) + + +def test_render_gap_pr_comment_reports_new_failures(): + module = importlib.import_module("render_gap_pr_test_comment") + + body = module.render_gap_pr_comment( + metadata={ + "pr_url": "https://github.com/gap-system/gap/pull/1234", + "repo_full_name": "gap-system/gap", + "number": 1234, + "head_sha": "0123456789abcdef0123456789abcdef01234567", + "requester": "fingolfin", + "command": "@gap-package-distribution-bot test", + }, + test_status={ + "workflow": "https://github.com/gap-system/PackageDistro/actions/runs/77", + "success": 120, + "failure": 2, + "skipped": 3, + "pkgs": { + "atlasrep": { + "status": "failure", + "version": "2.1", + "workflow_run": "https://example.invalid/job/atlasrep", + }, + "guava": { + "status": "failure", + "version": "3.0", + "workflow_run": "https://example.invalid/job/guava", + }, + }, + }, + report_diff={"failure_changed": 1, "failures_changed": ["atlasrep"]}, + report_markdown="# Package Evaluation Report\n\nDetails here.\n", + ) + + assert "gap-system/gap#1234" in body + assert "Requested by @fingolfin via `@gap-package-distribution-bot test`." in body + assert "`01234567`" in body + assert ( + "[workflow run](https://github.com/gap-system/PackageDistro/actions/runs/77)" + in body + ) + assert "120 succeeded, 2 failed, 3 skipped" in body + assert "New failures vs baseline" in body + assert "[atlasrep 2.1](https://example.invalid/job/atlasrep)" in body + assert "" in body + + +def test_render_gap_pr_comment_reports_current_failures_when_no_regressions(): + module = importlib.import_module("render_gap_pr_test_comment") + + body = module.render_gap_pr_comment( + metadata={ + "pr_url": "https://github.com/gap-system/gap/pull/1234", + "repo_full_name": "gap-system/gap", + "number": 1234, + "head_sha": "fedcba9876543210fedcba9876543210fedcba98", + }, + test_status={ + "workflow": "https://github.com/gap-system/PackageDistro/actions/runs/88", + "success": 100, + "failure": 1, + "skipped": 0, + "pkgs": { + "atlasrep": { + "status": "failure", + "version": "2.1", + "workflow_run": "https://example.invalid/job/atlasrep", + } + }, + }, + report_diff={"failure_changed": 0, "failures_current": ["atlasrep"]}, + report_markdown="# Package Evaluation Report\n\nDetails here.\n", + ) + + assert "Current failing packages" in body + assert "[atlasrep 2.1](https://example.invalid/job/atlasrep)" in body + assert "New failures vs baseline" not in body From aa87e9fd75b99765e3d3f25bba990c783a1ba824 Mon Sep 17 00:00:00 2001 From: Max Horn Date: Mon, 11 May 2026 21:32:51 +0200 Subject: [PATCH 2/2] Asked the AI to write more comments... --- .github/workflows/test-all.yml | 30 +++++++++++++++++++++++++++++ tools/gap_pr_utils.py | 22 +++++++++++++++++++++ tools/generate_report.py | 3 +++ tools/generate_test_status.py | 7 +++++++ tools/render_gap_pr_test_comment.py | 17 ++++++++++++++++ 5 files changed, 79 insertions(+) diff --git a/.github/workflows/test-all.yml b/.github/workflows/test-all.yml index 59af629bb..a5c6b17d9 100644 --- a/.github/workflows/test-all.yml +++ b/.github/workflows/test-all.yml @@ -26,11 +26,19 @@ on: required: false type: string default: https://github.com/gap-system/gap.git + # Optional PR-specific pin: when set, we still clone `which-gap` from + # `which-gap-repo`, but then verify that the branch head matches this + # exact commit SHA. This lets ad hoc runs target a moving PR branch + # without silently testing newer commits if the PR is updated mid-run. which-gap-sha: description: 'Exact GAP commit SHA expected at the branch head' required: false type: string default: "" + # `report-key` controls where reports/artifacts are stored, while + # `report-label` is what humans see in report.md. Scheduled runs use the + # same string for both ("master", "4.12.2", ...), but PR-triggered runs + # use a synthetic key and a friendlier display label. report-key: description: 'Identifier used for artifacts and report paths' required: false @@ -41,11 +49,18 @@ on: required: false type: string default: "" + # Comparison runs need an explicit baseline key because the thing we test + # (`which-gap`) is not always the thing we compare against. For a GAP PR + # head we test the PR branch/ref, but compare against the latest report + # for the PR base branch, usually `master`. baseline-key: description: 'Baseline key used for latest-* comparisons' required: false type: string default: "" + # The generated test-status script identifies package jobs by their + # workflow job names. Reusable callers can override the visible prefix + # here so the report code knows how to map job names back to packages. job-name-prefix: description: 'Reusable workflow job prefix override' required: false @@ -78,26 +93,31 @@ on: required: false type: string default: https://github.com/gap-system/gap.git + # See the matching comment under `workflow_dispatch` above. which-gap-sha: description: 'Exact GAP commit SHA expected at the branch head' required: false type: string default: "" + # See the matching comment under `workflow_dispatch` above. report-key: description: 'Identifier used for artifacts and report paths' required: false type: string default: "" + # See the matching comment under `workflow_dispatch` above. report-label: description: 'Human-readable GAP label for the generated report' required: false type: string default: "" + # See the matching comment under `workflow_dispatch` above. baseline-key: description: 'Baseline key used for latest-* comparisons' required: false type: string default: "" + # See the matching comment under `workflow_dispatch` above. job-name-prefix: description: 'Reusable workflow job prefix override' required: false @@ -192,6 +212,10 @@ jobs: fi cd $HOME/gap if [[ -n "${whichgapsha}" ]]; then + # The branch may have moved since the dispatch request was created. + # Fail explicitly in that case so the caller knows it must retrigger + # against the new PR head, rather than reporting results for the + # wrong commit. actualsha=$(git rev-parse HEAD) if [[ "${actualsha}" != "${whichgapsha}" ]]; then echo "Expected GAP branch head ${whichgapsha}, got ${actualsha}." @@ -429,6 +453,10 @@ jobs: - name: "Generate test-status.json" id: test-status run: | + # `REPORT_KEY` selects the storage location under data/reports/, + # while `REPORT_LABEL` controls the human-facing label inside the + # report. Keeping these separate lets PR runs reuse the same report + # generator without colliding with long-lived branch reports. REPORT_KEY="${{ github.event.inputs.report-key || inputs.report-key || github.event.inputs.which-gap || inputs.which-gap }}" REPORT_LABEL="${{ github.event.inputs.report-label || inputs.report-label || github.event.inputs.which-gap || inputs.which-gap }}" if [ "${{ inputs.job-name-prefix }}" != "" ]; then @@ -449,6 +477,8 @@ jobs: id: download-latest-report run: | ROOT="data/reports" + # Compare against the latest report for the chosen baseline key, which + # may differ from REPORT_KEY for ad hoc PR runs. BASELINE_KEY="${{ github.event.inputs.baseline-key || inputs.baseline-key || github.event.inputs.report-key || inputs.report-key || github.event.inputs.which-gap || inputs.which-gap }}" DIR_SYM_REL=latest-${BASELINE_KEY} DIR_SYM="${ROOT}/${DIR_SYM_REL}" diff --git a/tools/gap_pr_utils.py b/tools/gap_pr_utils.py index ffc4d63ed..79c9cb266 100644 --- a/tools/gap_pr_utils.py +++ b/tools/gap_pr_utils.py @@ -18,6 +18,13 @@ import requests from utils import error +# This helper is the "front door" for ad hoc PR-triggered runs. The +# PackageDistro workflow accepts only a GAP PR URL, and this script turns that +# into the concrete clone/ref/SHA metadata needed by test-all.yml. +# +# The returned JSON is intentionally workflow-oriented rather than a direct copy +# of the GitHub API payload: downstream steps consume stable keys such as +# `report_key`, `report_label`, and `job_name_prefix`. GAP_PR_URL_RE = re.compile( r"^https://github\.com/(?P[^/]+)/(?P[^/]+)/pull/(?P\d+)" r"(?:[/?#].*)?$" @@ -32,6 +39,8 @@ def github_headers(token: str | None) -> Dict[str, str]: def parse_gap_pull_request_url(url: str) -> Tuple[str, str, int]: + # The current feature is deliberately scoped to GAP core PRs. Rejecting any + # other repository early keeps the workflows and comments predictable. match = GAP_PR_URL_RE.match(url) if match is None: raise ValueError("Expected a GAP pull request URL on github.com") @@ -43,6 +52,9 @@ def parse_gap_pull_request_url(url: str) -> Tuple[str, str, int]: def build_report_key(owner: str, repo: str, number: int, head_sha: str) -> str: + # `report_key` becomes part of the artifact name and report path under + # data/reports/. It must be stable for a given PR head but distinct from the + # baseline branch key such as "master". return f"pr-{owner}-{repo}-{number}-{head_sha[:8]}".lower() @@ -57,6 +69,9 @@ def resolve_gap_pull_request( token: str | None = None, get: Callable[..., requests.Response] = requests.get, ) -> Dict[str, Any]: + # Resolve the PR once up front so the rest of the workflow can stay simple: + # it just consumes normalized metadata instead of duplicating API parsing in + # shell. owner, repo, number = parse_gap_pull_request_url(url) api_url = f"https://api.github.com/repos/{owner}/{repo}/pulls/{number}" response = get(api_url, headers=github_headers(token)) @@ -65,6 +80,10 @@ def resolve_gap_pull_request( base_ref = payload["base"]["ref"] head_sha = payload["head"]["sha"] + # The downstream workflow needs both the user-facing branch name (`head_ref`) + # and the exact commit (`head_sha`). test-all.yml clones the branch and then + # verifies that the current tip still matches this SHA so a moving PR head is + # detected explicitly instead of silently testing newer code. normalized = { "pr_url": f"https://github.com/{owner}/{repo}/pull/{number}", "api_url": api_url, @@ -81,6 +100,9 @@ def resolve_gap_pull_request( "head_repo_full_name": payload["head"]["repo"]["full_name"], "head_repo_clone_url": payload["head"]["repo"]["clone_url"], "head_repo_html_url": payload["head"]["repo"]["html_url"], + # These synthetic fields connect the resolver to the reporting pipeline. + # They let ad hoc PR runs reuse the same report generation code as the + # scheduled branch-based runs without colliding with branch report names. "report_key": build_report_key(owner, repo, number, head_sha), "report_label": build_report_label(owner, repo, number, head_sha, base_ref), "job_name_prefix": f"PR #{number}", diff --git a/tools/generate_report.py b/tools/generate_report.py index 49144839d..d839fd89a 100755 --- a/tools/generate_report.py +++ b/tools/generate_report.py @@ -101,6 +101,9 @@ ] report_diff["failures_changed"] = sorted(pkgs_changed["failure"]) +# Keep an explicit list of current failures in the JSON diff so workflows that +# post condensed summaries to PR comments do not have to re-derive it from the +# full package table. report_diff["failures_current"] = sorted( pkg for pkg, data in pkgs.items() if data["status"] == "failure" ) diff --git a/tools/generate_test_status.py b/tools/generate_test_status.py index 8de1847f8..492100d7f 100755 --- a/tools/generate_test_status.py +++ b/tools/generate_test_status.py @@ -99,6 +99,9 @@ def main(argv: List[str]) -> int: hash = argv[4] report_key = argv[5] if num_args == 8: + # Ad hoc PR runs pass a stable storage key plus a friendlier display + # label. Scheduled branch-based runs still use the older 7-argument + # calling convention in which the key also serves as the display name. gap_display_name = argv[6] job_name_prefix = argv[7] else: @@ -181,6 +184,10 @@ def main(argv: List[str]) -> int: date = str(datetime.now()).split(".")[0] report["date"] = date report["gap_version"] = gap_display_name + # `gap_report_key` is the machine-oriented identifier used to pick the + # storage location and compare against the right baseline. For regular runs + # it is just "master" etc.; for PR-triggered runs it is a synthetic key + # such as "pr-gap-system-gap-1234-deadbeef". report["gap_report_key"] = report_key report["id"] = os.path.join( report_key, "%s-%s" % (date.replace(" ", "-"), hash[:8]) diff --git a/tools/render_gap_pr_test_comment.py b/tools/render_gap_pr_test_comment.py index 5ddacad5c..a54bb5493 100644 --- a/tools/render_gap_pr_test_comment.py +++ b/tools/render_gap_pr_test_comment.py @@ -16,6 +16,9 @@ from utils import error +# The GAP-side trigger workflow and the PackageDistro-side completion workflow +# communicate through a single PR comment. This marker lets both sides find and +# update that comment rather than leaving a trail of stale status messages. COMMENT_MARKER = "" @@ -32,6 +35,8 @@ def load_text(path: str) -> str: def package_links( package_names: list[str], test_status: dict[str, Any], limit: int = 10 ) -> str: + # The underlying report can be large. The PR comment keeps only the most + # relevant failing packages inline and links each one directly to its job. rows = [] for name in package_names[:limit]: pkg = test_status["pkgs"][name] @@ -47,6 +52,13 @@ def render_gap_pr_comment( report_diff: dict[str, Any], report_markdown: str, ) -> str: + # This comment intentionally contains three layers of information: + # 1. immutable run identity (PR URL, tested SHA, workflow link), + # 2. a compact failure summary for quick triage, and + # 3. the full generated report in a collapsible details block. + # + # That keeps the PR conversation readable while still exposing the same + # report content that a user would otherwise have to fetch from artifacts. repo_full_name = metadata["repo_full_name"] number = metadata["number"] head_sha = metadata["head_sha"][:8] @@ -74,6 +86,8 @@ def render_gap_pr_comment( failures_changed = report_diff.get("failures_changed", []) failures_current = report_diff.get("failures_current", []) + # Prefer regressions over the full failure list: when a baseline exists, the + # new failures are the most actionable signal for reviewers. if report_diff.get("failure_changed", 0) > 0 and failures_changed: lines.append("### New failures vs baseline") lines.append("") @@ -99,6 +113,9 @@ def render_gap_pr_comment( def main(argv: list[str]) -> int: + # The workflow passes file paths instead of large JSON blobs on the command + # line so that the shell glue stays simple and the data is easy to inspect + # in failed CI runs. if len(argv) != 5: error( "usage: render_gap_pr_test_comment.py "