feat(config): add --post-assert-script to run a check once after all assertions - #65
Open
kaessert wants to merge 1 commit into
Open
feat(config): add --post-assert-script to run a check once after all assertions#65kaessert wants to merge 1 commit into
kaessert wants to merge 1 commit 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>
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.