Close integration testing gaps #12

New issue

Open

opened 2026-06-01 17:34:37 +00:00 by erik · 0 comments

erik commented

2026-06-01 17:34:37 +00:00

Owner

Testing gaps

Two gaps in the existing test suite, both at the storage boundary.

No real object storage smoke test in CI

internal/objectstore/s3.go (the minio-go-backed implementation) and internal/objectstore/fake.go are exercised together only by the fake. ROADMAP.md mentions "test against local object storage " but no .forgejo/workflows/ job currently runs against a real S3-compatible backend.

What's at risk:

CAS semantics (PutObjectIfMatch, PutObjectIfNoneMatch) — the fake implements them; real S3-compatible backends implement them with subtly different ETag normalisation rules across providers.
The iter.Seq2-based pagination in List — behaviour with continuation tokens, prefix-delimited listings.
Not-found mapping — the fake returns ErrNotFound directly; the real backend returns minio-go errors that get wrapped.
Content-type handling.

Proposed change: Add a just test::integration recipe that runs the existing integration tests against a containerised object store. A Forgejo workflow gates it on PRs touching internal/objectstore/, internal/upload/, internal/gc/, or internal/deviceregistry/. ~20 lines of YAML plus a small docker-compose for something like RustFS.

No fault injection for the tombstone lifecycle

internal/upload/tombstone_recovery_test.go covers state mid-lifecycle by constructing the bucket and SQLite into known intermediate shapes and asserting reconciliation converges. It does not test "kill the process at step boundary N and verify recovery from any N."

What's at risk: The four-step lifecycle has four crash windows. A future change to the order (or the addition of a fifth step) needs to re-prove correctness at every window. Without a property-style test the proof is unstated.

Proposed change: A parameterised test that, for each step boundary (1-after-pending, 2-after-real, 3-after-delete-pending, 4-after-commit), forces the lifecycle to halt at that point, then runs ReconcileTombstones and asserts the post-state matches a full-publish run. Five cases, one parameterised test.

This becomes much cheaper once tombstone-lifecycle-typed-pipeline lands — the lifecycle value can take a haltAfter step name and the test just iterates the four step names.

# Testing gaps Two gaps in the existing test suite, both at the storage boundary. ## No real object storage smoke test in CI `internal/objectstore/s3.go` (the minio-go-backed implementation) and `internal/objectstore/fake.go` are exercised together only by the fake. `ROADMAP.md` mentions "test against local object storage " but no `.forgejo/workflows/` job currently runs against a real S3-compatible backend. **What's at risk:** - CAS semantics (`PutObjectIfMatch`, `PutObjectIfNoneMatch`) — the fake implements them; real S3-compatible backends implement them with subtly different ETag normalisation rules across providers. - The `iter.Seq2`-based pagination in `List` — behaviour with continuation tokens, prefix-delimited listings. - Not-found mapping — the fake returns `ErrNotFound` directly; the real backend returns minio-go errors that get wrapped. - Content-type handling. **Proposed change:** Add a `just test::integration` recipe that runs the existing integration tests against a containerised object store. A Forgejo workflow gates it on PRs touching `internal/objectstore/`, `internal/upload/`, `internal/gc/`, or `internal/deviceregistry/`. ~20 lines of YAML plus a small docker-compose for something like RustFS. ## No fault injection for the tombstone lifecycle `internal/upload/tombstone_recovery_test.go` covers state mid-lifecycle by constructing the bucket and SQLite into known intermediate shapes and asserting reconciliation converges. It does not test "kill the process at step boundary N and verify recovery from any N." **What's at risk:** The four-step lifecycle has four crash windows. A future change to the order (or the addition of a fifth step) needs to re-prove correctness at every window. Without a property-style test the proof is unstated. **Proposed change:** A parameterised test that, for each step boundary (1-after-pending, 2-after-real, 3-after-delete-pending, 4-after-commit), forces the lifecycle to halt at that point, then runs `ReconcileTombstones` and asserts the post-state matches a full-publish run. Five cases, one parameterised test. This becomes much cheaper once [[tombstone-lifecycle-typed-pipeline]] lands — the lifecycle value can take a `haltAfter` step name and the test just iterates the four step names. **Related:** [[tombstone-lifecycle-typed-pipeline]].

erik self-assigned this

2026-06-01 17:34:37 +00:00