Skip to content

feat(config): add --post-assert-script to run a check once after all assertions - #65

Open
kaessert wants to merge 3 commits into
crossplane:mainfrom
kaessert:feat/post-assert-script
Open

feat(config): add --post-assert-script to run a check once after all assertions#65
kaessert wants to merge 3 commits into
crossplane:mainfrom
kaessert:feat/post-assert-script

Conversation

@kaessert

Copy link
Copy Markdown
Contributor

Description of your changes

Adds a --post-assert-script option that runs a single script after every resource in the test case has been asserted, and before the update, import and delete steps.

The existing uptest.upbound.io/post-assert-hook annotation is per-resource, and is rendered inside the assert step's try block — interleaved with the assertions themselves:

- assert:   <resource 1>
- command:  <resource 1 post-assert-hook>
- assert:   <resource 2>
- command:  <resource 2 post-assert-hook>
...

So a hook only ever observes one resource, at a point where later resources may not have been asserted yet. That is the right shape for a per-resource check and the wrong shape for any check whose subject is the test case as a whole. There is currently no phase boundary a consumer can extend for the latter.

Motivating case: a convergence check — asserting that managed resources reach steady state and stop issuing spurious Update calls (a resource stuck in an update loop reports Ready on every cycle, so a Ready assertion passes happily while the provider writes to the backend forever).

Run per resource, that check costs one observation window per resource, and each window observes a different stretch of wall-clock time. Run once, it costs a single window and observes every resource over the same stretch — which is both faster and a stronger assertion, since cross-resource interference falls between per-resource windows and is never seen.

Measured on a provider with 20 managed resources at a 15s window: 316s of sequential windows became 19s, with identical per-resource verdicts.

Two deliberate choices:

  • It renders its own named step, rather than appending to the assert step's try block. A failure is then attributed to Post Assert in the chainsaw output, instead of to whichever resource happened to be asserted last.
  • An unset flag renders nothing. The test case is byte-for-byte unchanged for consumers that do not opt in; there is a test asserting exactly that.

Plumbed through AutomatedTest, TestCase, the Builder, CLI flags and a template conditional, following the existing SetupScriptPath / TeardownScriptPath pattern.

Also extracts absScriptPath: the three script flags share identical resolve-or-die logic, and a third copy put e2eTests over the gocyclo limit. Net effect is one fewer branch in e2eTests than before this change.

Fixes #

I have:

  • Run make reviewable test to ensure this PR is ready for review.

    (This repo has no reviewable target; ran the CI gates directly — make lint0 issues., make -j2 test → all packages ok, templates coverage 88.2%, make check-diffbranch is clean.)

How has this code been tested

Unit — two new renderer tests: a golden-output test with the flag set, and a negative control asserting no Post Assert step is rendered when it is unset.

Both were verified to fail under mutation rather than merely pass:

mutation result
render the step unconditionally ({{- if true }}) TestRenderWithoutPostAssertScriptOmitsTheStep FAILS
move the step inside the per-resource range both new tests FAIL

The rendered output was also validated with chainsaw lint test -f 00-apply.yamlThe document is valid.

End-to-end, against a real provider (provider-infoblox-nios), a kind cluster and a live backend, with 20 managed resources and the new flag pointing at a convergence-barrier script:

Step execution order from the chainsaw log — the barrier runs once, after all 20 assertions:

Run Setup Script         | TRY | BEGIN → END
Apply Resources          | TRY | BEGIN → END
Assert Status Conditions | TRY | BEGIN → END      (all 20 resources)
Post Assert              | TRY | BEGIN → END      (once)
case outcome
barrier passes uptest exit 0 — 20/20 resources stable in one shared 15s window
barrier fails (one resource forced into a genuine update loop) uptest exit 1, failure attributed to Post Assert, the other 19 resources still reported individually

Worth noting for the failure case: the Assert Status Conditions step passedReady stayed True throughout — so the post-assert step was the only thing that caught it. That is the gap this option exists to let consumers close.

Relative script paths are resolved to absolute, verified via --render-only.

…assertions

Add a --post-assert-script option that runs a single script after every
resource in the test case has been asserted, and before the update, import
and delete steps.

The existing "uptest.upbound.io/post-assert-hook" annotation is per-resource
and is rendered inside the assert step's try block, interleaved with the
assertions themselves. A hook therefore only ever observes one resource, at a
point where later resources may not have been asserted yet. That is the right
shape for a per-resource check and the wrong shape for any check whose subject
is the test case as a whole.

The motivating case is a convergence check: asserting that managed resources
reach steady state and stop issuing spurious Update calls. Run per resource,
it costs one observation window per resource and each window observes a
different stretch of wall-clock time. Run once, it costs a single window and
observes every resource over the same stretch, which is both faster and a
stronger assertion — cross-resource interference falls between per-resource
windows and is never seen. Measured on a provider with 20 managed resources
and a 15s window: 316s of sequential windows became 19s, with identical
per-resource verdicts.

The flag is plumbed through AutomatedTest, TestCase, the Builder, CLI flags
and a template conditional, following the existing SetupScriptPath and
TeardownScriptPath pattern. It renders its own named step rather than
appending to the assert step's try block, so a failure is attributed to
"Post Assert" in the chainsaw output rather than to whichever resource
happened to be asserted last. An unset flag renders nothing, leaving the
test case byte-for-byte unchanged for consumers that do not opt in.

Also extract absScriptPath: the three script flags share identical
resolve-or-die logic, and a third copy put e2eTests over the gocyclo limit.

Signed-off-by: Tobias Kässer <tobias.kasser@upbound.io>
tester-ant and others added 2 commits September 5, 2026 12:35
PrepareManifests now decodes every document of a manifest file up front,
resolves an optional <manifest>.yaml.uptest sidecar (github.com/kaessert/
crossplane-update-tester/sidecar v0.1.0) against the whole document set, and
merges its annotations onto the matching objects in place before anything
downstream (the rendered chainsaw case, the applied object, the assertion
templates) sees them.

A manifest with no sidecar is untouched: sidecar.Load's own (nil, nil)
behaviour on a missing file, combined with the existing decode/marshal path,
keeps the un-migrated case byte-identical to before this change.

A sidecar's ${data.*} placeholders go through the same substitution the
manifest's own text does (injectValues split into injectDataSource +
injectRandom for this). ${Rand.*} is deliberately left untouched in a
sidecar's text, so a random placeholder used in a name:/namespace: selector
is rejected by the sidecar package's own templating check instead of being
silently substituted to a value that could never match the manifest's.

Once a sidecar exists for a manifest, any document in that file still
carrying one of uptest's own annotation keys inline is a hard error (switch,
not overlay) -- checked across every document in the file, not only the ones
the sidecar targets, so a stray annotation left on a prerequisite object is
caught rather than silently ignored.

PrepareManifests logs how many manifest sidecars it loaded, so an operator
reading ordinary E2E output can tell a sidecar-aware run from one that read
zero annotations.

go.mod: go 1.24.6 -> 1.25.0, required by the sidecar module's own go
directive.
de42c57 bumped go.mod's go directive 1.24.6 -> 1.25.0 (required by
sidecar's own go directive), but golangci-lint v1.64.8 -- the version
pinned by Makefile and ci.yml -- refuses to run at all against a
go1.25 target (built with go1.24). The ambient v2.12.2 binary also
failed, for the opposite reason: .golangci.yml was still v1 format.

- migrate .golangci.yml to v2 config format (golangci-lint migrate
  --skip-validation, hand-fixed to drop govet.check-shadowing which
  the v1 schema no longer accepts and to restore the SPDX header the
  migration does not carry over)
- bump GOLANGCILINT_VERSION 1.64.8 -> 2.12.2 (Makefile) and
  GOLANGCI_VERSION v1.64.8 -> v2.12.2 + GO_VERSION 1.24 -> 1.25
  (ci.yml), matching go.mod's go 1.25.0
- bump golangci-lint-action v3 -> v7.0.1: v3 only understands
  golangci-lint v1 output
- raise GO_REQUIRED_VERSION 1.24 -> 1.25 (Makefile) to match

The version bump surfaced pre-existing findings the old engine
missed, all on the base tag's tree, not introduced by this branch:

- internal/templates/renderer.go: the four chainsaw step file names
  were literal strings in two places (here and internal/tester.go).
  Extracted to exported constants (ApplyFilename etc.) and pointed
  both call sites at them.
- internal/tester.go: one exec.Command in a background log-collector
  goroutine flagged by noctx; switched to exec.CommandContext. The
  goroutine already receives a context from its caller
  (executeSingleTestFileCLIMode) via one extra parameter, which also
  cleared a gosec G118 finding (context.Background() used where a
  request-scoped context was available) that appeared once the first
  fix went in.
- .golangci.yml: added goconst to the existing test-file exclusion
  rule (same rule already excludes dupl/errcheck/gocyclo/gosec for
  _test.go). The remaining 19 goconst findings were all repeated
  table-driven test fixture literals in renderer_test.go; goconst's
  own default settings (min-len 3, min-occurrences 5) are unchanged.

One v1 rule could not be carried forward: {text: k8s.io/api/core/v1,
linters: [goimports]}. golangci-lint v2 moved goimports to
formatters, and formatters.exclusions has no text-matching rules in
v2 (only generated/paths) -- confirmed against the v2 JSON schema.
This is a platform limitation, not a migration defect: no current
file triggers it, confirmed by the clean run below.

Enabled linter set is unchanged. golangci-lint linters at v1.64.8
(old config) vs v2.12.2 (new config) differ only by two documented
v2 platform reshuffles, not a config authoring choice: gofmt and
goimports moved from linters to formatters (still enabled, still
run, just a different config section), and gosimple's checks merged
into the unified staticcheck linter. Every other one of the 41
linters is identical on both sides.

Verified on this tree (de42c57's tree plus the above):
- go build ./... && go vet ./... && go test ./... -count=1 && gofmt -l .: clean
- golangci-lint run ./... (v2.12.2, migrated config): 0 issues, exit 0
  (confirmed twice, once with a fresh cache)
- also confirmed 0 issues at v2.3.0-fork.987fe26 (the base tag) with
  the same new binary+config, isolating that these are pre-existing
  findings unlocked by the linter version bump, not something this
  branch's own commits introduced
- git diff --name-only: confined to the five files above; go.mod,
  go.sum, internal/prepare.go and internal/prepare_test.go untouched
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant