diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3459774c..fbc853b1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -55,7 +55,7 @@ jobs: # in qa) run in build-and-test below, which now also covers docs-only PRs. run: | ./mvnw -B -ntp clean \ - "-Dtest=EnginePdfBoundaryTest,DocumentationCoverageTest,CanonicalSurfaceGuardTest,PackageMapGuardTest,VersionConsistencyGuardTest,CiGuardListGuardTest,CodeQlScopeGuardTest" \ + "-Dtest=EnginePdfBoundaryTest,DocumentationCoverageTest,CanonicalSurfaceGuardTest,PackageMapGuardTest,VersionConsistencyGuardTest,CiGuardListGuardTest,CiGateCoverageGuardTest,CodeQlScopeGuardTest" \ test -pl :graph-compose-core changes: @@ -435,8 +435,14 @@ jobs: # "Architecture and Documentation Guards") instead of the individual, sometimes # -skipped matrix legs so a docs-only PR is never left waiting on a check that # never reports. + # + # `changes` is in the list even though it decides rather than builds. When its + # fetch fails the heavy jobs downstream resolve to `skipped`, not `failure`, so + # a gate that watched only them stayed green over a run where nothing was built. + # Every job that can run on a pull request belongs here; only the schedule-only + # benchmark job is outside, and CiGateCoverageGuardTest holds that line. if: always() && github.event_name != 'schedule' - needs: [architecture-and-documentation-guards, build-and-test, examples-generation, binary-compat, perf-smoke] + needs: [architecture-and-documentation-guards, changes, build-and-test, examples-generation, binary-compat, perf-smoke] runs-on: ubuntu-latest steps: - name: Fail if any required job failed diff --git a/CHANGELOG.md b/CHANGELOG.md index 47564c63..eab3eb9d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,17 @@ follow semantic versioning; release dates are ISO 8601. four that did match kept the job green. The list is now the five guards that live in the engine module, and `CiGuardListGuardTest` fails the job if a name in it stops resolving. +- **The aggregate status check notices when nothing was built.** `CI Gate` is one of + the two checks `develop` and `main` require, and it watched the four heavy jobs + without watching the path-detection job they all gate on. When that job's + `git fetch` returned HTTP 503 the four resolved to `skipped` rather than `failure`, + so the gate found nothing to report and went green over a run that compiled + nothing — leaving both required checks green and, on a pull request, the branch + protection satisfied by a build that never happened. The gate now aggregates the + detection job too, and `CiGateCoverageGuardTest` reads the workflow and fails if + any job that can run on a pull request is left out of it. Schedule-only jobs are + recognised from their own `if:` condition rather than an exclusion list, so a new + job either joins the gate or fails the guard. - **A documentation-only pull request is compiled.** Markdown was not a change-detection input, so a PR touching only `.md` skipped the reactor build and merged without `DocumentationSnippetCompileTest` ever compiling the java diff --git a/core/src/test/java/com/demcha/documentation/CiGateCoverageGuardTest.java b/core/src/test/java/com/demcha/documentation/CiGateCoverageGuardTest.java new file mode 100644 index 00000000..e77d69bb --- /dev/null +++ b/core/src/test/java/com/demcha/documentation/CiGateCoverageGuardTest.java @@ -0,0 +1,159 @@ +package com.demcha.documentation; + +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Guards that the aggregate CI status check watches every job that can run on a + * pull request. + * + *

{@code CI Gate} exists so branch protection has one stable check to require + * instead of matrix legs the path filter sometimes skips. It fails when a job it + * {@code needs} reports {@code failure} or {@code cancelled}; a job the filter + * legitimately skipped counts as success, which is the whole point.

+ * + *

That design has one hole, and it is not hypothetical. {@code changes} — the + * path-detection job every heavy job gates on — was absent from the gate's + * {@code needs}. When its {@code git fetch} returned HTTP 503 the job failed, the + * four reactor jobs downstream resolved to {@code skipped} rather than + * {@code failure}, and the gate reported success over a run that compiled nothing. + * Both checks branch protection is pointed at were green with zero tests + * executed.

+ * + *

So the rule is: every job in the workflow is either aggregated by the gate or + * demonstrably cannot run on a pull request. The second case is derived from the + * job's own {@code if:} condition rather than a list somebody maintains, so a new + * job joins the gate or fails this test — it cannot slip through by being + * forgotten.

+ */ +class CiGateCoverageGuardTest { + + private static final Path PROJECT_ROOT = RepoRoot.get(); + private static final Path WORKFLOW = PROJECT_ROOT.resolve(".github/workflows/ci.yml"); + + /** The aggregate check. It cannot depend on itself. */ + private static final String GATE = "ci-gate"; + + /** A job key: two-space indent under {@code jobs:}, nothing else on the line. */ + private static final Pattern JOB_KEY = Pattern.compile("(?m)^ ([a-z][a-z0-9-]*):$"); + + /** A job-level {@code if:} — four-space indent, first line only. */ + private static final Pattern JOB_IF = Pattern.compile("(?m)^ if: (.*)$"); + + /** An inline {@code needs: [a, b, c]} flow sequence. */ + private static final Pattern JOB_NEEDS = Pattern.compile("(?m)^ needs: \\[([^]]*)]"); + + /** + * A job that only runs on a schedule or a manual dispatch never appears on a + * pull request, so the gate has nothing to aggregate from it. + */ + private static final Pattern SCHEDULE_ONLY = Pattern.compile("github\\.event_name == 'schedule'"); + + @Test + void ciGateAggregatesEveryJobThatCanRunOnAPullRequest() throws IOException { + Map jobs = jobBlocks(); + + assertThat(jobs) + .describedAs("no jobs parsed out of %s — this guard is reading a workflow shape " + + "that moved, so it is no longer guarding anything", relative(WORKFLOW)) + .isNotEmpty() + .containsKey(GATE); + + Set aggregated = needsOf(jobs.get(GATE)); + assertThat(aggregated) + .describedAs("the '%s' job must declare an inline 'needs: [...]' list", GATE) + .isNotEmpty(); + + List unwatched = new ArrayList<>(); + for (Map.Entry job : jobs.entrySet()) { + String id = job.getKey(); + if (id.equals(GATE) || runsOnlyOnASchedule(job.getValue()) || aggregated.contains(id)) { + continue; + } + unwatched.add(id); + } + + assertThat(unwatched) + .describedAs("every job that can run on a pull request must be in the '%s' needs " + + "list, or a failure there leaves the aggregate check green over a run " + + "that built nothing", GATE) + .isEmpty(); + } + + @Test + void ciGateDoesNotDependOnAJobThatIsGone() throws IOException { + Map jobs = jobBlocks(); + Set aggregated = needsOf(jobs.get(GATE)); + + assertThat(jobs.keySet()) + .describedAs("'%s' aggregates a job id that no longer exists: the workflow would " + + "fail to load, and the rename that caused it is the finding", GATE) + .containsAll(aggregated); + } + + /** Job id to the source block that declares it, in workflow order. */ + private static Map jobBlocks() throws IOException { + // The workflow is checked out with CRLF on Windows. Normalise once, so the + // line-anchored patterns below capture ids and conditions without a trailing + // carriage return riding along into every comparison. + String workflow = Files.readString(WORKFLOW).replace("\r\n", "\n"); + // `on:` carries keys at the same indentation as a job (`push:`, `schedule:`), + // so parsing starts after the `jobs:` key rather than at the top of the file. + int jobsAt = workflow.indexOf("\njobs:\n"); + assertThat(jobsAt) + .describedAs("no top-level 'jobs:' key in %s", relative(WORKFLOW)) + .isNotNegative(); + String body = workflow.substring(jobsAt); + + Map blocks = new LinkedHashMap<>(); + Matcher key = JOB_KEY.matcher(body); + List ids = new ArrayList<>(); + List starts = new ArrayList<>(); + while (key.find()) { + ids.add(key.group(1)); + starts.add(key.end()); + } + for (int i = 0; i < ids.size(); i++) { + int end = i + 1 < ids.size() ? starts.get(i + 1) : body.length(); + blocks.put(ids.get(i), body.substring(starts.get(i), end)); + } + return blocks; + } + + private static Set needsOf(String jobBlock) { + Matcher needs = JOB_NEEDS.matcher(jobBlock); + if (!needs.find()) { + return Set.of(); + } + Set ids = new LinkedHashSet<>(); + for (String id : needs.group(1).split(",")) { + String trimmed = id.trim(); + if (!trimmed.isEmpty()) { + ids.add(trimmed); + } + } + return ids; + } + + private static boolean runsOnlyOnASchedule(String jobBlock) { + Matcher condition = JOB_IF.matcher(jobBlock); + return condition.find() && SCHEDULE_ONLY.matcher(condition.group(1)).find(); + } + + private static String relative(Path path) { + return PROJECT_ROOT.relativize(path).toString().replace('\\', '/'); + } +}