From d588f7c2165a533fcca4dc9c9f396619116df87e Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 24 Aug 2026 03:05:52 +0000 Subject: [PATCH] feat: upload the smoke report dir so per-script timings persist (#264) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PyAutoHands#264 has the runner write a consolidated `smoke_timings.json` into its report dir on every report write — one row per executed script/notebook with the runner's own measured duration, the cap in force and the exit code. That file dies with the job unless something uploads it, and the job log it would otherwise be scraped from ages out with log retention. Uploading it here, in the reusable workflow, means the dataset accumulates for every gate run across all ten workspaces at once, with no per-repo change. The path is a glob rather than one literal directory because this workflow does not pass `--report-dir`: each workspace's own `.github/scripts/run_smoke.py` does, and the runner script is deliberately left in the workspace (see the file header). `test-results/` is the house name — what workspace-validation.yml passes to its own `run_python.py` invocations — and the second pattern catches a workspace that named its report dir something else. `if: always()` plus `if-no-files-found: ignore` are both load-bearing. A docs-only change skips the matrix entirely and a crash before the first report write leaves no report dir at all; neither is a reason to fail a gate. An upload that failed closed would turn "we collected no timings" into "the PR is red", which is backwards. No `retention-days`, so the dataset keeps the repo's full default retention rather than a shortened window. The step sits ahead of the Slack notifier so a failing run's timings — the most interesting ones — are collected rather than skipped. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01EoDPz2LevKeBaDwqFKtZrU --- .github/workflows/smoke-tests.yml | 35 +++++++++++++++++++++ tests/test_workflow_wiring.py | 52 +++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+) diff --git a/.github/workflows/smoke-tests.yml b/.github/workflows/smoke-tests.yml index 2946b6a..d7b37e4 100644 --- a/.github/workflows/smoke-tests.yml +++ b/.github/workflows/smoke-tests.yml @@ -201,6 +201,41 @@ jobs: fi python "$RUNNER" $RUNNER_ARGS + - name: Upload the smoke report dir (per-script timings) + # PyAutoHands#264. The runner writes a consolidated smoke_timings.json + # into its report dir on every report write — one row per executed + # script/notebook with the runner's OWN measured duration, the cap in + # force, and the exit code. In the job log that data evaporates with + # the ~90-day log retention and is only recovered by hand-scraping; + # as an artifact it becomes a standing dataset the Heart board can + # ingest, for every gate run across all ten workspaces at once. + # + # WHY A GLOB and not one literal path: this workflow does not pass + # `--report-dir` — each workspace's own `.github/scripts/run_smoke.py` + # does, and the runner script is deliberately left in the workspace + # (see the header). `test-results/` is the house name, used by + # workspace-validation.yml's own run_python.py invocations; the second + # pattern catches a workspace that named its report dir something else, + # so no repo needs editing for its timings to land here. + # + # No retention-days: the dataset is the point, so it keeps the repo's + # full default artifact retention rather than a shortened window. + # + # if: always() + if-no-files-found: ignore are both load-bearing. A + # docs-only change skips the matrix entirely; a crash before the first + # report write (a failed install, a missing script list) leaves no + # report dir at all. Neither is a reason to fail a gate — and an upload + # that failed closed would turn "we collected no timings" into "the PR + # is red", which is exactly backwards. + if: always() + uses: actions/upload-artifact@v4 + with: + name: smoke-timings-${{ matrix.python-version }} + path: | + workspace/test-results/ + workspace/**/smoke_timings.json + if-no-files-found: ignore + - name: Slack notify on failure if: ${{ failure() }} uses: slackapi/slack-github-action@v1.21.0 diff --git a/tests/test_workflow_wiring.py b/tests/test_workflow_wiring.py index 180ec67..0e37a2d 100644 --- a/tests/test_workflow_wiring.py +++ b/tests/test_workflow_wiring.py @@ -71,3 +71,55 @@ def test_smoke_reusable_docs_only_gate_is_wired_fail_closed(): smoke = jobs["smoke"] assert smoke["needs"] == "changes" assert smoke["if"] == "needs.changes.outputs.docs_only != 'true'" + + +def _step(job, name_fragment): + for step in job["steps"]: + if name_fragment in step.get("name", ""): + return step + raise AssertionError(f"no step named like {name_fragment!r}") + + +def test_smoke_reusable_uploads_the_timing_dataset(): + """The per-script timing dataset leaves the job as an artifact. + + PyAutoHands#264: the runner writes smoke_timings.json into its report dir, + but a report dir dies with the runner unless something uploads it. The + artifact name carries the matrix python version so the two legs do not + collide. + """ + smoke = _load("smoke-tests.yml")["jobs"]["smoke"] + step = _step(smoke, "Upload the smoke report dir") + + assert step["uses"].startswith("actions/upload-artifact@v4") + assert step["with"]["name"] == "smoke-timings-${{ matrix.python-version }}" + assert "test-results/" in step["with"]["path"] + assert "smoke_timings.json" in step["with"]["path"] + + +def test_smoke_timings_upload_cannot_fail_the_gate(): + """A run with no timings is not a red PR. + + Both guards matter: `always()` so a failing script still yields its + timings, and `if-no-files-found: ignore` so a pre-report crash (or a + workspace whose runner wrote nothing) does not turn a missing dataset into + a failed job. + """ + smoke = _load("smoke-tests.yml")["jobs"]["smoke"] + step = _step(smoke, "Upload the smoke report dir") + + assert step["if"] == "always()" + assert step["with"]["if-no-files-found"] == "ignore" + + +def test_smoke_timings_upload_runs_before_the_slack_notifier(): + """Ordering is the reason the artifact survives a failing run. + + The Slack step is the job's terminal `failure()` hook; the upload has to + sit ahead of it so the timings for the run that just failed are collected + rather than skipped. + """ + names = [s.get("name", "") for s in _load("smoke-tests.yml")["jobs"]["smoke"]["steps"]] + upload = next(i for i, n in enumerate(names) if "Upload the smoke report dir" in n) + slack = next(i for i, n in enumerate(names) if "Slack notify" in n) + assert upload < slack