fix(ci-serialisation): report every outcome, and quieten the baseline builds - #10
Merged
Merged
Conversation
Three properties of ci-serialisation's action.yml that no test asserted, each broken in a way that produces no failure anywhere: The job summary is written by five mutually-exclusive steps whose conditions between them name five of the eighteen functional steps. A failure in any of the other thirteen matches no condition and writes nothing, so the check goes red with an empty summary. Seven of the thirteen run before serialisation is attempted. Two of the four builds pass -clp:ErrorsOnly and two do not. The two without it compile the base branch, so their warnings annotate the check with diagnostics from code the author did not write. The action resets the assemblies directory itself, immediately before the step that resets it properly. These tests fail on this commit. That is the point: each names the offending lines rather than describing them, so the fix that follows is verifiable rather than asserted. Text assertions rather than a YAML parse, because the powershell-tests job installs Pester and nothing else and all three facts are line-shaped.
… builds Three changes to one action, each closing a defect the preceding commit captured. The job summary was written by five mutually-exclusive steps. Their conditions had to enumerate the states worth reporting, and between them they named five of the action's eighteen functional steps, so a failure anywhere else matched nothing and the check went red with an empty summary. Seven of the unnamed steps run before serialisation is attempted, which is the case a reader is least able to diagnose. Replaced by one always-gated step whose decision lives in a script, so the branch that handles "none of the above" is a branch in code rather than a condition nobody wrote. That branch names the phase the check stopped in, derived from the signals already available, and says a stop is a CI failure rather than a finding about the pull request. Naming the step instead would mean giving every step an id and threading each conclusion through, which is the enumeration that failed here. The five existing messages are unchanged, and coverage figures are still emitted only when the branch leg measured them, so a summary never claims coverage a run did not produce. The two baseline builds now pass -clp:ErrorsOnly, as the branch-leg builds already did. setup-dotnet registers the csc problem matcher, which turns each MSBuild diagnostic into an annotation; these two compile the base branch, so their warnings belong to code the author did not write and consume the per-step annotation cap the caller's own diagnostics need. Console output only: no effect on build behaviour, exit codes, or how errors are reported. The assemblies reset in the base-checkout step is removed. resolve-dependencies removes and recreates that directory, and Upgrades alongside it, in its own Prepare folders step, which is the next step and runs unconditionally. The removed line covered one of the two directories and was redundant with the step after it, while reading as the mechanism that separates the two legs. No change to which checks are required anywhere.
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
Three defects in
ci-serialisation, none of which produces a failure anywhere. Each is aproperty of the action as a whole rather than of any one step, which is why none of them was
visible from reading a single place in the file.
A red check can have an empty job summary. The summary was written by five
mutually-exclusive steps, each gated on a different combination of step conclusions and
outputs. Those conditions had to enumerate the states worth reporting, and between them they
named five of the action's eighteen functional steps:
A failure in any of the thirteen matched no condition and wrote nothing, so the check went red
and said nothing at all. Seven of them run before serialisation is attempted — locating and
building the verification solution, inferring its configuration, publishing the runner — which
is exactly the case a reader is least equipped to diagnose, because the natural reading of a red
serialisation check is that serialisation found something.
Two of the four builds annotate the check with the wrong warnings.
setup-dotnetregistersthe
cscproblem matcher, which turns every MSBuild diagnostic line into a check-runannotation. The branch-leg builds pass
-clp:ErrorsOnlyto suppress that; the two baselinebuilds did not. Those two compile the base branch, so their warnings belong to code the
author did not write and cannot act on, and they consume a per-step annotation cap that the
caller's own diagnostics need.
The assemblies directory is reset twice. The base-checkout step removed the contents of
ProgramData\BHoM\Assemblies. The very next step,resolve-dependencies, removes and recreatesthat directory and
ProgramData\BHoM\Upgradesin its ownPrepare foldersstep, which runsunconditionally. The earlier line was strictly weaker and strictly redundant — and it is the
line a reader looking for where the two legs are separated finds first.
What changed
One summary step instead of five, always-gated, with the decision extracted to
.github/scripts/Write-SerialisationSummary.ps1so it can be tested without running aserialisation check. The five existing messages are unchanged. What is new is the last branch:
a catch-all that names the phase the check stopped in and states that a stop is a CI failure
rather than a finding about the pull request.
The catch-all names the phase, not the step, deliberately. Naming the step would mean giving
all eighteen an
idand threading each conclusion into the condition — which is the enumerationthat failed here in the first place. The phase is derived from the signals already available, so
adding a step cannot reopen the hole.
-clp:ErrorsOnlyon the two baseline builds. Console output only: no effect on buildbehaviour, exit codes, or how errors are reported. Errors still print and still annotate.
The redundant reset removed, with a comment recording where the reset actually happens so
the next reader does not go looking for it here.
Verification
19 new tests, and the three that matter failed before the fix. The first commit adds them
alone; on that commit all three fail, each naming the offending lines rather than describing
them:
The second commit makes them pass. Sixteen further tests cover the summary decision directly,
including the states that previously produced nothing — a failure before the branch leg ran, a
failure in the baseline leg, and the case where nothing can be identified at all.
Full Pester suite green in CI. The structural tests are text assertions rather than a YAML
parse: the PowerShell test job installs Pester and nothing else, and all three facts are
line-shaped, so a YAML dependency would cost more than it returns.
What is not verified
No end-to-end run exercises the paths this changes. That is a property of the changes, not
a gap in the testing:
run produces the same summary as before.
-clp:ErrorsOnlychanges console verbosity. Nothing observable in a green run changes.construction — nothing runs between the two lines.
Reaching the baseline leg for real needs a repository that genuinely fails serialisation, which
is not something the available test repositories provide. It would not prove anything additional
here, and I would rather say so than imply coverage this does not have.
What to look at
Write-SerialisationSummary.ps1. It is the point of the change, andit is the branch that will be wrong first if the action gains a phase.
ci-serialisation-action.Tests.ps1asserts one always-gated summary step. If a second isever added, that test fails — which is intended, and is the guard against the old shape
returning.
counterparts.
Not in scope
ci-versioninghas the same build-flag asymmetry: its caller build passes-clp:ErrorsOnlyand its verification-solution build does not. Left alone here to keep this to one action.
This does not change which checks are required anywhere.