diff --git a/.github/workflows/test-all.yml b/.github/workflows/test-all.yml index 2eede6d3c..a5c6b17d9 100644 --- a/.github/workflows/test-all.yml +++ b/.github/workflows/test-all.yml @@ -26,6 +26,46 @@ 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 + type: string + default: "" + report-label: + description: 'Human-readable GAP label for the generated report' + 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 + type: string + default: "" pkg-build-glob: description: 'Only build packages matching the given glob' required: false @@ -53,6 +93,36 @@ 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 + type: string + default: "" pkg-build-glob: description: 'Only build packages matching the given glob' required: false @@ -130,16 +200,29 @@ 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 + # 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}." + echo "The pull request changed after this run was requested." + exit 1 + fi + fi echo "::endgroup::" echo "::group::autogen" @@ -209,7 +292,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 +453,22 @@ 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` 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 + 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 +477,10 @@ jobs: id: download-latest-report run: | ROOT="data/reports" - DIR_SYM_REL=latest-${{ needs.build.outputs.artifact }} + # 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}" URL_SYM="https://raw.githubusercontent.com/${{ github.repository }}/${DIR_SYM}" # Check if file at url exists, @@ -405,7 +499,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..79c9cb266 --- /dev/null +++ b/tools/gap_pr_utils.py @@ -0,0 +1,123 @@ +#!/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 + +# 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"(?:[/?#].*)?$" +) + + +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]: + # 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") + 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: + # `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() + + +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]: + # 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)) + response.raise_for_status() + payload = response.json() + + 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, + "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"], + # 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}", + } + 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..d839fd89a 100755 --- a/tools/generate_report.py +++ b/tools/generate_report.py @@ -100,6 +100,14 @@ and pkgs[pkg]["status"] == status ] +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" +) + 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..492100d7f 100755 --- a/tools/generate_test_status.py +++ b/tools/generate_test_status.py @@ -90,15 +90,23 @@ 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: + # 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: + gap_display_name = report_key + job_name_prefix = argv[6] ################################################################################ # Collect names of all packages @@ -175,8 +183,15 @@ 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 + # `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]) + ) # 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..a54bb5493 --- /dev/null +++ b/tools/render_gap_pr_test_comment.py @@ -0,0 +1,141 @@ +#!/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 + +# 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 = "" + + +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: + # 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] + 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: + # 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] + 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", []) + # 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("") + 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: + # 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 " + "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