fix(test): flaky and unrunnable test suites - #1029
Merged
Merged
Conversation
alexbouchardd
approved these changes
Aug 10, 2026
The subtest wrote into the Config map shared by every subtest in TestAWSS3Destination_Validate, all of which run in parallel, so it could turn the valid-destination case invalid.
Every test in a provider's PublisherSuite reads one channel fed by one consumer built in SetupSuite. GetRecords can hand back a record an earlier call already returned — seen against localstack with no error reported and the shard iterator advanced normally — and a single redelivered record then shifts every later read by one, so each test verifies some other test's event. The symptoms all trace to that one shift. TestBasicPublish passes and its record comes around again, so TestClosePublisherDuringConcurrentPublish and TestConcurrentPublish panic on a payload with no message_id, and TestPublishWithDeliveryMetadata reports an event-id mismatch. It needs a slow localstack to show up, which is why it reads as a cold-start flake: restarting the container right before the run failed 10 of 20 times, and 0 of 20 with this change. Records within a shard are ordered, so tracking the last sequence number handed to the suite and dropping anything at or before it makes the stream deliver-once from the suite's point of view. Kinesis is the only consumer built on a position token; the other nine ack individually or are pushed, so none of them can replay this way. Also report a message that carries no message_id as a plain failure naming the payload. That check previously panicked inside a type assertion, which said nothing about what had gone wrong.
Each test in the Kinesis suite now creates its own stream and consumer
rather than sharing one built in SetupSuite, so nothing a test leaves
behind can reach the next one. Four stream create/deletes add about two
seconds to the suite.
The sequence-number check stays, for its own reason: a stream is an
at-least-once log and the consumer has to be idempotent whatever the
suite does around it. It also covers a record redelivered inside the test
that published it, which per-test streams cannot.
Ensure{RabbitMQ,LocalStack,Kafka,GCP} handed back endpoints supplied
through the environment without checking that anything was listening.
`docker compose up -d` returns once containers are created, not once they
accept connections, so a suite that connects immediately could lose the
race — seen as a RabbitMQ connection reset partway through SetupSuite.
Each now waits up to 30 seconds before the first test runs: an AMQP dial
for RabbitMQ, the health endpoint for LocalStack, a TCP dial for Kafka and
the Pub/Sub emulator. Only on the environment path; testcontainers already
applies its own wait strategies.
alexluong
force-pushed
the
fix/destawss3-validate-parallel-race
branch
from
August 11, 2026 06:46
ca769eb to
d7fdb03
Compare
Running the tests without TESTINFRA=1 is supposed to fall back to testcontainers. It had stopped working: of nine packages that need infrastructure, six failed before running a single assertion. None of it was visible day to day, because the failures are all in code the TESTINFRA=1 path never executes. Pin the images. The fallback asked for localstack/localstack:latest, which now resolves to a licensed build that exits during startup, and for postgres:latest and clickhouse-server:latest, which have drifted past what the code assumes. Tags live in .env.test, which compose reads via --env-file and testinfra reads via viper, so the two ways of providing infrastructure run the same versions instead of drifting apart. Fix the Kafka container. Its host port was mapped with a syntax testcontainers rejects as of v0.42, so the package had been unrunnable across two dependency bumps; bind the port through HostConfigModifier instead, and take one the OS reports free rather than hardcoding 19092, which two test binaries would otherwise contend for. The broker also needs a JAAS file, and needs its controller listener on the address the image's quorum voters name — without that it starts, fails to register, and shuts down again with the port already open. Keep shared containers for the life of the process. They were terminated when the first suite in a binary finished, by which point the sync.Once guarding each one had already fired, so every later suite in that binary connected to an endpoint with nothing behind it. The testcontainers reaper already removes them at exit. Gate every Ensure on readiness, whoever started the service. Neither `compose up -d` returning nor a wait strategy seeing an open port proves a service will complete a handshake, and both failure modes surface inside a test rather than at startup. Probes now speak the protocol: authenticate to Kafka, connect to Postgres, query ClickHouse. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Test-only changes. Four suites failed on state they did not create, or could not start at all.
1. S3 — parallel subtests shared a map
Subtests in
TestAWSS3Destination_Validateall runt.Parallel()and share oneConfigmap. The storage-class subtest wroteINVALIDinto it, which could fail the valid-destination case. Each subtest now gets its own map.2. Kinesis — the consumer assumed deliver-once
A stream is an at-least-once log.
GetRecordscan return a record it already returned — seen against localstack with no error reported and the iterator advanced normally. Every test in a provider suite reads one channel, so a single redelivered record shifted every later read by one and each test then verified another test's event: a missingmessage_idpanicked the concurrent tests, a stale event mismatchedevent-idin the sequential one.3. Readiness —
Ensure*returned endpoints uncheckedNeither
docker compose up -dreturning nor a wait strategy seeing an open port proves a service will complete a handshake, and both surface inside a test rather than at startup — seen as a RabbitMQ connection reset inSetupSuite, and as a Kafka SASL EOF.Every
Ensure*now waits up to 30s before the first test, speaking the protocol rather than dialing the port: authenticate to Kafka, connect to Postgres, query ClickHouse, AMQP dial for RabbitMQ, health endpoint for LocalStack.4. Running without
TESTINFRA=1was brokenWithout
TESTINFRA=1the tests start their own containers. That path had rotted — six of nine infrastructure-dependent packages failed before a single assertion. All four causes sit in code theTESTINFRA=1path never executes, so nothing surfaced them:localstack:latesthad become a licensed build that exits during startup;postgres:latestandclickhouse:latesthad drifted past what the code assumes. Tags are now pinned in.env.test, which compose reads via--env-fileand the tests read via viper, so both ways of providing infrastructure run the same versions. The compose stack was pinned only loosely (:3,16-alpine) with one floating tag; those are exact now too.HostConfigModifieron an OS-assigned port rather than a hardcoded19092that two test binaries would contend for.sync.Onceguarding each had already fired, so every later suite in that binary connected to an endpoint with nothing behind it. They now live for the process; the testcontainers reaper removes them at exit.Note for reviewers:
make up/testandmake down/testnow pass--env-file .env.test. Runningdocker-compose -f build/test/compose.yml up -ddirectly fails with a message pointing at the Makefile, rather than silently substituting an empty image name.Verification
Kinesis suite, cold start (
docker compose restart test-awsimmediately before each run):Per-package, both paths:
TESTINFRATESTINFRA=1cmd/e2e)S3 fix verified with
-count=20and-race.go vet ./...and the-shortsuite CI runs are clean.Not addressed:
mqinfra'sshould_create_dlq_queuewaits for 6 SQS redeliveries inside a fixed 10s budget and fails under load on both paths (1 failure, then 3/3 clean). Pre-existing, and a property of the test's design rather than of the infrastructure.🤖 Generated with Claude Code