ci: surface E2E screenshots in review comment (#69631)
* ci: surface E2E screenshots in review comment * ci: mark completed review commits in past tense * ci: surface approved sensitive-file reviews * ci: link sensitive files to reviewed changes * ci: stage desktop E2E visual evidence Track screenshots newly introduced against main and package visual diffs for a trusted publisher. * fix(ci): pass E2E evidence output paths Supply the manifest and staging-directory arguments required by the screenshot status helper. * fix(ci): download the OSV SARIF artifact Match the artifact name and result filename emitted by the pinned upstream reusable workflow.
This commit is contained in:
ethernet
GitHub
parent
8d6e045b8f
commit
433673067e
|
|
@ -39,6 +39,9 @@ outputs:
|
|||
ci_review:
|
||||
description: Require CI-sensitive file review label.
|
||||
value: ${{ steps.classify.outputs.ci_review }}
|
||||
ci_review_files:
|
||||
description: JSON list of CI-sensitive files changed by the pull request.
|
||||
value: ${{ steps.classify.outputs.ci_review_files }}
|
||||
|
||||
runs:
|
||||
using: composite
|
||||
|
|
|
|||
|
|
@ -46,6 +46,7 @@ jobs:
|
|||
docker_meta: ${{ steps.classify.outputs.docker_meta }}
|
||||
mcp_catalog: ${{ steps.classify.outputs.mcp_catalog }}
|
||||
ci_review: ${{ steps.classify.outputs.ci_review }}
|
||||
ci_review_files: ${{ steps.classify.outputs.ci_review_files }}
|
||||
event_name: ${{ github.event_name }}
|
||||
steps:
|
||||
- uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
|
||||
|
|
@ -163,6 +164,7 @@ jobs:
|
|||
uses: ./.github/workflows/review-labels.yml
|
||||
with:
|
||||
ci_review: ${{ needs.detect.outputs.ci_review == 'true' }}
|
||||
ci_review_files: ${{ needs.detect.outputs.ci_review_files }}
|
||||
mcp_catalog: ${{ needs.detect.outputs.mcp_catalog == 'true' }}
|
||||
supply_chain: ${{ needs.supply-chain.outputs.critical_findings == 'true' }}
|
||||
secrets: inherit
|
||||
|
|
@ -186,7 +188,7 @@ jobs:
|
|||
# ─────────────────────────────────────────────────────────────────────
|
||||
comment-live:
|
||||
name: CI review comment (live)
|
||||
needs: [detect, review-labels, lockfile-diff, supply-chain, osv-scanner, uv-lockfile, history-check, contributor-check]
|
||||
needs: [detect, review-labels, lockfile-diff, supply-chain, osv-scanner, uv-lockfile, history-check, contributor-check, e2e-desktop]
|
||||
if: always() && github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork != true
|
||||
runs-on: ubuntu-latest
|
||||
timeout-minutes: 40
|
||||
|
|
|
|||
|
|
@ -2,6 +2,10 @@ name: E2E Desktop
|
|||
|
||||
on:
|
||||
workflow_call:
|
||||
outputs:
|
||||
review_status:
|
||||
description: Screenshot and visual-diff status for the CI review comment.
|
||||
value: ${{ jobs.e2e.outputs.review_status }}
|
||||
|
||||
permissions:
|
||||
contents: read
|
||||
|
|
@ -15,6 +19,8 @@ jobs:
|
|||
name: Playwright E2E (Linux)
|
||||
runs-on: ubuntu-latest
|
||||
timeout-minutes: 20
|
||||
outputs:
|
||||
review_status: ${{ steps.review-status.outputs.review_status }}
|
||||
steps:
|
||||
- uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
|
||||
|
||||
|
|
@ -146,6 +152,25 @@ jobs:
|
|||
overwrite: true
|
||||
if-no-files-found: ignore
|
||||
|
||||
- name: Build screenshot review status
|
||||
id: review-status
|
||||
if: always()
|
||||
working-directory: apps/desktop
|
||||
env:
|
||||
RESULTS_URL: ${{ steps.upload-results.outputs.artifact-url }}
|
||||
run: |
|
||||
python3 ../../scripts/ci/e2e_screenshot_status.py \
|
||||
--results-dir test-results \
|
||||
--manifest-output /tmp/e2e-screenshot-manifest.json \
|
||||
--evidence-dir /tmp/e2e-evidence \
|
||||
--artifact-url "$RESULTS_URL" \
|
||||
--output /tmp/e2e-review-status.json
|
||||
{
|
||||
echo 'review_status<<__E2E_REVIEW_STATUS__'
|
||||
cat /tmp/e2e-review-status.json
|
||||
echo '__E2E_REVIEW_STATUS__'
|
||||
} >> "$GITHUB_OUTPUT"
|
||||
|
||||
# ── Generate step summary with visual diff info ───────────────────
|
||||
# Parse the JSON report + scan for diff images, then post a summary
|
||||
# to the GitHub Actions step output so reviewers can see what changed
|
||||
|
|
@ -159,49 +184,50 @@ jobs:
|
|||
RESULTS_URL: ${{ steps.upload-results.outputs.artifact-url }}
|
||||
DIFFS_URL: ${{ steps.upload-diffs.outputs.artifact-url }}
|
||||
run: |
|
||||
echo "## Desktop E2E — Visual Diff Report" >> "$GITHUB_STEP_SUMMARY"
|
||||
echo "" >> "$GITHUB_STEP_SUMMARY"
|
||||
{
|
||||
echo "## Desktop E2E — Visual Diff Report"
|
||||
echo ""
|
||||
|
||||
# Count diff images (playwright writes *-diff.png on mismatch)
|
||||
DIFF_COUNT=$(find test-results -name '*-diff.png' 2>/dev/null | wc -l)
|
||||
ACTUAL_COUNT=$(find test-results -name '*-actual.png' 2>/dev/null | wc -l)
|
||||
|
||||
if [ "$DIFF_COUNT" -eq 0 ]; then
|
||||
echo "✅ All $ACTUAL_COUNT screenshot(s) matched their baselines (or no baselines existed yet)." >> "$GITHUB_STEP_SUMMARY"
|
||||
echo "✅ All $ACTUAL_COUNT screenshot(s) matched their baselines (or no baselines existed yet)."
|
||||
else
|
||||
echo "📸 **$DIFF_COUNT of $ACTUAL_COUNT screenshot(s) differ from baseline:**" >> "$GITHUB_STEP_SUMMARY"
|
||||
echo "" >> "$GITHUB_STEP_SUMMARY"
|
||||
echo "| Test | Diff | Actual | Expected |" >> "$GITHUB_STEP_SUMMARY"
|
||||
echo "|------|------|--------|----------|" >> "$GITHUB_STEP_SUMMARY"
|
||||
echo "📸 **$DIFF_COUNT of $ACTUAL_COUNT screenshot(s) differ from baseline:**"
|
||||
echo ""
|
||||
echo "| Test | Diff | Actual | Expected |"
|
||||
echo "|------|------|--------|----------|"
|
||||
|
||||
# List each diff image with a link to the artifact
|
||||
for diff in $(find test-results -name '*-diff.png' 2>/dev/null | sort); do
|
||||
base=$(echo "$diff" | sed 's/-diff\.png$//')
|
||||
base=${diff%-diff.png}
|
||||
test_name=$(basename "$base")
|
||||
echo "| $test_name | [diff]($diff) | [actual](${base}-actual.png) | [expected](${base}-expected.png) |" >> "$GITHUB_STEP_SUMMARY"
|
||||
echo "| $test_name | [diff]($diff) | [actual](${base}-actual.png) | [expected](${base}-expected.png) |"
|
||||
done
|
||||
fi
|
||||
|
||||
echo "" >> "$GITHUB_STEP_SUMMARY"
|
||||
echo "📥 **Artifacts:**" >> "$GITHUB_STEP_SUMMARY"
|
||||
echo "" >> "$GITHUB_STEP_SUMMARY"
|
||||
echo ""
|
||||
echo "📥 **Artifacts:**"
|
||||
echo ""
|
||||
if [ -n "$RESULTS_URL" ]; then
|
||||
echo "- [playwright-test-results]($RESULTS_URL) — all screenshots (actual + expected + diff) + traces" >> "$GITHUB_STEP_SUMMARY"
|
||||
echo "- [playwright-test-results]($RESULTS_URL) — all screenshots (actual + expected + diff) + traces"
|
||||
fi
|
||||
if [ -n "$REPORT_URL" ]; then
|
||||
echo "- [playwright-report]($REPORT_URL) — interactive HTML report" >> "$GITHUB_STEP_SUMMARY"
|
||||
echo "- [playwright-report]($REPORT_URL) — interactive HTML report"
|
||||
fi
|
||||
if [ -n "$DIFFS_URL" ]; then
|
||||
echo "- [visual-diffs]($DIFFS_URL) — just the diffed screenshots (small, fast to review)" >> "$GITHUB_STEP_SUMMARY"
|
||||
echo "- [visual-diffs]($DIFFS_URL) — just the diffed screenshots (small, fast to review)"
|
||||
fi
|
||||
echo "" >> "$GITHUB_STEP_SUMMARY"
|
||||
echo "**To update baselines:** merge to main (baselines auto-update on main runs) or run \`npx playwright test --update-snapshots\` locally." >> "$GITHUB_STEP_SUMMARY"
|
||||
echo ""
|
||||
echo "**To update baselines:** merge to main (baselines auto-update on main runs) or run \`npx playwright test --update-snapshots\` locally."
|
||||
|
||||
# Also parse the JSON report for pass/fail counts
|
||||
if [ -f playwright-report/results.json ]; then
|
||||
echo "" >> "$GITHUB_STEP_SUMMARY"
|
||||
echo "### Test Results" >> "$GITHUB_STEP_SUMMARY"
|
||||
echo "" >> "$GITHUB_STEP_SUMMARY"
|
||||
echo ""
|
||||
echo "### Test Results"
|
||||
echo ""
|
||||
node -e "
|
||||
const r = require('./playwright-report/results.json');
|
||||
const stats = r.stats || {};
|
||||
|
|
@ -211,5 +237,6 @@ jobs:
|
|||
console.log('| ❌ Failed | ' + (stats.unexpected || 0) + ' |');
|
||||
console.log('| ⏭️ Skipped | ' + (stats.skipped || 0) + ' |');
|
||||
console.log('| 🔄 Flaky | ' + (stats.flaky || 0) + ' |');
|
||||
" >> "$GITHUB_STEP_SUMMARY" 2>/dev/null || true
|
||||
" 2>/dev/null || true
|
||||
fi
|
||||
} >> "$GITHUB_STEP_SUMMARY"
|
||||
|
|
|
|||
|
|
@ -48,6 +48,9 @@ jobs:
|
|||
--lockfile=uv.lock
|
||||
--lockfile=package-lock.json
|
||||
--lockfile=website/package-lock.json
|
||||
# The upstream reusable workflow uploads this exact file under its
|
||||
# fixed artifact name, which the wrapper downloads below.
|
||||
results-file-name: osv-results.sarif
|
||||
fail-on-vuln: false
|
||||
|
||||
emit-status:
|
||||
|
|
@ -64,7 +67,7 @@ jobs:
|
|||
- name: Download SARIF result
|
||||
uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4
|
||||
with:
|
||||
name: osv-results
|
||||
name: OSV Scanner SARIF file
|
||||
path: /tmp/osv-results
|
||||
continue-on-error: true
|
||||
|
||||
|
|
|
|||
|
|
@ -23,6 +23,10 @@ on:
|
|||
description: Whether CI-sensitive files (eslint config, workflows, actions) changed.
|
||||
type: boolean
|
||||
default: false
|
||||
ci_review_files:
|
||||
description: JSON list of CI-sensitive files changed by the pull request.
|
||||
type: string
|
||||
default: '[]'
|
||||
mcp_catalog:
|
||||
description: Whether the MCP catalog / installer changed.
|
||||
type: boolean
|
||||
|
|
@ -78,18 +82,25 @@ jobs:
|
|||
id: build-status
|
||||
env:
|
||||
CI_REVIEW: ${{ inputs.ci_review }}
|
||||
CI_REVIEW_FILES: ${{ inputs.ci_review_files }}
|
||||
MCP_CATALOG: ${{ inputs.mcp_catalog }}
|
||||
SUPPLY_CHAIN: ${{ inputs.supply_chain }}
|
||||
LABEL_PRESENT: ${{ steps.label-check.outputs.ci_reviewed }}
|
||||
REPO_URL: ${{ github.server_url }}/${{ github.repository }}
|
||||
BASE_SHA: ${{ github.event.pull_request.base.sha }}
|
||||
HEAD_SHA: ${{ github.event.pull_request.head.sha }}
|
||||
run: |
|
||||
set -euo pipefail
|
||||
args=()
|
||||
if [ "$CI_REVIEW" = "true" ]; then args+=(--ci-review); fi
|
||||
args+=(--ci-review-files "$CI_REVIEW_FILES")
|
||||
if [ "$MCP_CATALOG" = "true" ]; then args+=(--mcp-catalog); fi
|
||||
if [ "$SUPPLY_CHAIN" = "true" ]; then args+=(--supply-chain); fi
|
||||
if [ "$LABEL_PRESENT" = "true" ]; then args+=(--label-present); fi
|
||||
|
||||
python3 scripts/ci/emit_review_status.py "${args[@]}" --output "$GITHUB_OUTPUT"
|
||||
python3 scripts/ci/emit_review_status.py "${args[@]}" \
|
||||
--repo-url "$REPO_URL" --base-sha "$BASE_SHA" --head-sha "$HEAD_SHA" \
|
||||
--output "$GITHUB_OUTPUT"
|
||||
|
||||
- name: Fail on missing label
|
||||
if: steps.label-check.outputs.ci_reviewed != 'true'
|
||||
|
|
|
|||
|
|
@ -26,7 +26,7 @@ result objects::
|
|||
|
||||
Each result object has:
|
||||
|
||||
kind: "error" | "action_required" | "warning" | "info"
|
||||
kind: "error" | "action_required" | "warning" | "info" | "debug"
|
||||
title: section heading
|
||||
summary: one-line description
|
||||
detail: markdown detail (optional)
|
||||
|
|
@ -56,7 +56,7 @@ from pathlib import Path
|
|||
MARKER = "<!-- hermes-ci-review-bot -->"
|
||||
|
||||
# Severity ordering for display.
|
||||
_SEVERITY_ORDER = ["error", "action_required", "warning", "info"]
|
||||
_SEVERITY_ORDER = ["error", "action_required", "warning", "info", "debug"]
|
||||
|
||||
# Severities that trigger the "blocking issues" layout (vs. the
|
||||
# "looks good!" banner).
|
||||
|
|
@ -74,7 +74,7 @@ _SEVERITY_GROUP_HEADER = {
|
|||
class ReviewItem:
|
||||
"""A single piece of review information with a severity tag."""
|
||||
|
||||
severity: str # "error" | "action_required" | "warning" | "info"
|
||||
severity: str # "error" | "action_required" | "warning" | "info" | "debug"
|
||||
title: str # short section title, e.g. "package-lock.json"
|
||||
summary: str # one-line summary
|
||||
detail: str = "" # optional markdown detail (tables, bullet lists, etc.)
|
||||
|
|
@ -241,15 +241,15 @@ def _render_group(header: str, items: list[ReviewItem]) -> str:
|
|||
return f"{header}\n\n" + "\n\n---\n\n".join(blocks)
|
||||
|
||||
|
||||
def _render_info_details(items: list[ReviewItem]) -> str:
|
||||
"""Render each info item as its own collapsible ``<details>`` block."""
|
||||
def _render_debug_details(items: list[ReviewItem]) -> str:
|
||||
"""Render each debug item as its own collapsible ``<details>`` block."""
|
||||
blocks = []
|
||||
for item in items:
|
||||
inner = _render_item(item)
|
||||
blocks.append(
|
||||
f"<details>\n<summary>{item.title}</summary>\n\n{inner}\n\n</details>"
|
||||
)
|
||||
return "\n\n".join(blocks)
|
||||
return "### debug info\n\n" + "\n\n".join(blocks)
|
||||
|
||||
|
||||
def _render_pending_items(pending_jobs: list[str]) -> str:
|
||||
|
|
@ -263,13 +263,13 @@ def render_comment(items: list[ReviewItem], pending_jobs: list[str] | None = Non
|
|||
|
||||
Items are grouped by severity under ``##`` group headers, separated
|
||||
by ``---``. Errors and action_required items are always visible.
|
||||
Warnings are shown only when present. Info items are in a collapsible
|
||||
``<details>`` block. If ``pending_jobs`` is non-empty, a dimmed
|
||||
Warnings are shown only when present. Info items are visible; debug items
|
||||
are in a collapsible ``<details>`` block. If ``pending_jobs`` is non-empty, a dimmed
|
||||
``<sub>`` footer is appended listing jobs still running.
|
||||
|
||||
When there are no errors, action_required, or warnings (only info
|
||||
items, or nothing at all), a "looks good!" banner is shown at the top,
|
||||
and info items (if any) follow in a collapsible ``<details>`` block.
|
||||
When there are no errors, action_required, or warnings, an "all good!"
|
||||
banner is shown at the top. Info items remain visible and debug items
|
||||
follow in collapsible ``<details>`` blocks.
|
||||
"""
|
||||
pending = pending_jobs or []
|
||||
|
||||
|
|
@ -279,6 +279,7 @@ def render_comment(items: list[ReviewItem], pending_jobs: list[str] | None = Non
|
|||
by_severity.setdefault(item.severity, []).append(item)
|
||||
|
||||
info = by_severity.get("info", [])
|
||||
debug = by_severity.get("debug", [])
|
||||
has_blocking = any(by_severity.get(s) for s in _BLOCKING_SEVERITIES)
|
||||
|
||||
body = f"{MARKER}\n# ૮ >ﻌ< ა ci review\n\n"
|
||||
|
|
@ -287,7 +288,7 @@ def render_comment(items: list[ReviewItem], pending_jobs: list[str] | None = Non
|
|||
body += f"{commit_info}\n\n"
|
||||
|
||||
if not items and not pending:
|
||||
return f"{body}looks good to me!"
|
||||
return f"{body}all good!"
|
||||
|
||||
sections: list[str] = []
|
||||
|
||||
|
|
@ -296,9 +297,12 @@ def render_comment(items: list[ReviewItem], pending_jobs: list[str] | None = Non
|
|||
if group:
|
||||
sections.append(_render_group(_SEVERITY_GROUP_HEADER[sev], group))
|
||||
|
||||
# Info: collapsible <details>
|
||||
if info:
|
||||
sections.append(_render_info_details(info))
|
||||
sections.append(_render_group("## ℹ️ Info", info))
|
||||
|
||||
# Debug: collapsible <details>
|
||||
if debug:
|
||||
sections.append(_render_debug_details(debug))
|
||||
|
||||
if pending:
|
||||
body += _render_pending_items(pending)
|
||||
|
|
|
|||
|
|
@ -32,6 +32,7 @@ must never skip one a change could break:
|
|||
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
import os
|
||||
import sys
|
||||
|
||||
|
|
@ -89,6 +90,11 @@ def _is_ci_review(p: str) -> bool:
|
|||
return os.path.basename(p).startswith("eslint.config.")
|
||||
|
||||
|
||||
def ci_review_files(files: list[str]) -> list[str]:
|
||||
"""Return the CI-sensitive paths that need maintainer review."""
|
||||
return sorted({f.strip() for f in files if f.strip() and _is_ci_review(f.strip())})
|
||||
|
||||
|
||||
def classify(files: list[str]) -> dict[str, bool]:
|
||||
"""Map changed paths to ``{lane: should_run}``."""
|
||||
files = [f.strip() for f in files if f.strip()]
|
||||
|
|
@ -119,8 +125,12 @@ def classify(files: list[str]) -> dict[str, bool]:
|
|||
|
||||
|
||||
def main() -> int:
|
||||
lanes = classify(sys.stdin.read().splitlines())
|
||||
out = "\n".join(f"{k}={str(v).lower()}" for k, v in lanes.items())
|
||||
files = sys.stdin.read().splitlines()
|
||||
lanes = classify(files)
|
||||
out = "\n".join([
|
||||
*(f"{key}={str(value).lower()}" for key, value in lanes.items()),
|
||||
f"ci_review_files={json.dumps(ci_review_files(files))}",
|
||||
])
|
||||
if dest := os.environ.get("GITHUB_OUTPUT"):
|
||||
with open(dest, "a", encoding="utf-8") as fh:
|
||||
fh.write(out + "\n")
|
||||
|
|
|
|||
|
|
@ -0,0 +1,155 @@
|
|||
#!/usr/bin/env python3
|
||||
"""Select Desktop E2E visual evidence and build its CI review status."""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import argparse
|
||||
import hashlib
|
||||
import json
|
||||
import shutil
|
||||
from pathlib import Path
|
||||
|
||||
SOURCE = "playwright e2e"
|
||||
EVIDENCE_START = "<!-- hermes-e2e-evidence:start -->"
|
||||
EVIDENCE_END = "<!-- hermes-e2e-evidence:end -->"
|
||||
|
||||
|
||||
def _files(root: Path, pattern: str) -> list[Path]:
|
||||
return sorted(path for path in root.rglob(pattern) if path.is_file()) if root.exists() else []
|
||||
|
||||
|
||||
def _is_explicit_screenshot(path: Path) -> bool:
|
||||
"""Exclude Playwright's automatic and visual-comparator PNG outputs."""
|
||||
return not (
|
||||
path.name.startswith(("test-finished-", "test-failed-"))
|
||||
or path.name.endswith(("-actual.png", "-expected.png", "-diff.png"))
|
||||
)
|
||||
|
||||
|
||||
def build_manifest(results_dir: Path) -> dict:
|
||||
"""Record stable screenshot names from one E2E run for main/PR comparison."""
|
||||
screenshots = [path for path in _files(results_dir, "*.png") if _is_explicit_screenshot(path)]
|
||||
return {"version": 1, "screenshot_names": sorted({path.name for path in screenshots})}
|
||||
|
||||
|
||||
def _base_screenshot_names(path: Path | None) -> set[str] | None:
|
||||
"""Return ``None`` when main evidence is unavailable (never guess newness)."""
|
||||
if path is None or not path.is_file():
|
||||
return None
|
||||
try:
|
||||
data = json.loads(path.read_text(encoding="utf-8"))
|
||||
except json.JSONDecodeError:
|
||||
return None
|
||||
names = data.get("screenshot_names", []) if isinstance(data, dict) else []
|
||||
if not isinstance(data, dict) or not isinstance(names, list):
|
||||
return None
|
||||
return {name for name in names if isinstance(name, str)}
|
||||
|
||||
|
||||
def _stage_name(kind: str, path: Path, results_dir: Path) -> str:
|
||||
relative = path.relative_to(results_dir).as_posix()
|
||||
digest = hashlib.sha256(relative.encode("utf-8")).hexdigest()[:12]
|
||||
return f"{kind}-{digest}-{path.name}"
|
||||
|
||||
|
||||
def select_evidence(results_dir: Path, base_manifest: Path | None = None) -> dict:
|
||||
"""Select only screenshots new to main, plus every generated visual diff."""
|
||||
base_names = _base_screenshot_names(base_manifest)
|
||||
screenshots = [] if base_names is None else [
|
||||
path for path in _files(results_dir, "*.png")
|
||||
if _is_explicit_screenshot(path) and path.name not in base_names
|
||||
]
|
||||
diffs: list[dict[str, Path]] = []
|
||||
for diff in _files(results_dir, "*-diff.png"):
|
||||
stem = diff.with_name(diff.name.removesuffix("-diff.png"))
|
||||
entry = {"diff": diff}
|
||||
for kind in ("actual", "expected"):
|
||||
candidate = stem.with_name(f"{stem.name}-{kind}.png")
|
||||
if candidate.is_file():
|
||||
entry[kind] = candidate
|
||||
diffs.append(entry)
|
||||
return {"screenshots": screenshots, "diffs": diffs}
|
||||
|
||||
|
||||
def stage_evidence(results_dir: Path, evidence_dir: Path, selection: dict) -> dict:
|
||||
"""Copy selected PNGs into a flat, path-safe evidence artifact."""
|
||||
evidence_dir.mkdir(parents=True, exist_ok=True)
|
||||
staged: dict[Path, str] = {}
|
||||
|
||||
def stage(kind: str, path: Path) -> str:
|
||||
if path in staged:
|
||||
return staged[path]
|
||||
name = _stage_name(kind, path, results_dir)
|
||||
shutil.copyfile(path, evidence_dir / name)
|
||||
staged[path] = name
|
||||
return name
|
||||
|
||||
manifest = {"version": 1, "screenshots": [], "diffs": []}
|
||||
for screenshot in selection["screenshots"]:
|
||||
manifest["screenshots"].append({
|
||||
"name": screenshot.name,
|
||||
"file": stage("screenshot", screenshot),
|
||||
})
|
||||
for diff in selection["diffs"]:
|
||||
entry = {"name": diff["diff"].name.removesuffix("-diff.png"), "diff": stage("diff", diff["diff"])}
|
||||
for kind in ("actual", "expected"):
|
||||
if kind in diff:
|
||||
entry[kind] = stage(kind, diff[kind])
|
||||
manifest["diffs"].append(entry)
|
||||
|
||||
(evidence_dir / "e2e-evidence.json").write_text(
|
||||
json.dumps(manifest, sort_keys=True) + "\n", encoding="utf-8"
|
||||
)
|
||||
return manifest
|
||||
|
||||
|
||||
def build_status(selection: dict, artifact_url: str = "") -> list[dict]:
|
||||
"""Return the review status. The trusted publisher replaces its marker."""
|
||||
screenshots = selection["screenshots"]
|
||||
diffs = selection["diffs"]
|
||||
if not screenshots and not diffs:
|
||||
return []
|
||||
|
||||
summary_parts = []
|
||||
if screenshots:
|
||||
summary_parts.append(
|
||||
f"{len(screenshots)} new screenshot{'s' if len(screenshots) != 1 else ''} vs main"
|
||||
)
|
||||
if diffs:
|
||||
summary_parts.append(f"{len(diffs)} visual diff{'s' if len(diffs) != 1 else ''}")
|
||||
|
||||
result: dict[str, str] = {
|
||||
"kind": "info",
|
||||
"title": "Desktop E2E visual evidence",
|
||||
"summary": "; ".join(summary_parts) + ".",
|
||||
"detail": "\n".join((EVIDENCE_START, "<sub>inline evidence is publishing...</sub>", EVIDENCE_END)),
|
||||
}
|
||||
if artifact_url:
|
||||
result["link"] = artifact_url
|
||||
result["link_label"] = "View test artifacts"
|
||||
return [{"source": SOURCE, "results": [result]}]
|
||||
|
||||
|
||||
def main() -> int:
|
||||
parser = argparse.ArgumentParser(description=__doc__)
|
||||
parser.add_argument("--results-dir", type=Path, required=True)
|
||||
parser.add_argument("--base-manifest", type=Path)
|
||||
parser.add_argument("--manifest-output", type=Path, required=True)
|
||||
parser.add_argument("--evidence-dir", type=Path, required=True)
|
||||
parser.add_argument("--artifact-url", default="")
|
||||
parser.add_argument("--output", type=Path, required=True)
|
||||
args = parser.parse_args()
|
||||
|
||||
args.manifest_output.write_text(
|
||||
json.dumps(build_manifest(args.results_dir), sort_keys=True) + "\n", encoding="utf-8"
|
||||
)
|
||||
selection = select_evidence(args.results_dir, args.base_manifest)
|
||||
stage_evidence(args.results_dir, args.evidence_dir, selection)
|
||||
args.output.write_text(
|
||||
json.dumps(build_status(selection, args.artifact_url)) + "\n", encoding="utf-8"
|
||||
)
|
||||
return 0
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
raise SystemExit(main())
|
||||
|
|
@ -27,8 +27,10 @@ with the verification checklist.
|
|||
from __future__ import annotations
|
||||
|
||||
import argparse
|
||||
import hashlib
|
||||
import json
|
||||
import sys
|
||||
from urllib.parse import quote
|
||||
|
||||
# The source identifier used for error-synthesis exclusion. This must
|
||||
# match (as a normalized substring) the job name as it appears in the
|
||||
|
|
@ -41,24 +43,57 @@ import sys
|
|||
SOURCE = "review-label-gate"
|
||||
|
||||
|
||||
def _ci_review_detail(
|
||||
files_json: str, repo_url: str, base_sha: str, head_sha: str,
|
||||
) -> str:
|
||||
"""Render links to the changed CI-sensitive files that triggered review."""
|
||||
try:
|
||||
files = json.loads(files_json)
|
||||
except (json.JSONDecodeError, TypeError):
|
||||
return ""
|
||||
if not isinstance(files, list) or not repo_url or not base_sha or not head_sha:
|
||||
return ""
|
||||
|
||||
links = []
|
||||
for path in files:
|
||||
if not isinstance(path, str) or not path:
|
||||
continue
|
||||
label = path.replace("\\", "\\\\").replace("[", "\\[").replace("]", "\\]")
|
||||
path_hash = hashlib.sha256(path.encode()).hexdigest()
|
||||
url = (
|
||||
f"{repo_url}/compare/{quote(base_sha, safe='')}...{quote(head_sha, safe='')}"
|
||||
f"#diff-{path_hash}"
|
||||
)
|
||||
links.append(f"- [`{label}`]({url})")
|
||||
return "**Sensitive files changed:**\n" + "\n".join(links) if links else ""
|
||||
|
||||
|
||||
def build_results(
|
||||
ci_review: bool,
|
||||
mcp_catalog: bool,
|
||||
supply_chain: bool,
|
||||
label_present: bool,
|
||||
ci_review_files: str = "[]",
|
||||
repo_url: str = "",
|
||||
base_sha: str = "",
|
||||
head_sha: str = "",
|
||||
) -> list[dict]:
|
||||
"""Build the list of result objects for this source."""
|
||||
results: list[dict] = []
|
||||
|
||||
if ci_review:
|
||||
detail = _ci_review_detail(ci_review_files, repo_url, base_sha, head_sha)
|
||||
if label_present:
|
||||
results.append({
|
||||
result = {
|
||||
"kind": "info",
|
||||
"title": "CI-sensitive file review",
|
||||
"summary": "`ci-reviewed` label is present.",
|
||||
})
|
||||
"summary": (
|
||||
"PR touches sensitive files, but the `ci-reviewed` label has been "
|
||||
"added, approving them."
|
||||
),
|
||||
}
|
||||
else:
|
||||
results.append({
|
||||
result = {
|
||||
"kind": "action_required",
|
||||
"title": "CI-sensitive file review",
|
||||
"summary": (
|
||||
|
|
@ -72,12 +107,15 @@ def build_results(
|
|||
"- no workflow changes that widen permissions or remove guards,\n"
|
||||
"- no composite action changes that alter what gets executed."
|
||||
),
|
||||
})
|
||||
}
|
||||
if detail:
|
||||
result["detail"] = detail
|
||||
results.append(result)
|
||||
|
||||
if mcp_catalog:
|
||||
if label_present:
|
||||
results.append({
|
||||
"kind": "info",
|
||||
"kind": "debug",
|
||||
"title": "MCP catalog security review",
|
||||
"summary": "`ci-reviewed` label is present.",
|
||||
})
|
||||
|
|
@ -119,9 +157,16 @@ def build_statuses(
|
|||
mcp_catalog: bool,
|
||||
supply_chain: bool,
|
||||
label_present: bool,
|
||||
ci_review_files: str = "[]",
|
||||
repo_url: str = "",
|
||||
base_sha: str = "",
|
||||
head_sha: str = "",
|
||||
) -> list[dict]:
|
||||
"""Build the full review_status array (one entry with a results list)."""
|
||||
results = build_results(ci_review, mcp_catalog, supply_chain, label_present)
|
||||
results = build_results(
|
||||
ci_review, mcp_catalog, supply_chain, label_present,
|
||||
ci_review_files, repo_url, base_sha, head_sha,
|
||||
)
|
||||
if not results:
|
||||
return []
|
||||
return [{"source": SOURCE, "results": results}]
|
||||
|
|
@ -131,18 +176,27 @@ def main() -> int:
|
|||
parser = argparse.ArgumentParser(description=__doc__)
|
||||
parser.add_argument("--ci-review", action="store_true",
|
||||
help="Whether CI-sensitive files changed.")
|
||||
parser.add_argument("--ci-review-files", default="[]",
|
||||
help="JSON list of CI-sensitive files changed.")
|
||||
parser.add_argument("--mcp-catalog", action="store_true",
|
||||
help="Whether the MCP catalog / installer changed.")
|
||||
parser.add_argument("--supply-chain", action="store_true",
|
||||
help="Whether the critical supply-chain scanner found a risk.")
|
||||
parser.add_argument("--label-present", action="store_true",
|
||||
help="Whether the ci-reviewed label is present.")
|
||||
parser.add_argument("--repo-url", default="",
|
||||
help="Repository URL used for changed-file links.")
|
||||
parser.add_argument("--base-sha", default="",
|
||||
help="Pull request base SHA used for changed-file links.")
|
||||
parser.add_argument("--head-sha", default="",
|
||||
help="Pull request head SHA used for changed-file links.")
|
||||
parser.add_argument("--output", default="-",
|
||||
help="Output file ('-' for stdout, or a GITHUB_OUTPUT path).")
|
||||
args = parser.parse_args()
|
||||
|
||||
statuses = build_statuses(
|
||||
args.ci_review, args.mcp_catalog, args.supply_chain, args.label_present
|
||||
args.ci_review, args.mcp_catalog, args.supply_chain, args.label_present,
|
||||
args.ci_review_files, args.repo_url, args.base_sha, args.head_sha,
|
||||
)
|
||||
json_str = json.dumps(statuses)
|
||||
|
||||
|
|
|
|||
|
|
@ -352,6 +352,13 @@ def _merge_statuses(
|
|||
return json.dumps(merged) if merged else ""
|
||||
|
||||
|
||||
def _commit_info_for_state(commit_info: str, pending: list[str]) -> str:
|
||||
"""Use past tense in the final comment after every CI job completes."""
|
||||
if pending:
|
||||
return commit_info
|
||||
return commit_info.replace("<sub>running on ", "<sub>ran on ", 1)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Polling loop
|
||||
# ---------------------------------------------------------------------------
|
||||
|
|
@ -410,11 +417,12 @@ def run(
|
|||
print(f" Found ci-timings artifact with {len(artifact_statuses)} status entries")
|
||||
|
||||
merged_json = _merge_statuses(base_statuses, artifact_statuses)
|
||||
current_commit_info = _commit_info_for_state(commit_info, pending)
|
||||
|
||||
body = build_comment_body(
|
||||
asm, completed, pending, run_url, job_urls,
|
||||
merged_json,
|
||||
commit_info,
|
||||
current_commit_info,
|
||||
)
|
||||
|
||||
if body != last_body:
|
||||
|
|
|
|||
|
|
@ -909,7 +909,7 @@ def generate_review_status(
|
|||
"""Produce a review_status JSON array for the CI timings review section.
|
||||
|
||||
Returns a list with one ``{source, results: [...]}`` entry. The
|
||||
result kind is ``"info"`` or ``"warning"`` (timings is never error —
|
||||
result kind is ``"debug"`` or ``"warning"`` (timings is never error —
|
||||
it's an observability job). *summary* is a single short line suitable
|
||||
for the PR comment. *detail* has the per-job deltas as a markdown
|
||||
fragment.
|
||||
|
|
@ -917,7 +917,7 @@ def generate_review_status(
|
|||
stats = compute_stats(timings, baseline)
|
||||
|
||||
if baseline is None:
|
||||
severity = "info"
|
||||
severity = "debug"
|
||||
summary = f"Wall time {fmt_dur(stats['wall'])} (no baseline yet)."
|
||||
else:
|
||||
wall = stats["wall"]
|
||||
|
|
@ -928,10 +928,10 @@ def generate_review_status(
|
|||
if pct > _TIMINGS_WARN_PCT * 100:
|
||||
severity = "warning"
|
||||
else:
|
||||
severity = "info"
|
||||
severity = "debug"
|
||||
else:
|
||||
wall_str = f"Wall time {fmt_dur(wall)}."
|
||||
severity = "info"
|
||||
severity = "debug"
|
||||
|
||||
if stats["slower"]:
|
||||
wall_str += f" {stats['slower']} job(s) slower,"
|
||||
|
|
|
|||
|
|
@ -1,7 +1,7 @@
|
|||
"""Tests for scripts/ci/assemble_review_comment.py.
|
||||
|
||||
The assembler collects status from every CI sub-workflow into ReviewItems
|
||||
classified by severity (error / action_required / warning / info), then
|
||||
classified by severity (error / action_required / warning / info / debug), then
|
||||
renders them into a single PR comment body.
|
||||
|
||||
Status data comes from two sources:
|
||||
|
|
@ -16,7 +16,7 @@ Layout rules tested here:
|
|||
- each item is a ### section under its group header
|
||||
- errors + action_required always visible
|
||||
- warnings shown only when present
|
||||
- info in a collapsible <details> block
|
||||
- info above the fold; debug in a collapsible <details> block
|
||||
- sections separated by ---
|
||||
- how_to_fix rendered at bottom of action_required items
|
||||
- empty → clean banner
|
||||
|
|
@ -93,6 +93,16 @@ def test_statuses_info():
|
|||
assert sources == {"review-label-gate"}
|
||||
|
||||
|
||||
def test_statuses_debug():
|
||||
statuses = _status("ci-timings", [{
|
||||
"kind": "debug",
|
||||
"title": "CI timings",
|
||||
"summary": "No regression.",
|
||||
}])
|
||||
items, _ = _mod.collect_from_statuses(statuses)
|
||||
assert items[0].severity == "debug"
|
||||
|
||||
|
||||
def test_statuses_multiple_results_same_source():
|
||||
"""One source can emit multiple results of different kinds."""
|
||||
statuses = _status("review-label-gate", [
|
||||
|
|
@ -238,24 +248,24 @@ def test_failed_jobs_fallback_to_run_url():
|
|||
|
||||
|
||||
def test_render_empty_shows_clean_banner():
|
||||
"""Completely clean — dog kaomoji + 'looks good' banner, no sections."""
|
||||
"""Completely clean — dog kaomoji + 'all good' banner, no sections."""
|
||||
body = _mod.render_comment([])
|
||||
assert body.startswith(MARKER)
|
||||
assert "૮ >ﻌ< ა" in body
|
||||
assert "looks good to me!" in body
|
||||
assert "all good!" in body
|
||||
assert "##" not in body # no section headers
|
||||
|
||||
|
||||
def test_render_info_only_shows_details():
|
||||
"""Info items only — header + collapsible details, no blocking sections."""
|
||||
def test_render_info_only_is_visible_above_the_fold():
|
||||
"""Info items are visible rather than hidden in debug details."""
|
||||
items = [
|
||||
ReviewItem(severity="info", title="lockfile", summary="No changes."),
|
||||
ReviewItem(severity="info", title="timings", summary="OK."),
|
||||
]
|
||||
body = _mod.render_comment(items)
|
||||
assert "૮ >ﻌ< ა" in body
|
||||
assert "<details>" in body
|
||||
assert "</details>" in body
|
||||
assert "## ℹ️ Info" in body
|
||||
assert "<details>" not in body
|
||||
assert "No changes." in body
|
||||
assert "OK." in body
|
||||
# No blocking sections
|
||||
|
|
@ -263,12 +273,12 @@ def test_render_info_only_shows_details():
|
|||
assert "## ⚠️" not in body
|
||||
|
||||
|
||||
def test_render_info_only_with_pending_shows_details_plus_footer():
|
||||
def test_render_info_only_with_pending_shows_info_plus_footer():
|
||||
items = [ReviewItem(severity="info", title="lockfile", summary="No changes.")]
|
||||
body = _mod.render_comment(items, pending_jobs=["ci-timings"])
|
||||
assert "૮ >ﻌ< ა" in body
|
||||
assert "Still running" in body
|
||||
assert "<details>" in body
|
||||
assert "## ℹ️ Info" in body
|
||||
assert "Still running" in body
|
||||
assert "`ci-timings`" in body
|
||||
|
||||
|
|
@ -348,17 +358,19 @@ def test_render_errors_always_visible():
|
|||
assert "### tests" in body
|
||||
assert "Job **tests** failed." in body
|
||||
assert "[View job](https://run)" in body
|
||||
assert "<details>" in body
|
||||
assert "## ℹ️ Info" in body
|
||||
assert "No changes." in body
|
||||
|
||||
|
||||
def test_render_info_in_collapsible_details():
|
||||
"""Each info item is its own <details> block."""
|
||||
def test_render_debug_in_collapsible_details():
|
||||
"""Debug items have a small label and each has its own <details> block."""
|
||||
items = [
|
||||
ReviewItem(severity="info", title="lockfile", summary="No changes."),
|
||||
ReviewItem(severity="info", title="timings", summary="OK."),
|
||||
ReviewItem(severity="debug", title="lockfile", summary="No changes."),
|
||||
ReviewItem(severity="debug", title="timings", summary="OK."),
|
||||
]
|
||||
body = _mod.render_comment(items)
|
||||
assert "### debug info" in body
|
||||
assert body.index("### debug info") < body.index("<details>")
|
||||
assert body.count("<details>") == 2
|
||||
assert body.count("</details>") == 2
|
||||
assert "<summary>lockfile</summary>" in body
|
||||
|
|
@ -367,9 +379,10 @@ def test_render_info_in_collapsible_details():
|
|||
assert "OK." in body
|
||||
|
||||
|
||||
def test_render_order_errors_then_action_then_warn_then_info():
|
||||
def test_render_order_errors_then_action_then_warn_then_info_then_debug():
|
||||
items = [
|
||||
ReviewItem(severity="info", title="i", summary="info"),
|
||||
ReviewItem(severity="debug", title="d", summary="debug"),
|
||||
ReviewItem(severity="warning", title="w", summary="warn"),
|
||||
ReviewItem(severity="action_required", title="a", summary="action"),
|
||||
ReviewItem(severity="error", title="e", summary="error"),
|
||||
|
|
@ -378,8 +391,9 @@ def test_render_order_errors_then_action_then_warn_then_info():
|
|||
error_pos = body.index("## ❌ Job failures")
|
||||
action_pos = body.index("## ⚠️ Action required")
|
||||
warn_pos = body.index("## ⚠️ Warnings")
|
||||
info_pos = body.index("<details>")
|
||||
assert error_pos < action_pos < warn_pos < info_pos
|
||||
info_pos = body.index("## ℹ️ Info")
|
||||
debug_pos = body.index("<details>")
|
||||
assert error_pos < action_pos < warn_pos < info_pos < debug_pos
|
||||
|
||||
|
||||
# ─── render_comment (pending jobs) ────────────────────────────────────
|
||||
|
|
@ -422,7 +436,7 @@ def test_assemble_all_skipped_clean_banner():
|
|||
body = _mod.assemble()
|
||||
assert body.startswith(MARKER)
|
||||
assert "૮ >ﻌ< ა" in body
|
||||
assert "looks good to me!" in body
|
||||
assert "all good!" in body
|
||||
assert "##" not in body
|
||||
|
||||
|
||||
|
|
@ -457,6 +471,32 @@ def test_assemble_with_review_statuses():
|
|||
assert "## ❌ Job failures" not in body
|
||||
|
||||
|
||||
def test_assemble_review_status_detail_renders_sensitive_file_links():
|
||||
statuses = _status("review-label-gate", [{
|
||||
"kind": "action_required",
|
||||
"title": "CI-sensitive file review",
|
||||
"summary": "Changes detected.",
|
||||
"detail": "**Sensitive files:**\n- [`ci.yml`](https://example.test/ci.yml)",
|
||||
}])
|
||||
body = _mod.assemble(review_statuses_json=statuses)
|
||||
assert "**Sensitive files:**" in body
|
||||
assert "[`ci.yml`](https://example.test/ci.yml)" in body
|
||||
|
||||
|
||||
def test_assemble_info_keeps_screenshot_details_visible_below_its_summary():
|
||||
statuses = _status("playwright e2e", [{
|
||||
"kind": "info",
|
||||
"title": "Desktop E2E screenshots",
|
||||
"summary": "1 screenshot captured; 0 visual diffs.",
|
||||
"detail": "<details>\n<summary>1 captured screenshot</summary>\n\n- [`proof.png`](https://example.test/artifact)\n\n</details>",
|
||||
}])
|
||||
body = _mod.assemble(review_statuses_json=statuses)
|
||||
assert "## ℹ️ Info" in body
|
||||
assert "1 screenshot captured; 0 visual diffs." in body
|
||||
assert "<summary>1 captured screenshot</summary>" in body
|
||||
assert "[`proof.png`](https://example.test/artifact)" in body
|
||||
|
||||
|
||||
def test_assemble_pending_jobs():
|
||||
body = _mod.assemble(pending_jobs=["ci-timings"])
|
||||
assert "Still running" in body
|
||||
|
|
@ -473,9 +513,9 @@ def test_assemble_with_items_and_pending():
|
|||
|
||||
|
||||
def test_assemble_with_timings_status():
|
||||
"""Timings status from the nested format renders as info or warning."""
|
||||
"""Timings status from the nested format renders as debug or warning."""
|
||||
statuses = _status("ci-timings", [{
|
||||
"kind": "info",
|
||||
"kind": "debug",
|
||||
"title": "CI timings",
|
||||
"summary": "Wall time 3m (no baseline yet).",
|
||||
"detail": "",
|
||||
|
|
@ -490,14 +530,14 @@ def test_assemble_with_timings_status():
|
|||
|
||||
|
||||
def test_assemble_with_lockfile_status():
|
||||
"""Lockfile no-changes status renders as info in the details block."""
|
||||
"""Lockfile no-changes status renders as visible info."""
|
||||
statuses = _status("lockfile-diff", [{
|
||||
"kind": "info",
|
||||
"title": "package-lock.json",
|
||||
"summary": "No lockfile changes — locked versions match the target branch.",
|
||||
}])
|
||||
body = _mod.assemble(review_statuses_json=statuses)
|
||||
assert "<details>" in body
|
||||
assert "## ℹ️ Info" in body
|
||||
assert "### package-lock.json" in body
|
||||
assert "No lockfile changes" in body
|
||||
|
||||
|
|
@ -602,7 +642,7 @@ def test_assemble_passes_commit_info():
|
|||
"""assemble() passes commit_info through to render_comment."""
|
||||
body = _mod.assemble(commit_info="<sub>running on abc1234</sub>")
|
||||
assert "running on abc1234" in body
|
||||
assert "looks good to me!" in body
|
||||
assert "all good!" in body
|
||||
|
||||
|
||||
def test_render_both_emitted_link_and_job_url():
|
||||
|
|
|
|||
|
|
@ -19,6 +19,7 @@ if _spec is None or _spec.loader is None:
|
|||
_mod = importlib.util.module_from_spec(_spec)
|
||||
_spec.loader.exec_module(_mod)
|
||||
classify = _mod.classify
|
||||
ci_review_files = _mod.ci_review_files
|
||||
|
||||
DEFAULT = {
|
||||
"python": True,
|
||||
|
|
@ -130,3 +131,15 @@ CASES = {
|
|||
@pytest.mark.parametrize("files,expected", CASES.values(), ids=CASES.keys())
|
||||
def test_classify(files, expected):
|
||||
assert classify(files) == expected
|
||||
|
||||
|
||||
def test_ci_review_files_returns_only_sensitive_paths_sorted_and_unique():
|
||||
assert ci_review_files([
|
||||
"apps/desktop/src/app.tsx",
|
||||
".github/workflows/ci.yml",
|
||||
"apps/desktop/eslint.config.mjs",
|
||||
".github/workflows/ci.yml",
|
||||
]) == [
|
||||
".github/workflows/ci.yml",
|
||||
"apps/desktop/eslint.config.mjs",
|
||||
]
|
||||
|
|
|
|||
|
|
@ -0,0 +1,57 @@
|
|||
"""Tests for scripts/ci/e2e_screenshot_status.py."""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import importlib.util
|
||||
import sys
|
||||
from pathlib import Path
|
||||
|
||||
_PATH = Path(__file__).resolve().parents[2] / "scripts" / "ci" / "e2e_screenshot_status.py"
|
||||
_spec = importlib.util.spec_from_file_location("e2e_screenshot_status", _PATH)
|
||||
if _spec is None or _spec.loader is None:
|
||||
raise ImportError("Failed to load e2e_screenshot_status.py")
|
||||
_mod = importlib.util.module_from_spec(_spec)
|
||||
_spec.loader.exec_module(_mod)
|
||||
|
||||
|
||||
def test_status_selects_only_new_explicit_screenshots_and_all_diffs(tmp_path):
|
||||
for name in (
|
||||
"explicit-proof.png",
|
||||
"test-finished-1.png",
|
||||
"visual-actual.png",
|
||||
"visual-expected.png",
|
||||
"visual-diff.png",
|
||||
):
|
||||
(tmp_path / name).write_bytes(b"png")
|
||||
|
||||
base_manifest = tmp_path / "main-manifest.json"
|
||||
base_manifest.write_text('{"screenshot_names":["already-on-main.png"]}', encoding="utf-8")
|
||||
(tmp_path / "already-on-main.png").write_bytes(b"png")
|
||||
|
||||
selection = _mod.select_evidence(tmp_path, base_manifest)
|
||||
status = _mod.build_status(selection, "https://github.test/artifacts/1")
|
||||
|
||||
result = status[0]["results"][0]
|
||||
assert result["kind"] == "info"
|
||||
assert result["summary"] == "1 new screenshot vs main; 1 visual diff."
|
||||
assert _mod.EVIDENCE_START in result["detail"]
|
||||
assert "already-on-main.png" not in result["detail"]
|
||||
assert result["link"] == "https://github.test/artifacts/1"
|
||||
|
||||
|
||||
def test_cli_output_ends_with_newline_for_github_output_delimiter(tmp_path, monkeypatch):
|
||||
output = tmp_path / "review-status.json"
|
||||
manifest = tmp_path / "main-manifest.json"
|
||||
evidence_dir = tmp_path / "evidence"
|
||||
monkeypatch.setattr(sys, "argv", [
|
||||
"e2e_screenshot_status.py",
|
||||
"--results-dir", str(tmp_path),
|
||||
"--manifest-output", str(manifest),
|
||||
"--evidence-dir", str(evidence_dir),
|
||||
"--output", str(output),
|
||||
])
|
||||
|
||||
assert _mod.main() == 0
|
||||
assert output.read_text(encoding="utf-8") == "[]\n"
|
||||
assert manifest.read_text(encoding="utf-8") == '{"screenshot_names": [], "version": 1}\n'
|
||||
assert evidence_dir.joinpath("e2e-evidence.json").is_file()
|
||||
|
|
@ -0,0 +1,60 @@
|
|||
"""Tests for scripts/ci/emit_review_status.py."""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import importlib.util
|
||||
import sys
|
||||
from pathlib import Path
|
||||
|
||||
_PATH = Path(__file__).resolve().parents[2] / "scripts" / "ci" / "emit_review_status.py"
|
||||
_spec = importlib.util.spec_from_file_location("emit_review_status", _PATH)
|
||||
if _spec is None or _spec.loader is None:
|
||||
raise ImportError("Failed to load emit_review_status.py")
|
||||
_mod = importlib.util.module_from_spec(_spec)
|
||||
sys.modules["emit_review_status"] = _mod
|
||||
_spec.loader.exec_module(_mod)
|
||||
|
||||
|
||||
def test_ci_review_status_links_to_each_sensitive_file_change():
|
||||
results = _mod.build_results(
|
||||
ci_review=True,
|
||||
mcp_catalog=False,
|
||||
supply_chain=False,
|
||||
label_present=False,
|
||||
ci_review_files='[".github/workflows/ci.yml", "apps/desktop/eslint.config.mjs"]',
|
||||
repo_url="https://github.com/nousresearch/hermes-agent",
|
||||
base_sha="base456",
|
||||
head_sha="abc123",
|
||||
)
|
||||
|
||||
assert results[0]["detail"] == (
|
||||
"**Sensitive files changed:**\n"
|
||||
"- [`.github/workflows/ci.yml`](https://github.com/nousresearch/hermes-agent/compare/base456...abc123#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03f)\n"
|
||||
"- [`apps/desktop/eslint.config.mjs`](https://github.com/nousresearch/hermes-agent/compare/base456...abc123#diff-a45471520795db6e46840d1ba2a82c1f8a2841039bd60fb50624488c5f192438)"
|
||||
)
|
||||
|
||||
|
||||
def test_approved_ci_review_is_visible_info():
|
||||
results = _mod.build_results(
|
||||
ci_review=True,
|
||||
mcp_catalog=False,
|
||||
supply_chain=False,
|
||||
label_present=True,
|
||||
ci_review_files='[".github/workflows/ci.yml"]',
|
||||
repo_url="https://github.com/nousresearch/hermes-agent",
|
||||
base_sha="base456",
|
||||
head_sha="abc123",
|
||||
)
|
||||
|
||||
assert results == [{
|
||||
"kind": "info",
|
||||
"title": "CI-sensitive file review",
|
||||
"summary": (
|
||||
"PR touches sensitive files, but the `ci-reviewed` label has been "
|
||||
"added, approving them."
|
||||
),
|
||||
"detail": (
|
||||
"**Sensitive files changed:**\n"
|
||||
"- [`.github/workflows/ci.yml`](https://github.com/nousresearch/hermes-agent/compare/base456...abc123#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03f)"
|
||||
),
|
||||
}]
|
||||
|
|
@ -160,3 +160,15 @@ def test_classify_unknown_status_skipped():
|
|||
completed, pending, job_urls = _mod.classify_jobs(jobs)
|
||||
assert completed == {}
|
||||
assert pending == []
|
||||
|
||||
|
||||
def test_commit_info_uses_present_tense_while_jobs_are_pending():
|
||||
info = "<sub>running on [abc1234](https://commit-url) — fix: thing</sub>"
|
||||
assert _mod._commit_info_for_state(info, ["Python tests"]) == info
|
||||
|
||||
|
||||
def test_commit_info_uses_past_tense_after_jobs_complete():
|
||||
info = "<sub>running on [abc1234](https://commit-url) — fix: thing</sub>"
|
||||
assert _mod._commit_info_for_state(info, []) == (
|
||||
"<sub>ran on [abc1234](https://commit-url) — fix: thing</sub>"
|
||||
)
|
||||
|
|
|
|||
|
|
@ -53,28 +53,28 @@ def _result(statuses: list[dict]) -> dict:
|
|||
return results[0]
|
||||
|
||||
|
||||
def test_no_baseline_is_info():
|
||||
def test_no_baseline_is_debug():
|
||||
t = _timings([_job("tests", 60.0)])
|
||||
result = _result(_mod.generate_review_status(t, None))
|
||||
assert result["kind"] == "info"
|
||||
assert result["kind"] == "debug"
|
||||
assert "no baseline" in result["summary"].lower()
|
||||
assert "link" not in result # no report_url → no link field
|
||||
|
||||
|
||||
def test_no_regression_is_info():
|
||||
def test_no_regression_is_debug():
|
||||
cur = _timings([_job("tests", 60.0)])
|
||||
bl = _timings([_job("tests", 60.0)])
|
||||
result = _result(_mod.generate_review_status(cur, bl))
|
||||
assert result["kind"] == "info"
|
||||
assert result["kind"] == "debug"
|
||||
assert "+0.0%" in result["summary"]
|
||||
|
||||
|
||||
def test_small_regression_is_info():
|
||||
def test_small_regression_is_debug():
|
||||
cur = _timings([_job("tests", 65.0)])
|
||||
bl = _timings([_job("tests", 60.0)])
|
||||
result = _result(_mod.generate_review_status(cur, bl))
|
||||
# +8.3% — well under the 25% warning threshold
|
||||
assert result["kind"] == "info"
|
||||
assert result["kind"] == "debug"
|
||||
|
||||
|
||||
def test_large_regression_is_warning():
|
||||
|
|
@ -86,11 +86,11 @@ def test_large_regression_is_warning():
|
|||
assert "+33" in result["summary"]
|
||||
|
||||
|
||||
def test_improvement_is_info():
|
||||
def test_improvement_is_debug():
|
||||
cur = _timings([_job("tests", 40.0)])
|
||||
bl = _timings([_job("tests", 60.0)])
|
||||
result = _result(_mod.generate_review_status(cur, bl))
|
||||
assert result["kind"] == "info"
|
||||
assert result["kind"] == "debug"
|
||||
assert "-33" in result["summary"]
|
||||
|
||||
|
||||
|
|
@ -137,7 +137,7 @@ def test_nested_format_structure():
|
|||
assert isinstance(statuses[0]["results"], list)
|
||||
assert len(statuses[0]["results"]) == 1
|
||||
r = statuses[0]["results"][0]
|
||||
assert r["kind"] == "info"
|
||||
assert r["kind"] == "debug"
|
||||
assert r["title"] == "CI timings"
|
||||
assert "summary" in r
|
||||
assert "detail" in r
|
||||
|
|
|
|||
Loading…
Reference in New Issue