Refactor tests to use sqlx::test and improve test isolation - #3
Merged
Conversation
`sqlxmq_stress` has not compiled since the sqlx 0.9 bump: it still pinned sqlx 0.8, so its `Pool<Postgres>` was a different type from the one `sqlxmq` expects and none of the executor bounds resolved. CI did not notice because `cargo check` and `cargo clippy --all-targets` at a root that is both a package and a workspace only build the root package. Both now run with `--workspace`, as does `cargo fmt`. The stress crate also carried its own copy of the migrations, which had drifted to three of the seven currently in the repository, so it was exercising a schema several fixes behind. Replace the copy with a symlink to the canonical directory. Verified by running the harness end to end against a fresh database: 10000 jobs, ~649 jobs/s. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CmYdXrFbwtMHHHm1tREDUJ
The pure helpers in this crate had no coverage at all: every test needed a live PostgreSQL connection, so nothing exercised the plain arithmetic. `to_duration` converts a `PgInterval` into the sleep duration the runner waits before its next poll. Cover the month/day/microsecond components and, in particular, the negative case: `Duration` cannot represent a negative value, so an overdue `attempt_at` must collapse to zero and be polled immediately rather than waited on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CmYdXrFbwtMHHHm1tREDUJ
`should_retry` tells callers which database errors are worth retrying, but nothing exercised it. The cases it distinguishes are easy to get wrong and expensive when wrong: a constraint violation on one of the two ordered-channel constraints is a lost race and should be retried, whereas the same SQLSTATE raised by the caller's own schema is a genuine conflict that will just recur. The errors are built from a stub `DatabaseError`, so these run without a database. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CmYdXrFbwtMHHHm1tREDUJ
The SKIP LOCKED change in #2 exists so that concurrent pollers claim disjoint sets of messages instead of blocking on each other's row locks, but nothing tested it: no test ever started more than one runner. Two tests, at different levels: `it_polls_disjoint_messages_from_concurrent_transactions` calls mq_poll from two explicit transactions so the first still holds its row locks when the second runs. It asserts the second poll returns promptly, that the two claims are disjoint, and that together they cover every spawned job. Confirmed to fail against the pre-SKIP-LOCKED mq_poll, where the second poll blocks until the test's timeout fires. `it_delivers_each_job_exactly_once_with_concurrent_runners` covers the same ground end to end: four runners with deliberately low concurrency limits, jobs spread over several channels, asserting exactly-once delivery and an empty queue afterwards. It does not single out SKIP LOCKED - it guards the delivery contract the locking strategy exists to uphold, so it stays valid if that strategy changes again. Both are event-driven rather than sleep-based, and complete in about a second. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CmYdXrFbwtMHHHm1tREDUJ
Two changes to the same harness. Isolation: replace the hand-rolled pool with `#[sqlx::test]`, which gives each test a fresh database with the migrations already applied. That removes three problems at once. The global `TEST_MUTEX` is gone, so tests run in parallel instead of queueing for one shared database. The `TRUNCATE mq_payloads` / `DELETE FROM mq_msgs` reset is gone, and with it the risk of `cargo test` wiping the job tables of whatever database an ambient `DATABASE_URL` happened to point at - `dotenvy::dotenv()` does not override an already-set variable, so `.env` was not the safeguard it looked like. And migrations are now exercised by every test run rather than applied out of band, so CI no longer builds sqlx-cli: the workflow just creates an empty database. Synchronisation: the tests asserted on a counter after sleeping a fixed interval, which made them a race between the runner and the clock - `it_can_checkpoint_jobs` lost that race consistently on a slower machine, because the automatic keep-alive pushes `attempt_at` out from under a test that assumes a retry lands exactly one backoff after the attempt. Runners now hand each dispatched job to the test, which waits for the events it cares about. Where the old tests waited to see whether something further would happen, the new ones assert on the state that makes it impossible: a cleared channel has no rows left, an exhausted message has `attempt_at IS NULL` and can never be polled again, and a completed one is deleted. Remaining timeouts are hang detectors set far above any real duration, not timing assumptions. The assertions got stronger in the process. The ordered-batch test now checks the chain in `mq_msgs` directly, which is what the ordinality migration actually changed; the checkpoint test checks that the payload was replaced rather than only counting attempts; and `it_can_use_registry`, which previously asserted nothing at all, now confirms all three jobs ran by having them report through a context. No `sleep` remains in the test module. The suite is green 15 runs in a row and 5 more under 2x CPU oversubscription, in about 4 seconds against 11.5 locally and roughly 37 seconds of sleeping on CI. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CmYdXrFbwtMHHHm1tREDUJ
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
2 participants
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.
Summary
This PR significantly refactors the test suite to use
#[sqlx::test]macro for better test isolation and removes the stress test migration files. The changes improve test reliability by giving each test its own freshly migrated database and eliminate shared state issues.Key Changes
#[sqlx::test]: Replaced manual database pool management and test synchronization with sqlx's built-in test macro, which automatically provides isolated databases for each testtest_pool()with direct pool parameter injectionJobStreamabstraction to capture dispatched jobs instead of using counters and sleepsconfigured_job_runner()for tests that need custom runner optionsqueue_len,after_message_id,attempt_state,stored_payload,retry_backoff) for direct state inspectionpause()calls and counter-based assertions with direct database queries and event streams, making tests deterministic and fastershould_retry(): Createdshould_retry_testsmodule with aStubDbErrorimplementation to test retry logic without database connectionsto_duration(): Added tests inrunner.rsfor the interval-to-duration conversion helpersqlxmq_stress/migrations/as they're now symlinked to the main migrations directory#[sqlx::test]which handles migrations automaticallymigratefeature for dev dependenciesNotable Implementation Details
JobStreampattern allows tests to drive job execution explicitly, enabling precise control over retry and ordering scenariosTIMEOUTconstant (30 seconds) serves as a hang detector rather than a timing assumption, preventing flaky failures on slow machineshttps://claude.ai/code/session_01CmYdXrFbwtMHHHm1tREDUJ