From bac49866837e0e915da381ad377ddd41cc210019 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Wed, 11 Nov 2026 14:17:50 +0000 Subject: [PATCH 1/2] pr-review skill: say how to verify that a test proves its claim Pass 9 told the reviewer what a good test looks like but not how to check it. Add the techniques that found the test findings in recent reviews: run the tests (also under another Go toolchain), revert the fix for regression tests, restore scripts from BASE for variant-parity changes, read the PR's Integration Tests run before asking about a real workspace, and check Repls for host-dependent URL tokens. Co-authored-by: Isaac --- .agents/skills/pr-review/SKILL.md | 34 ++++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/.agents/skills/pr-review/SKILL.md b/.agents/skills/pr-review/SKILL.md index 397ea485a14..d60ea8f4ee1 100644 --- a/.agents/skills/pr-review/SKILL.md +++ b/.agents/skills/pr-review/SKILL.md @@ -135,24 +135,48 @@ on code the change adds or modifies, not on pre-existing issues next to it. functions and helpers with unit tests next to the existing ones. Flag a unit test that should be an acceptance test, and logic with many input combinations that has no unit tests (name a case they would catch). + - Run the changed unit and acceptance tests locally. If another Go + toolchain is installed, run the acceptance tests under it as well + (`GOTOOLCHAIN=goX.Y.Z`): a golden whose values change across toolchains + asserts the standard library, not the CLI. - Does each new or changed test show the effect it claims? The output should include the state after the action (list or print it again after a sync, a deploy or a migration) and the exact value the change is - about, not just a successful exit. + about, not just a successful exit. For a test that adds coverage for an + earlier fix, revert the fix locally and run the test; a variant that + still passes does not assert the claim. For a test that asserts an + invariant over existing types or files, run a scratch positive control + to see the check fire, and compare the check with the walker or helper + the repo already uses for that traversal. + - When a script or helper changes only to keep goldens equal across + `EnvMatrix` variants (a `nostamp` pipe, a new scrub pattern), restore it + from `BASE` and run the variant it targets; a change that passes without + it is dead at this commit. For a change to a shared helper under + `acceptance/bin`, check that some golden shows each new option doing + something. - The fake server in `libs/testserver` only returns what its handlers were taught, so a test that records the request, or a handler that echoes a field back, shows what the CLI sends, not what the API accepts. When the change sends a new field, a new value format (paths, identifiers, durations) or a new call to a real API, and no test covering it has - `Cloud = true` (`Cloud` is inherited, so check `out.test.toml`) and the - description does not mention a run against a real workspace, ask whether - it was verified against one. + `Cloud = true` (`Cloud` is inherited, so check `out.test.toml`), look at + `gh pr checks` first: the PR cloud run selects the tests whose + directories changed (`DATABRICKS_TEST_SELECT_CHANGED`), so a passing + Integration Tests run answers the question for those. It does not cover + a test in another directory that picks the change up through a shared + template or helper; for that one, or when there is no cloud run and the + description does not mention a real workspace, ask whether it was + verified against one. + - In a `Cloud = true` test, scan the golden output for host-dependent + tokens (`?w=[WORKSPACE_ID]`, `?o=`) and check that a `Repls` entry + strips them; Azure and GCP hosts omit the workspace query parameter. - Are fixtures readable? Prefer generating archives and other binary inputs in the test script over committing opaque files. - Does the test reuse the repo's existing test helpers and scripts, and keep helpers that a planned follow-up will need again? - One long script that walks many failure modes is hard to review: - suggest splitting it by category. + suggest splitting it by category, unless its sections share setup that + is expensive to repeat in a cloud run (a deploy, a break-glass call). 10. **User-facing text.** Read every new or changed changelog fragment (`.nextchanges/`), error message, warning and help text. It should be short, accurate and written for users, follow `.nextchanges/README.md` From 6530d299bea09ef24b4d34dfa0710c072d55655c Mon Sep 17 00:00:00 2001 From: "andrew.nester" Date: Wed, 11 Nov 2026 15:02:15 +0000 Subject: [PATCH 2/2] pr-review skill: read CI and the regression report before running tests Two review threads on pass 9 asked why the reviewer should run tests locally when CI runs them, and to lean on the regression report instead of reverting the fix by hand. The toolchain bullet now says what a green `gh pr checks` already proves: the `task test (linux, ...)` jobs run the whole suite and the windows and macos cells always keep the tests the PR changed, all under the one toolchain pinned in go.mod. The local run stays only for another Go version, which no job runs; a golden that encodes standard-library output passes CI and fails on the next `Bump Go toolchain` PR, as acceptance/cmd/api/request-handling/output.txt did in #7052. The "revert the fix" sentence now reads the Regression test report comment first: `regression=` and `unreleased=` count the changed acceptance tests that fail on main or on the latest release, so a test under `coverage=` passes without the fix unless the fix is already released. Reverting locally is kept for what the report cannot answer: unit tests, tests beyond its selection limit, and released fixes. Co-authored-by: Isaac --- .agents/skills/pr-review/SKILL.md | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/.agents/skills/pr-review/SKILL.md b/.agents/skills/pr-review/SKILL.md index d60ea8f4ee1..f1db42665ea 100644 --- a/.agents/skills/pr-review/SKILL.md +++ b/.agents/skills/pr-review/SKILL.md @@ -135,19 +135,28 @@ on code the change adds or modifies, not on pre-existing issues next to it. functions and helpers with unit tests next to the existing ones. Flag a unit test that should be an acceptance test, and logic with many input combinations that has no unit tests (name a case they would catch). - - Run the changed unit and acceptance tests locally. If another Go - toolchain is installed, run the acceptance tests under it as well - (`GOTOOLCHAIN=goX.Y.Z`): a golden whose values change across toolchains - asserts the standard library, not the CLI. + - Look at `gh pr checks` before running tests locally: the + `task test (linux, ...)` jobs run the whole suite and the windows and + macos cells always include the tests the PR changed, so a green build + shows them passing under the toolchain pinned in `go.mod`. No job runs + another Go version, so run the changed acceptance tests under one + (`GOTOOLCHAIN=goX.Y.Z` downloads it): a golden whose values change + across toolchains asserts the standard library, not the CLI, and would + fail only on the next `Bump Go toolchain` PR. - Does each new or changed test show the effect it claims? The output should include the state after the action (list or print it again after a sync, a deploy or a migration) and the exact value the change is - about, not just a successful exit. For a test that adds coverage for an - earlier fix, revert the fix locally and run the test; a variant that - still passes does not assert the claim. For a test that asserts an - invariant over existing types or files, run a scratch positive control - to see the check fire, and compare the check with the walker or helper - the repo already uses for that traversal. + about, not just a successful exit. For a test that pins a fix, read the + Regression test report comment on the PR first (`gh pr view --comments`): + it ran the changed acceptance tests against main and the latest release, + so a test under `regression=` or `unreleased=` fails without the fix, + and one under `coverage=` passes without it unless the fix is already + released. Revert the fix locally and run the test only when the report + cannot tell: a unit test, a test it did not select, or a released fix; a + variant that still passes does not assert the claim. For a test that + asserts an invariant over existing types or files, run a scratch + positive control to see the check fire, and compare the check with the + walker or helper the repo already uses for that traversal. - When a script or helper changes only to keep goldens equal across `EnvMatrix` variants (a `nostamp` pipe, a new scrub pattern), restore it from `BASE` and run the variant it targets; a change that passes without