feat(config): add --post-assert-script to run a check once after all assertions - #65
Open
kaessert wants to merge 3 commits into
Open
feat(config): add --post-assert-script to run a check once after all assertions#65kaessert wants to merge 3 commits into
kaessert wants to merge 3 commits into
Conversation
…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>
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
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.
Description of your changes
Adds a
--post-assert-scriptoption 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-hookannotation is per-resource, and is rendered inside the assert step'stryblock — interleaved with the assertions themselves: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
Updatecalls (a resource stuck in an update loop reportsReadyon every cycle, so aReadyassertion 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:
tryblock. A failure is then attributed toPost Assertin the chainsaw output, instead of to whichever resource happened to be asserted last.Plumbed through
AutomatedTest,TestCase, theBuilder, CLI flags and a template conditional, following the existingSetupScriptPath/TeardownScriptPathpattern.Also extracts
absScriptPath: the three script flags share identical resolve-or-die logic, and a third copy pute2eTestsover thegocyclolimit. Net effect is one fewer branch ine2eTeststhan before this change.Fixes #
I have:
Run
make reviewable testto ensure this PR is ready for review.(This repo has no
reviewabletarget; ran the CI gates directly —make lint→0 issues.,make -j2 test→ all packages ok, templates coverage 88.2%,make check-diff→branch 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 Assertstep is rendered when it is unset.Both were verified to fail under mutation rather than merely pass:
{{- if true }})TestRenderWithoutPostAssertScriptOmitsTheStepFAILSrangeThe rendered output was also validated with
chainsaw lint test -f 00-apply.yaml→The 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:
uptestexit 0 — 20/20 resources stable in one shared 15s windowuptestexit 1, failure attributed toPost Assert, the other 19 resources still reported individuallyWorth noting for the failure case: the
Assert Status Conditionsstep passed —ReadystayedTruethroughout — 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.