Skip to content

fix(test): container flakes now fail CI after #1826, and a same-family false green remains in store_postgres_db_test #202

Description

@cristim

Two follow-ups deferred from LeanerCloud/cloud-commitments-cli#1826 (which closes LeanerCloud/cloud-commitments-cli#1597), both surfaced by that PR's adversarial review.

1. Container-lifecycle flakes now land red instead of skipping

internal/database/postgres/testhelpers/postgres.go:110-113

LeanerCloud/cloud-commitments-cli#1597 was a false green: a t.Skipf on migration failure turned a real failure into a PASS. LeanerCloud/cloud-commitments-cli#1826 correctly converts that to a hard failure. The consequence, which is correct by design but previously undisclosed, is that a transient container-lifecycle error now fails CI too.

Reproduced during the review, roughly 1 in 10 container starts under churn:

Error Trace: testhelpers/postgres.go:111
             migrations/000087_purchase_history_marketplace_listing_test.go:29
Error:       failed to get container port: port "5432/tcp" not found
Messages:    Docker is healthy but the PostgreSQL test container did not come up

The container logged Container is ready and then MappedPort failed. Confirmed a flake rather than a regression: the same test passes 3/3 when run alone on the branch, and passes on main.

This is not an argument to revert. You cannot distinguish "host exhausted" from "something is genuinely broken" at that point, and choosing red is the fail-loud answer. But CI runs -race across six modules, which is exactly the churn that produced it, so expect intermittent reds.

Deliberately not fixed pre-emptively. A retry layer is its own bug surface, and the proportionate move is to measure the rate first. Act only if it proves painful, and then the narrow fix is one retry of SetupPostgresContainer on a container-lifecycle error specifically, not a blanket retry.

What to do here: watch the rate. If it is material, implement the narrow retry with both directions asserted, i.e. a genuine setup failure must still fail after the retry is exhausted.

2. A same-family false green in a file LeanerCloud/cloud-commitments-cli#1826 already edits

internal/config/store_postgres_db_test.go:576-578

t.Run("GetExecutionByID success", func(t *testing.T) {
    if generatedExecID == "" {
        t.Skip("no execution ID available from prior test")
    }

If the earlier subtest fails to produce an ID, this one reports skipped rather than failed. That is the same "answer a failure with no verdict" shape LeanerCloud/cloud-commitments-cli#1597 was filed about. It is not a guarded call, so the new guard from LeanerCloud/cloud-commitments-cli#1826 does not see it.

Out of LeanerCloud/cloud-commitments-cli#1826's stated scope, hence deferred rather than folded in.

Fix direction: fail rather than skip when the precondition was supposed to be produced by a prior step in the same test. A skip is honest only when the environment genuinely cannot provide something; here the environment was expected to provide it and did not.

Worth a sweep for the same shape: a t.Skip whose condition is a value an earlier subtest was responsible for producing. That is distinguishable from an environment skip and is always a false green.

Verification bar for both

Both directions. The genuine failure must fail, and the genuinely-unavailable environment must still skip. A change that makes everything fail breaks every developer machine without Docker; one that makes everything skip restores the bug.

Findings from the 2026-09-02 codebase audit

Added by an automated audit of 3c0f8ac94048a2c36fce5ccddee54e6c4849a5cd (tip of origin/main). Each item below was reported by one reviewer and independently confirmed by a second that did not write it. Full report: docs/audits/codebase-audit-2026-09-02.md.

A04-020 (low)

One more item in this file while it is open, same family of "the harness papers over something". store_postgres_db_test.go:53-59 creates a second unique index on execution_id under the comment "the migration schema does not create a UNIQUE constraint on execution_id". That is false: migration 000008_add_execution_id_unique.up.sql:3 adds CONSTRAINT unique_execution_id UNIQUE (execution_id) and every ON CONFLICT (execution_id) in production depends on it. The harness has already applied the full migration chain at :49-51 before layering the redundant index at :55-56, so the integration suite runs against a schema with two unique indexes and would keep passing if 000008 were ever dropped -- the only regression the extra DDL could have caught. Deleting the DDL and the comment makes the suite run against exactly the migrated schema. (audit finding A04-020)

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions