diff --git a/core/src/test/java/com/demcha/documentation/CiGateCoverageGuardParsingTest.java b/core/src/test/java/com/demcha/documentation/CiGateCoverageGuardParsingTest.java index 9024b9b5..2ad52e94 100644 --- a/core/src/test/java/com/demcha/documentation/CiGateCoverageGuardParsingTest.java +++ b/core/src/test/java/com/demcha/documentation/CiGateCoverageGuardParsingTest.java @@ -84,6 +84,77 @@ void keysNestedInsideAJobAreNotMistakenForJobs() { .containsOnlyKeys("build-and-test"); } + @Test + void aJobWrittenAsAnInlineMappingIsSeen() { + Map jobs = CiGateCoverageGuardTest.jobBlocks(""" + on: [push] + + jobs: + deploy: {runs-on: ubuntu-latest} + build-and-test: + runs-on: ubuntu-latest + """); + + assertThat(jobs) + .describedAs("GitHub documents the value of jobs. as a map, and an inline " + + "mapping is one — a job spelled this way must not be invisible to the " + + "guard, or it can sit outside the gate's needs with the build green") + .containsKeys("deploy", "build-and-test"); + } + + @Test + void quotesAroundAKeyAreNotPartOfTheJobId() { + Map jobs = CiGateCoverageGuardTest.jobBlocks(""" + on: [push] + + jobs: + "build-and-test": + runs-on: ubuntu-latest + 'perf_smoke': + runs-on: ubuntu-latest + """); + + assertThat(jobs) + .describedAs("the quotes are YAML's encoding of the key, not the name — kept, the " + + "id could never match the bare name in the gate's needs list and a legal " + + "workflow would be reported as having an unwatched job") + .containsOnlyKeys("build-and-test", "perf_smoke"); + } + + /** + * A key GitHub would reject is still parsed, so the guard can report it. + * + *

{@code "security scan"} is not a legal job id — GitHub requires a leading + * letter or {@code _} and then only letters, digits, {@code -} or {@code _}. The + * parser does not filter on that grammar, because filtering is how a key becomes + * invisible; it hands the key over and + * {@code CiGateCoverageGuardTest#everyJobIdIsOneGitHubWouldAccept} fails on it.

+ */ + @Test + void aKeyThatIsNotALegalJobIdIsStillParsedRatherThanSkipped() { + Map jobs = CiGateCoverageGuardTest.jobBlocks(""" + on: [push] + + jobs: + "security scan": + runs-on: ubuntu-latest + """); + + assertThat(jobs) + .describedAs("an unparseable job-level key must reach the assertions rather than " + + "vanish — a guard that filters its input reports on a subset and calls " + + "it coverage") + .containsOnlyKeys("security scan"); + } + + @Test + void unquotingLeavesAnUnmatchedOrEmbeddedQuoteAlone() { + assertThat(CiGateCoverageGuardTest.unquoted("\"build")).isEqualTo("\"build"); + assertThat(CiGateCoverageGuardTest.unquoted("bui\"ld")).isEqualTo("bui\"ld"); + assertThat(CiGateCoverageGuardTest.unquoted("\"")).isEqualTo("\""); + assertThat(CiGateCoverageGuardTest.unquoted("'perf'")).isEqualTo("perf"); + } + @Test void theBlockOfAJobStopsAtTheNextJob() { Map jobs = CiGateCoverageGuardTest.jobBlocks(""" diff --git a/core/src/test/java/com/demcha/documentation/CiGateCoverageGuardTest.java b/core/src/test/java/com/demcha/documentation/CiGateCoverageGuardTest.java index 001a0cb9..26f304a7 100644 --- a/core/src/test/java/com/demcha/documentation/CiGateCoverageGuardTest.java +++ b/core/src/test/java/com/demcha/documentation/CiGateCoverageGuardTest.java @@ -48,23 +48,43 @@ class CiGateCoverageGuardTest { private static final String GATE = "ci-gate"; /** - * A job key: two-space indent under {@code jobs:}, then nothing that changes - * what the key is — trailing spaces or a {@code #} comment are allowed. + * A job key: a two-space-indented mapping key under {@code jobs:}, whatever + * follows the colon. * - *

The name is matched structurally rather than against GitHub's identifier - * grammar. A character class admitting only what today's job names happen to - * use — lower case and hyphens — drops a job called {@code build_and_test} or - * {@code CodeQL} on the floor, and requiring the line to end at the colon drops - * {@code security_scan: # nightly}. Either way the guard reports on the jobs it - * parsed, and one it never saw is one it cannot report missing from the gate: - * the job is absent from {@code needs}, the test is green, and the aggregate - * check is blind to it. That is the silence this guard exists to break, so the - * pattern takes every key the YAML puts at that level and lets the assertions - * decide. {@link #jobBlocks(String)} is package-private for exactly this - * reason — {@code CiGateCoverageGuardParsingTest} feeds it the shapes the real - * workflow does not currently contain.

+ *

The key is matched structurally — anything YAML puts at that + * level — rather than against GitHub's identifier grammar, and the difference + * is the whole point. A pattern that only admits legal job ids silently drops + * everything else, and a job this guard never sees is one it cannot report + * missing from the gate: absent from {@code needs}, test green, aggregate check + * blind to it. Matching everything and judging afterwards means an id the guard + * cannot interpret becomes a failure rather than an omission — see + * {@link #everyJobIdIsOneGitHubWouldAccept}.

+ * + *

Accepting any tail after the colon is what admits a job written as an + * inline mapping ({@code deploy: {runs-on: ubuntu-latest}}). GitHub documents + * the value of {@code jobs.} as "a map of the job's configuration + * data", and an inline mapping is a map; requiring the line to end at the colon + * excluded a legal spelling and hid any job using it. YAML only treats a colon + * as a key separator when a space or a line end follows it, which is what the + * lookahead encodes.

+ * + *

Quotes around the key are YAML's, not part of the id, and + * {@link #unquoted(String)} strips them. Left on, {@code "build-and-test":} + * produced an id that could never match the bare name in {@code needs}, so a + * legal workflow was reported as having an unwatched job.

+ * + *

{@link #jobBlocks(String)} is package-private so + * {@code CiGateCoverageGuardParsingTest} can drive it with the spellings this + * repository's own workflow does not contain.

+ */ + private static final Pattern JOB_KEY = + Pattern.compile("(?m)^ (?!#)(\\S[^:]*?):(?=[ \\t]|$)[^\\r\\n]*$"); + + /** + * A job id GitHub accepts: "must start with a letter or {@code _} and contain + * only alphanumeric characters, {@code -}, or {@code _}". */ - private static final Pattern JOB_KEY = Pattern.compile("(?m)^ ([^\\s:#]+):[ \\t]*(?:#.*)?$"); + private static final Pattern LEGAL_JOB_ID = Pattern.compile("[A-Za-z_][A-Za-z0-9_-]*"); /** A job-level {@code if:} — four-space indent, first line only. */ private static final Pattern JOB_IF = Pattern.compile("(?m)^ if: (.*)$"); @@ -109,6 +129,32 @@ void ciGateAggregatesEveryJobThatCanRunOnAPullRequest() throws IOException { .isEmpty(); } + /** + * The parser takes every key at job level, so it can also pick up something that + * is not a job id at all — a malformed workflow, or a shape nobody anticipated. + * + *

That is deliberate, and this is the other half of it. Skipping such a key + * would put the guard back where it started: quietly reporting on a subset. Here + * it fails instead, and names what it could not interpret.

+ */ + @Test + void everyJobIdIsOneGitHubWouldAccept() throws IOException { + List illegal = new ArrayList<>(); + for (String id : jobBlocks().keySet()) { + if (!LEGAL_JOB_ID.matcher(id).matches()) { + illegal.add(id); + } + } + + assertThat(illegal) + .describedAs("job-level keys in %s that GitHub would not accept as job ids — a " + + "job id must start with a letter or '_' and hold only letters, digits, " + + "'-' or '_'. Either the workflow is malformed, or it uses a shape this " + + "guard parses wrongly; both are findings, and neither should be passed " + + "over in silence", relative(WORKFLOW)) + .isEmpty(); + } + @Test void ciGateDoesNotDependOnAJobThatIsGone() throws IOException { Map jobs = jobBlocks(); @@ -155,7 +201,7 @@ static Map jobBlocks(String workflowText) { List ids = new ArrayList<>(); List starts = new ArrayList<>(); while (key.find()) { - ids.add(key.group(1)); + ids.add(unquoted(key.group(1).trim())); starts.add(key.end()); } for (int i = 0; i < ids.size(); i++) { @@ -165,6 +211,27 @@ static Map jobBlocks(String workflowText) { return blocks; } + /** + * Strips the quotes YAML may put around a mapping key. + * + *

{@code "build-and-test"} and {@code build-and-test} are the same key; the + * quotes are the encoding, not the name. Only a matched surrounding pair is + * removed, so a key that merely contains a quote is left alone.

+ * + * @param key the key exactly as it appears in the source + * @return the key with one matched pair of surrounding quotes removed + */ + static String unquoted(String key) { + if (key.length() < 2) { + return key; + } + char first = key.charAt(0); + if ((first == '"' || first == '\'') && key.charAt(key.length() - 1) == first) { + return key.substring(1, key.length() - 1); + } + return key; + } + private static Set needsOf(String jobBlock) { Matcher needs = JOB_NEEDS.matcher(jobBlock); if (!needs.find()) {