feat(compliance): report how many files each run examined - #11
Merged
Conversation
Both compliance runners walk the files they are handed through the same four-exit loop: dropped by the filter, absent from disk, no result returned, or examined. Three of those are drops and one of the three is completely silent. Neither runner reports how many files it examined, so a run that checked every file and a run that checked none produce the same output and the same green check. Characterisation tests, written to pass against today's behaviour so the absence is recorded rather than described. Each says what it should become, in the same style as the entry-point tests beside them. Five assertions on the runners: no coverage line from either runner when nothing is examined, no output at all when the filter discards a file, and no counts in the json payload for a machine-readable consumer. The filter-discard case uses a name that a '*AssemblyInfo.cs' pathspec selects and the project filter then rejects, which is the shape that produces a green check having inspected nothing. Two on the action: the runner invocation is inside a collapsed group, so anything printed there is hidden by default, and the job summary carries only pass or fail. The first is asserted because the second's reasoning depends on it.
A compliance run that examined every file it was handed and one that examined none produced the same output and the same green check. Both runners now report a denominator, so a green can be read as evidence rather than taken on trust. The accounting is a separate class rather than counters inlined in each entry point. Those entry points compile against BHoM types, so inlined counters could only be checked by a full CI run against a real dependency closure; extracted, the arithmetic and the wording are tested directly on a bare runner. It holds counters and formats strings and decides nothing. Both runners walk the same four-exit loop, so both are instrumented. Leaving one of two identical loops uninstrumented would be an arbitrary asymmetry and the gap would be found again later. Where the counts go, and where they deliberately do not: console and github get a Coverage line naming files examined out of files handed in, plus each drop reason that actually occurred. Reasons that did not occur are omitted rather than listed at zero, so the one that fired is not buried. json and sarif get the counts inside the payload. They must not get the line: those formats put a payload on stdout and nothing may precede it. A test asserts the payload parses with coverage present, so the gating is held in place rather than merely observed to work once. The job summary gets the count, read off the one line the runner controls. The runner's own output sits inside a log group the UI collapses by default, so a number printed only there is invisible to a reader who is not already suspicious. Both ends of that contract are asserted. When files were handed in and none was examined, a warning names the breakdown rather than the bare fact. The breakdown is what separates a pull request that changed nothing relevant from a selection pattern handing over files the check will never accept: a file can be selected by the changed-file pattern, counted into the decision not to skip, handed over, and then rejected by the runner's own filter. Drop reasons are counted but not sub-classified. A file dropped as not relevant may have been excluded on purpose, since project compliance deliberately skips test projects and anything under .ci/. Telling those apart would require the filter to explain itself, which is a change to a class both runners share. Making the ratio visible and diagnosing it are separate jobs. Reporting only. No exit code changes, no merged status changes, and the question of whether examining nothing should fail is untouched and still open.
Three pre-existing tests asserted the silence this change removes. ExaminedCount_IsNotReported recorded that no count existed and said adding one was the cheapest partial mitigation. The count now exists, so the assertion is inverted and renamed. It still does not touch the pass-versus-fail question, which is what that test was careful to say and is still open. The two GitHubOutput_ForPassingRun_ProducesNoAnnotationLines tests, one per runner, asserted that stdout was entirely empty for a run whose only file was filtered out. That is a stronger claim than their names make, and the silence it protected was the defect: a run that examined nothing looked exactly like a run that examined everything. They now assert what the name means, that a passing run produces no finding annotations, and additionally that it reports the zero it examined. Renamed to match.
The summary reworded the runner's line and produced 'Files examined: 0 of 1 file(s) examined', which reads badly and put the same sentence in two places where it could drift. The runner owns the wording; the action owns where it appears.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A compliance run that examined every file it was handed and a run that examined none produce the
same output and the same green check. There is no number anywhere that distinguishes them.
Both runners walk the same loop with four exits:
Three of the four are drops, and the first prints nothing at all — not even under
--output console. That one matters most, because it is reachable from a pattern/filter disagreement: afile can be selected by the changed-file pattern, counted into the decision not to skip the
check, handed to the runner, and then rejected by the runner's own filter.
*AssemblyInfo.csselects
Properties/NotAssemblyInfo.cs; the project filter requires the name to equalAssemblyInfo.csexactly. The check reports success having inspected nothing, and says sonowhere.
What changed
A denominator, from both runners.
Coverage: 1 of 4 file(s) examined; 1 not relevant to this check, 1 not found on disk, 1 returned no result.Reasons that did not occur are omitted ratherthan listed at zero, so the one that fired is not buried.
A warning when files were handed in and none was examined, carrying the breakdown rather
than the bare fact.
3 file(s) were handed to this check and none was examined: 3 not relevant to this check. The check reports success without having inspected anything. If the files were dropped as not relevant, the pattern that selected them and the filter that rejected them disagree.The bare fact does not distinguish a pull request that changed nothing relevant from aselection layer handing over files the check will never accept; the breakdown does.
Both runners, because both have the identical loop. Instrumenting one would be an arbitrary
asymmetry, and the gap would be found again later.
Where the counts go, and where they deliberately do not
console,githubCoverage:linejson,sarifThe last row is the one worth explaining. The action wraps the runner in
::group::, which theGitHub UI collapses by default, so a number printed only to stdout is invisible to a reader
who is not already suspicious. The summary is the surface people actually read, and it previously
carried one line saying only whether the check passed. The action now reads the count off the one
line the runner controls and puts it there. Both ends of that contract are asserted, so renaming
the line on one side fails a test rather than silently dropping the count.
What is deliberately not done
Drop reasons are counted, not sub-classified. A file dropped as not relevant may have been
excluded on purpose — project compliance deliberately skips test projects and anything under
.ci/— or may be a pattern/filter disagreement. Telling those apart needs the filter to explainitself, which is a change to a class both runners share. Making the ratio visible and diagnosing
it are separate jobs, and this is the first.
Nothing fails that did not fail before. No exit code changes, no merged status changes. The
question of whether examining nothing should fail is untouched and still open. This reports a
number and decides nothing, which is why it does not need that question answered first.
Verification
80 tests, 9 of them on arithmetic and wording, in the hermetic project. The accounting is a
separate class rather than counters inlined in each entry point: those entry points compile
against BHoM types, so inlined counters could only be checked by a full CI run against a real
dependency closure. Extracted, they are tested on a bare runner in milliseconds. The wording is
asserted as well as the arithmetic, because that text is the deliverable — it is what a reader
sees when a check reports success having examined nothing — and it should change by a deliberate
edit to a test, not by drift.
The machine-readable property is pinned, not merely avoided. A test parses the
jsonpayloadwith coverage present and asserts the
Coverage:line is absent from stdout. If the gating isever removed, that test fails rather than a downstream consumer.
The preceding commit characterises the old behaviour, so the diff shows the change
assertion by assertion rather than only as new code. Three pre-existing tests asserted the
silence this removes and were updated in their own commit rather than quietly: two of them
claimed a passing run produces no annotation lines, but asserted something stronger — that
stdout was entirely empty — and that emptiness was the defect.
Confirmed where the text lands, not only that it was written. Running the check for real on a
pull request whose only changed file is selected by the pattern and rejected by the filter:
the run carries a
warningannotation titledCompliance coverage, visible without expandinganything, naming the counts and the likely cause;
the job summary reads
That is item-3's reachable case reporting itself. An earlier iteration of this branch put
Files examined: 0 of 1 file(s) examinedin the summary, which was found by reading the renderedoutput rather than the code, and is why the summary now takes the runner's line verbatim.
What to look at
FileAccounting.CoverageLine()andExaminedNothingWarning(). That wording is the deliverable.OutputEmitter. Removing it would put a line ahead of a payload.denominator is lying and every number above it is unreadable.