diff --git a/action.yml b/action.yml index ab7f483..7ca8012 100644 --- a/action.yml +++ b/action.yml @@ -183,6 +183,9 @@ outputs: seed_source: description: 'Which state the review head analysis grew from: pr-chain or base.' value: ${{ steps.review_analyze.outputs.seed_source }} + base_analysis_method: + description: 'How the analysis of the review base commit was obtained: reused, incremental, or full.' + value: ${{ steps.review_analyze.outputs.base_analysis_method }} merge_base_sha: description: 'Merge base used as the review comparison baseline.' value: ${{ steps.guard.outputs.merge_base_sha }} @@ -533,8 +536,14 @@ runs: REVIEW_BASE_REPO: ${{ steps.guard.outputs.base_repo }} PR_NUMBER: ${{ steps.guard.outputs.pr_number }} BASE_DIR: ${{ runner.temp }}/cb-state/${{ github.action }}/base + BASE_FETCH_SECONDS: ${{ steps.fetch_base.outputs.seconds }} WARMSTART_DIR: ${{ runner.temp }}/cb-state/${{ github.action }}/warmstart RENEW_BASE: ${{ steps.fetch_base.outputs.renew }} + # For rewriting the progress comment while a base is built from scratch. + PROGRESS_HEADER: ${{ steps.guard.outputs.comment_id }} + BASE_REF: ${{ steps.guard.outputs.base_ref }} + REPOSITORY: ${{ github.repository }} + GH_HOST: ${{ github.server_url }} STAGE_DIR: ${{ runner.temp }}/cb-state/${{ github.action }}/out ENGINE_VERSION: ${{ steps.state.outputs.engine_version }} CFG_HASH: ${{ steps.state.outputs.cfg_hash }} @@ -608,6 +617,10 @@ runs: SEED_SOURCE: ${{ steps.review_analyze.outputs.seed_source }} CHAIN_DEPTH: ${{ steps.review_analyze.outputs.chain_depth }} ANALYSED_FILES_CHANGED: ${{ steps.review_render.outputs.analysed_files_changed }} + BASE_ANALYSIS_METHOD: ${{ steps.review_analyze.outputs.base_analysis_method }} + BASE_ANALYSIS_REASON: ${{ steps.review_analyze.outputs.base_analysis_reason }} + BASE_SECONDS: ${{ steps.review_analyze.outputs.base_seconds }} + HEAD_SECONDS: ${{ steps.review_analyze.outputs.head_seconds }} run: "$GITHUB_ACTION_PATH/scripts/action/build-review-artifact.sh" - name: Upload review artifact @@ -639,6 +652,10 @@ runs: HEAD_SHA: ${{ steps.guard.outputs.head_sha }} # Analysed files whose content hash differs between base and head, from the render step. ANALYSED_FILES_CHANGED: ${{ steps.review_render.outputs.analysed_files_changed }} + BASE_ANALYSIS_METHOD: ${{ steps.review_analyze.outputs.base_analysis_method }} + BASE_ANALYSIS_REASON: ${{ steps.review_analyze.outputs.base_analysis_reason }} + BASE_SECONDS: ${{ steps.review_analyze.outputs.base_seconds }} + HEAD_SECONDS: ${{ steps.review_analyze.outputs.head_seconds }} run: "$GITHUB_ACTION_PATH/scripts/action/build-review-comment.sh" - name: Post review comment diff --git a/docs/COMMIT_STRATEGY.md b/docs/COMMIT_STRATEGY.md index 2849dfb..9f9fdcf 100644 --- a/docs/COMMIT_STRATEGY.md +++ b/docs/COMMIT_STRATEGY.md @@ -70,7 +70,13 @@ never fires. | `base_artifact_id` | string | **which one**, since two artifacts can share that name and disagree: the engine is not deterministic, and a sync run publishes bases for the same commit | | `merge_base_resolved` | **boolean** | `false` means the merge base could not be resolved, so the comparison is against `base_sha` | | `base_sha` | string | the base branch tip when the event fired — *not* what was compared against | -| `pr_number`, `mode`, `seed_source`, `chain_depth` | string | provenance; nothing rendering a diagram needs them | +| `kind` | string | always `review`, so a reader can tell this artifact from a base or warm-start bundle | +| `analysed_files_changed` | string | analysed files whose content hash differs between base and head; `unknown` when the analyses cannot say | +| `base_analysis_method` | string | how this run obtained the analysis of the merge base: `reused` (the merge base already had one: its saved artifact, or a committed baseline with nothing to catch up), `incremental` (an earlier commit's analysis updated to the merge base) or `full` (analyzed from scratch in this run). Whichever, the base graph is the merge base's own analysis | +| `base_analysis_reason` | string | the method in words, e.g. `updated the analysis of 9f8e7d6 to a1b2c3d, 4 commits caught up`; where an incremental run started and how far it caught up are detail here, present when known | +| `base_seconds` | string | wall time spent obtaining the base, the artifact lookup included | +| `head_seconds` | string | wall time of the head analysis | +| `pr_number`, `mode`, `seed_source`, `chain_depth` | string | provenance; nothing rendering a diagram needs them. `mode` is the head's engine mode; `base_analysis_method` answers for the base | **A sync run** publishes the base graph under both the commit it analyzed and the baseline commit it writes on top, because a pull request opened either side of @@ -104,7 +110,11 @@ them: | no compatible committed baseline either | full analysis directly, at the configured `depth_cap` | A trusted run that computed the base publishes it, so the next pull request -forking from that commit gets the first row. +forking from that commit gets the first row. The review metadata reports the +row as `base_analysis_method` (`reused`, `incremental` or `full`) with a +`base_analysis_reason`, and the review comment repeats both under the diagram, +with measured times. While a base is computed, the progress +comment says so in two steps, with the elapsed time and the reason. The configuration hash includes `depth_cap`. The workflow input controls depth for both fresh and fallback analyses; stored legacy depth values never override it. diff --git a/scripts/action/analyze.sh b/scripts/action/analyze.sh index 5f246a0..a7048e5 100755 --- a/scripts/action/analyze.sh +++ b/scripts/action/analyze.sh @@ -139,13 +139,100 @@ analyze_sync() { "$ANALYSIS_MODE" "$ANALYSIS_PATH" "$state" >> "$GITHUB_OUTPUT" } +# How far below the merge base this run looks for the commit a saved analysis +# describes. Past it, a catch-up count is reported as unknown. +CATCHUP_BOUND=100 + +# A depth above 1 also deepens a commit the shallow checkout already holds. fetch_commit() { - local repository="$1" sha="$2" - git -C "$CHECKOUT_DIR" cat-file -e "$sha^{commit}" 2>/dev/null && return 0 + local repository="$1" sha="$2" depth="${3:-1}" + if git -C "$CHECKOUT_DIR" cat-file -e "$sha^{commit}" 2>/dev/null; then + [ "$depth" -gt 1 ] && [ "$(git -C "$CHECKOUT_DIR" rev-parse --is-shallow-repository)" = true ] || return 0 + fi local auth - auth="$(printf 'x-access-token:%s' "$GIT_TOKEN" | base64 -w0)" + auth="$(printf 'x-access-token:%s' "${GIT_TOKEN:-}" | base64 -w0)" git -C "$CHECKOUT_DIR" -c "http.extraheader=AUTHORIZATION: basic $auth" fetch \ - "${GITHUB_SERVER_URL%/}/${repository}.git" "$sha" --depth=1 + "${GITHUB_SERVER_URL%/}/${repository}.git" "$sha" --depth="$depth" +} + +# The commit a baseline committed at $1 describes, or empty when that cannot be +# told. Sync writes the baseline in a commit of its own on top of the analysed +# commit, pushed straight to the branch or merged in from its pull request's +# branch. A writer sync did not make (a squash, a rebase, a hand edit) says +# nothing about which commit was analysed. +baseline_commit() { + local writer + writer="$(baseline_writer "$1")" + if [ -n "$writer" ] && git -C "$CHECKOUT_DIR" rev-parse -q --verify "$writer^2" >/dev/null; then + writer="$(baseline_writer "$writer^2")" + fi + [ -n "$writer" ] && is_sync_commit "$writer" || return 0 + git -C "$CHECKOUT_DIR" rev-parse -q --verify "$writer^1" || true +} +# The newest first-parent commit at or below $1 that wrote the baseline. +baseline_writer() { + local writer shallow + writer="$(git -C "$CHECKOUT_DIR" log --first-parent -1 --format=%H "$1" -- .codeboarding/analysis.json 2>/dev/null || true)" + [ -n "$writer" ] || return 0 + # A shallow boundary looks like it added every file, so it proves nothing. + shallow="$(git -C "$CHECKOUT_DIR" rev-parse --git-path shallow)" + case "$shallow" in /*) ;; *) shallow="$CHECKOUT_DIR/$shallow" ;; esac + if [ -f "$shallow" ] && grep -qx "$writer" "$shallow"; then + return 0 + fi + echo "$writer" +} +is_sync_commit() { + case "$(git -C "$CHECKOUT_DIR" log -1 --format=%ce "$1")" in + 'codeboarding-review[bot]@users.noreply.github.com' | 'codeboarding[bot]@users.noreply.github.com') ;; + *) return 1 ;; + esac + [ -z "$(code_paths_changed "$1")" ] +} +# Against the first parent, so a merge counts as the change it brought in. The +# baseline and the attributes line sync may add are not code. +code_paths_changed() { + local exclude=(-- . ':(exclude).codeboarding' ':(exclude).gitattributes') + if git -C "$CHECKOUT_DIR" rev-parse -q --verify "$1^1" >/dev/null; then + git -C "$CHECKOUT_DIR" diff --name-only "$1^1" "$1" "${exclude[@]}" 2>/dev/null || true + else + git -C "$CHECKOUT_DIR" diff-tree --root --no-commit-id --name-only -r "$1" "${exclude[@]}" 2>/dev/null || true + fi +} +# First-parent commits from $1 to $2 that change code: a sync commit changes +# nothing the analysis reads, so it is nothing to catch up. +catchup_count() { + local from="$1" to="$2" commit count=0 + for commit in $(git -C "$CHECKOUT_DIR" rev-list --first-parent "$from..$to" 2>/dev/null); do + [ -z "$(code_paths_changed "$commit")" ] || count=$(( count + 1 )) + done + echo "$count" +} + +# Rewrites the sticky progress comment while the base is built from scratch. A +# fork's read-only token makes every call fail, which costs nothing. +PROGRESS_PID="" +PROGRESS_STOP="${RUNNER_TEMP:-}/codeboarding-progress-stop" +progress() { + GH_TOKEN="${GIT_TOKEN:-}" GH_ENTERPRISE_TOKEN="${GIT_TOKEN:-}" FULL_CAUSE="$full_cause" \ + PROGRESS_STOP_FILE="$PROGRESS_STOP" "$ACTION_PATH/scripts/action/post-progress.sh" "$@" >/dev/null 2>&1 || true +} +progress_start() { + local started="$1" + rm -f "$PROGRESS_STOP" + progress base 0 + ( while sleep 60 && [ ! -e "$PROGRESS_STOP" ]; do progress base "$(( $(date +%s) - started ))"; done ) >/dev/null 2>&1 & + PROGRESS_PID=$! +} +# Stops the ticker and waits for it, so no "still running" edit can land after the +# next one. Only its sleep is killed: an edit in flight finishes or, having seen +# the stop file, never starts. +progress_stop() { + [ -n "$PROGRESS_PID" ] || return 0 + touch "$PROGRESS_STOP" + pkill -x sleep -P "$PROGRESS_PID" 2>/dev/null || true + wait "$PROGRESS_PID" 2>/dev/null || true + PROGRESS_PID="" } # The artifact name pins configuration; verify the stored cap and lineage too. @@ -174,24 +261,48 @@ analyze_review() { # A published base graph is this merge base's own analysis, named for it, so it # needs no engine run at all. Without one, the merge base is checked out and - # analyzed from whatever baseline the repository committed there. - local base_source=published + # analyzed from whatever baseline the repository committed there. Each path + # records how the base analysis was obtained, for the comment and the review + # artifact: reused, incremental or full. + local base_started base_method=reused base_published=true full_cause="" base_from_sha="" catchup_commits="" + base_started="$(date +%s)" if [ "$(depth_cap_from "${BASE_DIR:-}/analysis.json")" = "$DEPTH_CAP" ]; then mkdir -p "$base_state" cp -a "$BASE_DIR/." "$base_state/" else - base_source=computed + base_published=false + # A bundle under this exact name that the run cannot use was made with another cap. + [ ! -f "${BASE_DIR:-}/analysis.json" ] || full_cause=incompatible fetch_commit "$REVIEW_BASE_REPO" "$REVIEW_BASE_SHA" git -C "$CHECKOUT_DIR" worktree add --detach "$base_checkout" "$REVIEW_BASE_SHA" >/dev/null seed_state "$base_checkout" "$base_state" REQUIRES_FULL=true if [ "$(depth_cap_from "$base_state/analysis.json")" = "$DEPTH_CAP" ]; then incremental "$base_checkout" "$base_state" + if [ "$REQUIRES_FULL" = true ]; then + full_cause=incompatible + else + fetch_commit "$REVIEW_BASE_REPO" "$REVIEW_BASE_SHA" "$(( CATCHUP_BOUND + 1 ))" || true + base_from_sha="$(baseline_commit "$REVIEW_BASE_SHA")" + [ -z "$base_from_sha" ] || catchup_commits="$(catchup_count "$base_from_sha" "$REVIEW_BASE_SHA")" + # Nothing to catch up means the committed analysis already describes this code. + [ "$catchup_commits" = 0 ] || base_method=incremental + fi + elif [ -f "$base_state/analysis.json" ]; then + full_cause=incompatible fi if [ "$REQUIRES_FULL" = true ]; then + base_method=full + full_cause="${full_cause:-no_baseline}" + trap progress_stop EXIT + progress_start "$base_started" full "$base_checkout" "$base_state" "$DEPTH_CAP" + progress_stop + progress head "$(( $(date +%s) - base_started ))" fi fi + # The lookup in the step before this one is part of obtaining the base too. + local base_seconds=$(( $(date +%s) - base_started + ${BASE_FETCH_SECONDS:-0} )) unset GIT_TOKEN local base_analysis="$base_state/analysis.json" @@ -213,10 +324,13 @@ analyze_review() { fi rm -f "$head_state/origin.json" + local head_started + head_started="$(date +%s)" incremental "$CHECKOUT_DIR" "$head_state" if [ "$REQUIRES_FULL" = true ]; then full "$CHECKOUT_DIR" "$head_state" "$DEPTH_CAP" fi + local head_seconds=$(( $(date +%s) - head_started )) write_origin "$head_state" "$seed_source" "$chain_depth" "$(analysis_digest "$base_analysis")" stage "$head_state" warmstart @@ -226,13 +340,46 @@ analyze_review() { # by id for its whole retention, so one about to expire is renewed rather than # left dangling under a review that outlives it. local publish_base=false - if [ "$base_source" = computed ] || [ "${RENEW_BASE:-false}" = true ]; then + if [ "$base_published" != true ] || [ "${RENEW_BASE:-false}" = true ]; then stage "$base_state" base publish_base=true fi printf 'analysis_mode=%s\nanalysis_path=%s\nbase_analysis_path=%s\nseed_source=%s\nchain_depth=%s\npublish_base=%s\n' \ "$ANALYSIS_MODE" "$ANALYSIS_PATH" "$base_analysis" "$seed_source" "$chain_depth" "$publish_base" >> "$GITHUB_OUTPUT" + printf 'base_analysis_method=%s\nbase_analysis_reason=%s\nbase_seconds=%s\nhead_seconds=%s\n' \ + "$base_method" "$(base_reason "$base_method" "$full_cause" "$base_from_sha" "$catchup_commits")" \ + "$base_seconds" "$head_seconds" >> "$GITHUB_OUTPUT" +} + +# Why the base analysis was obtained the way it was, in words. Whichever way, the +# result is an analysis of the merge base itself, so where an incremental run +# started is detail for this sentence, not a field of its own. +base_reason() { + local method="$1" cause="$2" from="$3" count="$4" base="${REVIEW_BASE_SHA:0:7}" reason + case "$method" in + reused) reason="$base already has a saved analysis" ;; + incremental) + if [ -z "$from" ]; then + reason="updated an existing analysis to $base" + else + reason="updated the analysis of ${from:0:7} to $base" + case "$count" in + '') ;; + 1) reason="$reason, 1 commit caught up" ;; + *) reason="$reason, $count commits caught up" ;; + esac + fi + ;; + full) + if [ "$cause" = incompatible ]; then + reason="the existing analysis was incompatible or could not be updated incrementally" + else + reason="no usable analysis was available" + fi + ;; + esac + echo "$reason" } case "$ANALYSIS_KIND" in diff --git a/scripts/action/build-review-artifact.sh b/scripts/action/build-review-artifact.sh index acdf52a..9db59e4 100755 --- a/scripts/action/build-review-artifact.sh +++ b/scripts/action/build-review-artifact.sh @@ -23,7 +23,8 @@ HEALTH_REPORT="$(dirname "$ANALYSIS_PATH")/health/health_report.json" # pr_base_sha carries the same value under the name the webview already reads: # its lookup is base_commit_sha || pr_base_sha || base_sha, so without it the # webview silently falls through to the branch tip and compares against a base -# this review never used. +# this review never used. base_analysis_method and base_analysis_reason say how +# this run obtained the base analysis; all strings, empty from an older run. jq -n \ --arg kind review \ --arg mode "$ANALYSIS_MODE" \ @@ -37,10 +38,16 @@ jq -n \ --arg base_artifact "$BASE_ARTIFACT_NAME" \ --arg base_artifact_id "$BASE_ARTIFACT_ID" \ --arg analysed_files_changed "${ANALYSED_FILES_CHANGED:-unknown}" \ + --arg base_analysis_method "${BASE_ANALYSIS_METHOD:-}" \ + --arg base_analysis_reason "${BASE_ANALYSIS_REASON:-}" \ + --arg base_seconds "${BASE_SECONDS:-}" \ + --arg head_seconds "${HEAD_SECONDS:-}" \ '{kind: $kind, mode: $mode, base_sha: $base_sha, merge_base_sha: $merge_base_sha, pr_base_sha: $merge_base_sha, merge_base_resolved: $merge_base_resolved, head_sha: $head_sha, pr_number: $pr_number, seed_source: $seed_source, chain_depth: $chain_depth, base_artifact: $base_artifact, base_artifact_id: $base_artifact_id, - analysed_files_changed: $analysed_files_changed}' \ + analysed_files_changed: $analysed_files_changed, + base_analysis_method: $base_analysis_method, base_analysis_reason: $base_analysis_reason, + base_seconds: $base_seconds, head_seconds: $head_seconds}' \ > "${RUNNER_TEMP}/cb-review-artifact/metadata.json" echo "artifact_dir=${RUNNER_TEMP}/cb-review-artifact" >> "$GITHUB_OUTPUT" diff --git a/scripts/action/build-review-comment.sh b/scripts/action/build-review-comment.sh index 96743a6..4e092a1 100755 --- a/scripts/action/build-review-comment.sh +++ b/scripts/action/build-review-comment.sh @@ -45,10 +45,33 @@ elif [ "$BEHIND" -gt 0 ] 2>/dev/null; then printf '\nCompared against the merge base: this branch is %s %s behind `%s`.\n' \ "$BEHIND" "$COMMIT_NOUN" "${BASE_REF:-the base branch}" >> "$BODY" fi +# How the base analysis was obtained, with measured times only: an estimate would be +# wrong for exactly the slow runs it is meant to explain. Supporting detail, so it +# sits under the diagram with the run links. +duration() { + local seconds="${1:-0}" + case "$seconds" in ''|*[!0-9]*) seconds=0 ;; esac + if [ "$seconds" -ge 60 ]; then + printf '%s m %s s' "$(( seconds / 60 ))" "$(( seconds % 60 ))" + else + printf '%s s' "$seconds" + fi +} +BASE_METHOD="${BASE_ANALYSIS_METHOD:-}" +BASE_LINE="" +if [ -n "$BASE_METHOD" ]; then + BASE_LINE="Base: ${BASE_METHOD}" + # A reused analysis had nothing to catch up, so its time is not worth a figure. + [ "$BASE_METHOD" = reused ] || BASE_LINE="${BASE_LINE} in $(duration "${BASE_SECONDS:-}")" + [ -z "${BASE_ANALYSIS_REASON:-}" ] || BASE_LINE="${BASE_LINE} (${BASE_ANALYSIS_REASON})" + BASE_LINE="${BASE_LINE} · changes $(duration "${HEAD_SECONDS:-}")" +fi { printf '\n' cat "$DIAGRAM" - printf '\n\n' + printf '\n' + [ -z "$BASE_LINE" ] || printf '\n%s\n' "$BASE_LINE" + printf '\n' if [ -n "$ARTIFACT_URL" ]; then printf '[download artifacts](%s) · ' "$ARTIFACT_URL" fi @@ -56,7 +79,12 @@ fi # The machine-readable line: what a reader of the comment (the web platform's dashboard, an # agent) needs without parsing the prose or the diagram. An HTML comment renders as nothing. # Keep it one line, `key=value` pairs, values without spaces, so a regex over it stays trivial. - printf '\n' \ - "$PLATFORM_URL" "$N_CHANGED" "$ANALYSED_FILES_CHANGED" "${HEAD_SHA:-}" + # The reason is prose, so it stays out of here: it is in the review artifact's metadata. + BASE_KEYS="" + if [ -n "$BASE_METHOD" ]; then + BASE_KEYS=" base_analysis_method=${BASE_METHOD} base_seconds=${BASE_SECONDS:-} head_seconds=${HEAD_SECONDS:-}" + fi + printf '\n' \ + "$PLATFORM_URL" "$N_CHANGED" "$ANALYSED_FILES_CHANGED" "${HEAD_SHA:-}" "$BASE_KEYS" } >> "$BODY" echo "path=$BODY" >> "$GITHUB_OUTPUT" diff --git a/scripts/action/fetch-state.sh b/scripts/action/fetch-state.sh index cf16a1e..e90dacd 100755 --- a/scripts/action/fetch-state.sh +++ b/scripts/action/fetch-state.sh @@ -4,6 +4,10 @@ # leave the directory absent, and the caller derives from the base instead. set -euo pipefail [ -n "${ARTIFACT_NAME:-}" ] || exit 0 +# A review reports how long obtaining its base took, and this lookup is part of it. +started="$(date +%s)" +report_seconds() { [ -z "${GITHUB_OUTPUT:-}" ] || echo "seconds=$(( $(date +%s) - started ))" >> "$GITHUB_OUTPUT"; } +trap report_seconds EXIT # Clear first, on every path. These destinations are fixed, so a second use of # the action in one job would otherwise inherit the first one's files and treat diff --git a/scripts/action/post-progress.sh b/scripts/action/post-progress.sh new file mode 100755 index 0000000..7ccb210 --- /dev/null +++ b/scripts/action/post-progress.sh @@ -0,0 +1,59 @@ +#!/usr/bin/env bash +# Rewrites the review's sticky progress comment into two steps while the base is +# built from scratch: that is the slow path, and the reader should know why. +# Usage: post-progress.sh base|head . Best effort throughout. +set -euo pipefail +step="$1" elapsed="$2" +[ -n "${PROGRESS_HEADER:-}" ] && [ -n "${PR_NUMBER:-}" ] && [ -n "${REPOSITORY:-}" ] || exit 0 +export GH_HOST="${GH_HOST:-github.com}" +GH_HOST="${GH_HOST#*://}" + +# The sticky-comment action finds its comment by this line, so it must survive the edit. +marker="" +id_file="${RUNNER_TEMP:?}/codeboarding-progress-comment-${PROGRESS_HEADER}" +id="$(cat "$id_file" 2>/dev/null || true)" +if [ -z "$id" ]; then + id="$(gh api --paginate "repos/$REPOSITORY/issues/$PR_NUMBER/comments?per_page=100" \ + --jq ".[] | select((.body // \"\") | startswith(\"### CodeBoarding review\") and contains(\"$marker\")) | .id" | + tail -n 1)" + [ -n "$id" ] || exit 0 + echo "$id" > "$id_file" +fi + +if [ -n "${BASE_REF:-}" ]; then + branch="\`$BASE_REF\`" +else + branch="the base branch" +fi +sha7="${REVIEW_BASE_SHA:0:7}" +case "${FULL_CAUSE:-}" in + incompatible) why="The saved diagram of $branch was made by a different engine version or settings, so this review builds a new one." ;; + *) why="$branch has no saved diagram yet, so this review builds one first. Once a diagram of $branch is saved, reviews start from it and skip this step." ;; +esac +minutes=$(( elapsed / 60 )) +if [ "$step" = base ]; then + running="running for $minutes min" + [ "$minutes" -gt 0 ] || running="running for less than a minute" + first="1. ⏳ Building the diagram of $branch @$sha7 from scratch · $running" + second="2. Analysing this PR's changes" +else + took="$(( elapsed % 60 )) s" + [ "$minutes" -eq 0 ] || took="$minutes m $took" + first="1. ✅ Built the diagram of $branch @$sha7 from scratch in $took" + second="2. ⏳ Analysing this PR's changes" +fi + +platform="https://app.codeboarding.org/$REPOSITORY/pull/$PR_NUMBER?utm_source=github&utm_medium=pr_comment&utm_campaign=gh_action" +run_url="${GITHUB_SERVER_URL:-https://github.com}/$REPOSITORY/actions/runs/${GITHUB_RUN_ID:-}" +body="$(printf '%s\n\n%s\n %s\n%s\n\n%s\n\n%s\n%s' \ + '### CodeBoarding review · analyzing…' \ + "$first" "$why" "$second" \ + "Open it in [CodeBoarding]($platform) meanwhile: the files, comments and review are there already, and the diff appears when the run finishes." \ + "run [${GITHUB_RUN_ID:-}]($run_url) · attempt ${GITHUB_RUN_ATTEMPT:-1}" \ + "$marker")" +# Checked last: the ticker may have been stopped while this ran, and a stale +# "running" edit must not land on top of the next step. +if [ "$step" = base ] && [ -n "${PROGRESS_STOP_FILE:-}" ] && [ -e "$PROGRESS_STOP_FILE" ]; then + exit 0 +fi +gh api -X PATCH "repos/$REPOSITORY/issues/comments/$id" -f body="$body" >/dev/null diff --git a/tests/test_action_state.py b/tests/test_action_state.py index 7b5a8aa..de351fd 100644 --- a/tests/test_action_state.py +++ b/tests/test_action_state.py @@ -312,6 +312,241 @@ def test_base_and_head_fallbacks_keep_configured_depth(self) -> None: self.assertEqual([c["mode"] for c in calls], ["incremental", "full", "incremental", "full"]) self.assertEqual([c["depth"] for c in calls if c["mode"] == "full"], ["4", "4"]) + # How the base was obtained is reported, not just used: the review comment and + # the webview explain a slow run by it. + + def _git(self, *args: str) -> str: + return subprocess.run( + [ + "git", + "-C", + str(self.checkout), + "-c", + "user.name=Test", + "-c", + "user.email=test@example.com", + "-c", + "commit.gpgsign=false", + *args, + ], + check=True, + capture_output=True, + text=True, + ).stdout.strip() + + def _commit(self, message: str, files: dict[str, str], *, bot: bool = False) -> str: + for name, content in files.items(): + (self.checkout / name).parent.mkdir(parents=True, exist_ok=True) + (self.checkout / name).write_text(content, encoding="utf-8") + self._git("add", "-A") + committer = ("-c", "user.email=codeboarding-review[bot]@users.noreply.github.com") if bot else () + self._git(*committer, "commit", "-q", "-m", message) + return self._git("rev-parse", "HEAD") + + def _sync_history(self) -> tuple[str, str]: + """An analysed commit with a sync commit on top that writes only .codeboarding/.""" + self._git("init", "-q", "-b", "main") + analysed = self._commit("feat: code", {"code.py": "pass\n"}) + _state(self.checkout / ".codeboarding", cap=2) + sync = self._commit("chore(codeboarding): sync analysis baseline", {}, bot=True) + return analysed, sync + + def _merge(self, branch: str, files: dict[str, str], *, bot: bool = False) -> str: + """A commit on `branch` merged into main with --no-ff.""" + self._git("checkout", "-q", "-b", branch) + self._commit(f"work on {branch}", files, bot=bot) + self._git("checkout", "-q", "main") + self._git("merge", "-q", "--no-ff", "-m", f"Merge {branch}", branch) + return self._git("rev-parse", "HEAD") + + def _provenance(self, values: dict[str, str]) -> dict[str, str]: + keys = ("base_analysis_method", "base_analysis_reason") + for key in ("base_seconds", "head_seconds"): + self.assertRegex(values[key], r"^[0-9]+$", key) + return {key: values[key] for key in keys} + + def test_a_saved_base_is_reused(self) -> None: + _state(self.base_dir) + + values = self._analyze(BASE_FETCH_SECONDS="7") + + self.assertEqual( + self._provenance(values), + {"base_analysis_method": "reused", "base_analysis_reason": "merge-b already has a saved analysis"}, + ) + # The download happened in the step before; it is still time spent on the base. + self.assertGreaterEqual(int(values["base_seconds"]), 7) + + def test_a_base_with_nothing_to_seed_it_is_a_full_analysis(self) -> None: + sha = self._commit_base() + + values = self._analyze(REVIEW_BASE_SHA=sha) + + self.assertEqual( + self._provenance(values), + {"base_analysis_method": "full", "base_analysis_reason": "no usable analysis was available"}, + ) + + def _assert_incompatible(self, values: dict[str, str]) -> None: + self.assertEqual( + self._provenance(values), + { + "base_analysis_method": "full", + "base_analysis_reason": "the existing analysis was incompatible or could not be updated incrementally", + }, + ) + + def test_a_committed_baseline_with_another_depth_is_incompatible(self) -> None: + sha = self._commit_base(legacy=True) + + self._assert_incompatible(self._analyze(REVIEW_BASE_SHA=sha)) + + def test_a_saved_base_with_another_depth_is_incompatible(self) -> None: + sha = self._commit_base() + _state(self.base_dir, cap=1) + + self._assert_incompatible(self._analyze(REVIEW_BASE_SHA=sha)) + + def test_an_engine_that_demands_a_full_run_makes_the_baseline_incompatible(self) -> None: + sha = self._commit_base(cap=2) + + self._assert_incompatible(self._analyze(REVIEW_BASE_SHA=sha, CB_REQUIRE_FULL="true")) + + def test_a_baseline_committed_at_the_merge_base_is_reused(self) -> None: + # The sync commit changes only .codeboarding/, which the analysis ignores, + # so a merge base on it is exactly the commit the baseline describes. + _analysed, sync = self._sync_history() + + values = self._analyze(REVIEW_BASE_SHA=sync) + + self.assertEqual( + self._provenance(values), + {"base_analysis_method": "reused", "base_analysis_reason": f"{sync[:7]} already has a saved analysis"}, + ) + # Not under the merge base's name yet, so this run publishes it. + self.assertEqual(values["publish_base"], "true") + + def test_a_committed_baseline_counts_the_commits_it_caught_up(self) -> None: + analysed, _sync = self._sync_history() + self._commit("feat: more", {"more.py": "pass\n"}) + merge_base = self._commit("feat: again", {"code.py": "print()\n"}) + + values = self._analyze(REVIEW_BASE_SHA=merge_base) + + self.assertEqual( + self._provenance(values), + { + "base_analysis_method": "incremental", + "base_analysis_reason": f"updated the analysis of {analysed[:7]} to {merge_base[:7]}, 2 commits caught up", + }, + ) + + def test_merged_pull_requests_count_as_commits_to_catch_up(self) -> None: + # Merged with --no-ff, the first-parent chain is merge commits only, and + # each one brings code in even though it changes nothing against itself. + analysed, _sync = self._sync_history() + self._merge("feature-a", {"a.py": "pass\n"}) + merge_base = self._merge("feature-b", {"b.py": "pass\n"}) + + values = self._analyze(REVIEW_BASE_SHA=merge_base) + + self.assertEqual( + values["base_analysis_reason"], + f"updated the analysis of {analysed[:7]} to {merge_base[:7]}, 2 commits caught up", + ) + + def test_a_merged_sync_pull_request_describes_its_own_parent(self) -> None: + # sync_strategy: pull_request. The sync commit sits on codeboarding/sync on + # top of the analysed commit; main moved on before the merge. + self._git("init", "-q", "-b", "main") + analysed = self._commit("feat: code", {"code.py": "pass\n"}) + self._git("checkout", "-q", "-b", "codeboarding/sync") + _state(self.checkout / ".codeboarding", cap=2) + self._commit("chore(codeboarding): sync analysis baseline", {}, bot=True) + self._git("checkout", "-q", "main") + self._commit("feat: meanwhile", {"later.py": "pass\n"}) + self._git("merge", "-q", "--no-ff", "-m", "Merge codeboarding/sync", "codeboarding/sync") + merge_base = self._git("rev-parse", "HEAD") + + values = self._analyze(REVIEW_BASE_SHA=merge_base) + + self.assertEqual( + self._provenance(values), + { + "base_analysis_method": "incremental", + "base_analysis_reason": f"updated the analysis of {analysed[:7]} to {merge_base[:7]}, 1 commit caught up", + }, + ) + + def test_a_baseline_sync_did_not_write_is_of_unknown_origin(self) -> None: + # A squash or a hand edit: its parent is not known to be what was analysed, + # so the reason names no starting commit and claims nothing was exact. + self._git("init", "-q", "-b", "main") + self._commit("feat: code", {"code.py": "pass\n"}) + _state(self.checkout / ".codeboarding", cap=2) + merge_base = self._commit("chore(codeboarding): sync analysis baseline (#7)", {}) + + values = self._analyze(REVIEW_BASE_SHA=merge_base) + + self.assertEqual( + self._provenance(values), + { + "base_analysis_method": "incremental", + "base_analysis_reason": f"updated an existing analysis to {merge_base[:7]}", + }, + ) + + def test_an_attributes_line_is_not_code_to_catch_up(self) -> None: + _analysed, _sync = self._sync_history() + merge_base = self._commit( + "chore: attributes", {".gitattributes": ".codeboarding/analysis.json linguist-generated=true\n"} + ) + + values = self._analyze(REVIEW_BASE_SHA=merge_base) + + self.assertEqual(values["base_analysis_method"], "reused") + + def _progress_stub(self) -> Path: + calls = self.root / "gh-calls" + gh = self.bin_dir / "gh" + gh.write_text( + "#!/usr/bin/env bash\n" + f'printf "%s\\n----\\n" "$*" >> "{calls}"\n' + 'case "$*" in *"/comments?per_page"*) echo 77 ;; esac\n', + encoding="utf-8", + ) + gh.chmod(0o755) + return calls + + def test_building_the_base_from_scratch_rewrites_the_progress_comment(self) -> None: + calls = self._progress_stub() + sha = self._commit_base() + + self._analyze( + REVIEW_BASE_SHA=sha, + PROGRESS_HEADER="codeboarding-review", + REPOSITORY="owner/repo", + BASE_REF="develop", + GIT_TOKEN="token", + ) + + patches = [call for call in calls.read_text().split("\n----\n") if "PATCH" in call] + self.assertGreaterEqual(len(patches), 2, calls.read_text()) + self.assertIn("repos/owner/repo/issues/comments/77", patches[0]) + self.assertIn(f"Building the diagram of `develop` @{sha[:7]} from scratch", patches[0]) + self.assertIn("`develop` has no saved diagram yet", patches[0]) + self.assertIn("2. ⏳ Analysing this PR's changes", patches[-1]) + # The sticky-comment action finds its comment by this line on the final write. + self.assertIn("", patches[-1]) + + def test_a_saved_base_leaves_the_progress_comment_alone(self) -> None: + calls = self._progress_stub() + _state(self.base_dir) + + self._analyze(PROGRESS_HEADER="codeboarding-review", REPOSITORY="owner/repo", GIT_TOKEN="token") + + self.assertFalse(calls.exists()) + def test_invalid_depth_fails_before_analysis(self) -> None: result = subprocess.run( [str(ANALYZE)], diff --git a/tests/test_progress_comment.py b/tests/test_progress_comment.py new file mode 100644 index 0000000..8994422 --- /dev/null +++ b/tests/test_progress_comment.py @@ -0,0 +1,91 @@ +"""The progress comment while a base is built from scratch: two steps, elapsed time only.""" + +import os +import subprocess +import tempfile +import unittest +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] +POST_PROGRESS = ROOT / "scripts" / "action" / "post-progress.sh" + + +class ProgressCommentTests(unittest.TestCase): + def setUp(self) -> None: + self.temp_dir = tempfile.TemporaryDirectory() + self.root = Path(self.temp_dir.name) + self.bin_dir = self.root / "bin" + self.bin_dir.mkdir() + self.calls = self.root / "calls" + + def tearDown(self) -> None: + self.temp_dir.cleanup() + + def _post(self, step: str, elapsed: int, comment_id: str = "77", **extra: str) -> list[str]: + gh = self.bin_dir / "gh" + gh.write_text( + "#!/usr/bin/env bash\n" + f'printf "%s\\n----\\n" "$*" >> "{self.calls}"\n' + f'case "$*" in *"/comments?per_page"*) echo "{comment_id}" ;; esac\n', + encoding="utf-8", + ) + gh.chmod(0o755) + subprocess.run( + [str(POST_PROGRESS), step, str(elapsed)], + env={ + "PATH": f"{self.bin_dir}:{os.environ['PATH']}", + "RUNNER_TEMP": str(self.root), + "PROGRESS_HEADER": "codeboarding-review", + "PR_NUMBER": "9", + "REPOSITORY": "owner/repo", + "BASE_REF": "main", + "REVIEW_BASE_SHA": "a1b2c3d4e5f6", + "GITHUB_RUN_ID": "55", + **extra, + }, + check=True, + capture_output=True, + text=True, + ) + return [c for c in self.calls.read_text().split("\n----\n") if "PATCH" in c] if self.calls.exists() else [] + + def test_the_base_step_shows_elapsed_minutes_and_the_reason(self) -> None: + (patch,) = self._post("base", 125, FULL_CAUSE="no_baseline") + self.assertIn("1. ⏳ Building the diagram of `main` @a1b2c3d from scratch · running for 2 min\n", patch) + self.assertIn(" `main` has no saved diagram yet, so this review builds one first.", patch) + self.assertIn("\n2. Analysing this PR's changes\n", patch) + self.assertNotIn("estimate", patch.lower()) + + def test_the_first_minute_does_not_read_as_zero(self) -> None: + (patch,) = self._post("base", 0) + self.assertIn("running for less than a minute", patch) + + def test_an_incompatible_base_says_why(self) -> None: + (patch,) = self._post("base", 60, FULL_CAUSE="incompatible") + self.assertIn("The saved diagram of `main` was made by a different engine version or settings", patch) + + def test_the_head_step_reports_the_measured_base_time(self) -> None: + (patch,) = self._post("head", 534) + self.assertIn("1. ✅ Built the diagram of `main` @a1b2c3d from scratch in 8 m 54 s\n", patch) + self.assertIn("2. ⏳ Analysing this PR's changes", patch) + self.assertTrue(patch.rstrip().endswith("")) + + def test_the_comment_is_looked_up_once(self) -> None: + self._post("base", 0) + self._post("base", 60) + lookups = [c for c in self.calls.read_text().split("\n----\n") if "/comments?per_page" in c] + self.assertEqual(len(lookups), 1) + + def test_a_stopped_ticker_never_edits_after_the_next_step(self) -> None: + stop = self.root / "stop" + stop.touch() + self.assertEqual(self._post("base", 120, PROGRESS_STOP_FILE=str(stop)), []) + # The step-2 edit is the one that follows the stop, so it still lands. + self.assertEqual(len(self._post("head", 120, PROGRESS_STOP_FILE=str(stop))), 1) + + def test_no_progress_comment_means_no_edit(self) -> None: + self.assertEqual(self._post("base", 0, comment_id=""), []) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_review_artifact_metadata.py b/tests/test_review_artifact_metadata.py index 6b43526..7a79060 100644 --- a/tests/test_review_artifact_metadata.py +++ b/tests/test_review_artifact_metadata.py @@ -65,6 +65,34 @@ def test_a_count_the_render_step_could_not_make_is_recorded_as_unknown(self) -> metadata, _outputs = _build(Path(tmp)) self.assertEqual(metadata["analysed_files_changed"], "unknown") + def test_how_the_base_was_obtained_is_recorded_as_strings(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + metadata, _outputs = _build( + Path(tmp), + BASE_ANALYSIS_METHOD="incremental", + BASE_ANALYSIS_REASON="updated the analysis of 9f8e7d6 to a1b2c3d, 3 commits caught up", + BASE_SECONDS="41", + HEAD_SECONDS="159", + ) + self.assertEqual( + {key: metadata[key] for key in metadata if key.startswith("base_") or key.endswith("_seconds")}, + { + "base_sha": "tip-sha", + "base_artifact": "codeboarding-base-cfg-mergebasesha", + "base_artifact_id": "4242", + "base_analysis_method": "incremental", + "base_analysis_reason": "updated the analysis of 9f8e7d6 to a1b2c3d, 3 commits caught up", + "base_seconds": "41", + "head_seconds": "159", + }, + ) + + def test_an_older_run_without_provenance_records_empty_strings(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + metadata, _outputs = _build(Path(tmp)) + for key in ("base_analysis_method", "base_analysis_reason", "base_seconds", "head_seconds"): + self.assertEqual(metadata[key], "", key) + if __name__ == "__main__": unittest.main() diff --git a/tests/test_review_comment.py b/tests/test_review_comment.py index 709f693..ca77256 100644 --- a/tests/test_review_comment.py +++ b/tests/test_review_comment.py @@ -82,5 +82,78 @@ def test_a_changed_analysed_file_gets_no_verdict_even_at_zero_components(self) - self.assertIn("changed=0 analysed_files_changed=2 head=abc123", body) +class BaseLineTests(unittest.TestCase): + """One line under the diagram saying how the base analysis was obtained, with measured times only.""" + + def _body(self, **extra: str) -> str: + with tempfile.TemporaryDirectory() as tmp: + return _build(Path(tmp), BASE_REF="main", MERGE_BASE_SHA="f00dfeed" * 5, **extra) + + def _base_line(self, **extra: str) -> str: + body = self._body(**extra) + lines = [line for line in body.splitlines() if line.startswith("Base: ")] + self.assertEqual(len(lines), 1, body) + return lines[0] + + def test_a_full_analysis_says_why_and_how_long(self) -> None: + line = self._base_line( + BASE_ANALYSIS_METHOD="full", + BASE_ANALYSIS_REASON="no usable analysis was available", + BASE_SECONDS="534", + HEAD_SECONDS="192", + ) + self.assertEqual( + line, "Base: full in 8 m 54 s (no usable analysis was available) · changes 3 m 12 s" + ) + + def test_a_reused_analysis_has_no_base_time(self) -> None: + line = self._base_line( + BASE_ANALYSIS_METHOD="reused", + BASE_ANALYSIS_REASON="a1b2c3d already has a saved analysis", + BASE_SECONDS="3", + HEAD_SECONDS="159", + ) + self.assertEqual(line, "Base: reused (a1b2c3d already has a saved analysis) · changes 2 m 39 s") + + def test_an_incremental_analysis_carries_where_it_started(self) -> None: + line = self._base_line( + BASE_ANALYSIS_METHOD="incremental", + BASE_ANALYSIS_REASON="updated the analysis of 9f8e7d6 to a1b2c3d, 4 commits caught up", + BASE_SECONDS="41", + HEAD_SECONDS="159", + ) + self.assertEqual( + line, + "Base: incremental in 41 s (updated the analysis of 9f8e7d6 to a1b2c3d, 4 commits caught up)" + " · changes 2 m 39 s", + ) + + def test_the_base_line_sits_under_the_diagram_above_the_run_links(self) -> None: + body = self._body(BASE_ANALYSIS_METHOD="reused", BASE_ANALYSIS_REASON="r", HEAD_SECONDS="4") + diagram = body.index("```mermaid") + base = body.index("Base: ") + footer = body.index("run [1234]") + self.assertLess(diagram, base) + self.assertLess(base, footer) + self.assertIn("· changes 4 s\n\nrun [1234]", body) + + def test_the_marker_carries_the_method_after_the_existing_keys(self) -> None: + body = self._body( + BASE_ANALYSIS_METHOD="full", + BASE_ANALYSIS_REASON="no usable analysis was available", + BASE_SECONDS="534", + HEAD_SECONDS="192", + ) + self.assertTrue( + body.rstrip("\n").endswith("head=abc123 base_analysis_method=full base_seconds=534 head_seconds=192 -->"), + body, + ) + + def test_without_a_method_there_is_no_line_and_no_keys(self) -> None: + body = self._body() + self.assertNotIn("Base:", body) + self.assertNotIn("base_analysis_method=", body) + + if __name__ == "__main__": unittest.main()