Skip to content

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

Open
kaessert wants to merge 1 commit 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 1 commit 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>
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