Skip to content

Delete the test snapshot in teardown so failures do not orphan it - #1099

Open
kaidaguerre wants to merge 1 commit into
developfrom
fix-snapshot-test-cleanup
Open

Delete the test snapshot in teardown so failures do not orphan it#1099
kaidaguerre wants to merge 1 commit into
developfrom
fix-snapshot-test-cleanup

Conversation

@kaidaguerre

Copy link
Copy Markdown
Contributor

The snapshot acceptance tests upload a snapshot to turbot-ops/clitesting and then delete it — the existing comment says why: "to avoid exceeding quota".

But the DELETE was the last statement of each test body, so it only ran when every assertion passed. Any failure after the upload left the snapshot in the workspace permanently, and these tests upload on every CI run.

Change

Capture the delete URL immediately after the upload, and move the DELETE into a bats teardown(), which runs whether the test passes or fails. req_url is reset in setup() and the DELETE is guarded on it being non-empty, so a skipped test or an upload that never produced a URL is a no-op.

No assertion or expected-output file is touched.

Verification

bats test_files/snapshot.bats with no SPIPETOOLS_TOKEN — parses and skips all three cleanly, teardown no-ops on the empty URL.

The teardown behaviour itself was proven with an equivalent throwaway bats file, since the real one needs a token:

@test "upload succeeds then assertion fails" { req_url="https://pipes/api/v0/snapshot/abc"; [ "1" = "2" ]; }
@test "upload itself fails before url captured"  { false; req_url="never-set"; }

teardown log:

DELETED:https://pipes/api/v0/snapshot/abc
NO_URL

So a post-upload failure still deletes, and a pre-URL failure skips cleanly.

What this does not fix

The current CI failures are 504 Gateway Timeout from the snapshot publish endpoint:

# Error: failed to publish snapshot to turbot-ops/clitesting: 504 Gateway Timeout

A timed-out publish never returns a URL, so there is nothing for teardown to delete — and a 504 is a gateway timeout rather than a rejection, so the backend insert may still have completed. Any snapshots already orphaned in turbot-ops/clitesting need purging in Pipes directly; this PR only stops the pile growing from ordinary test failures.

Worth noting the write path is the only thing affected — database_precedence.bats queries the same workspace with the same token and passes in under a minute, so the workspace and credentials are fine.

Separately, newPipesClient in pipe-fittings builds its HTTP client from pipes.NewConfiguration() with no timeout and no retry, so any slow Pipes call becomes a hard CI failure across steampipe, powerpipe and flowpipe. Adding a timeout plus one retry on 5xx there would make all three resilient, but that affects every consumer so it is left as its own decision.

🤖 Generated with Claude Code

The DELETE was the last statement of each test body, so it only ran when every
assertion passed. Any failure after the upload left the snapshot in
turbot-ops/clitesting permanently — and the tests upload on every CI run, so
those accumulate.

Capture the delete URL immediately after the upload and move the DELETE into a
bats teardown, which runs whether the test passes or fails. The guard on an
empty URL covers a skipped test and an upload that never returned one.
@MichaelBurgess
MichaelBurgess self-requested a review July 28, 2026 11:26
kaidaguerre added a commit that referenced this pull request Aug 18, 2026
Snapshot creates against turbot-ops/clitesting take longer than the API
gateway's 60s limit and return 504, so the three tests in snapshot.bats
fail on most runs. Measured 2026-08-17: three consecutive publishes to
that workspace returned 504 at 60.458s, 60.444s and 60.443s.

The block is commented out rather than deleted so restoring it is a
one-line change once creates are fast again. The tests themselves assert
CSV, JSON and table output formatting in snapshot mode, not the upload
path, so what goes dark is output-format coverage.

A timed-out create still lands server-side, and the client never receives
the snapshot ID it would need to delete the row - so leaving the block
enabled also grows the workspace on every failure. #1099 fixes the other
leak path (a failing assertion skipping the inline delete) and is still
worth landing on its own merits.

Tracked by #1106.
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.

2 participants