Tests share one database and global settings, so packages must run one at a time #192
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: wiki-cafe/member-console#192
Reference in New Issue
Block a user
make testruns packages one at a time because tests commit rows others read. Tests assert on subsets, filter out others' rows, and change the personal org type's default ladder and restore it. About twenty test files set viper, because production reads it directly. #33 is one symptom.Done when: each test runs in its own transaction, or in a database cloned from a migrated template when it must commit; settings are passed in; and
make testruns packages in parallel.Every test that uses the shared test databases now reaches them through
internal/testkit/dbtest:Open(orOpenE2Efor the plan-management test), andDSNwhere a test needs the address itself.Openrefuses a database whose core ledger is behind, and no test migrates the shared databases, sotest/reset-test-db.shis their only migrator. A cloned database or a per-test transaction would change whatOpenandDSNreturn, not the tests.Two tests open a second connection from
DSN, and each needs it to reach the databaseOpenreturned:TestInvoiceIssuedPaymentPaidBackfill, whose subject is migration 00022, writes its account row throughOpen. Then, over a second, simple-protocol connection (DSNwithConnect) and inside a transaction it rolls back, it dropsissued_atandpaid_atand replays 00022's Up section on the shared database. That transaction holds ACCESS EXCLUSIVE oncore.invoicesandcore.paymentsuntil the rollback.TestRunValidateConfig_DBseeds a config override row throughOpenand passesDSNto validate-config, whose own code opens a pool from it and must read that row.So
OpenandDSNhave to return the same clone, and a per-test transaction would hide the seeded rows from each test's second connection; that option has to handle both tests.One more test changes the shared schema while it runs.
TestPoolAssignments_DedupPreflightDemotesDuplicate, whose subject is migration 00010, drops the unique indexcore.uq_pool_assignments_one_primary_per_workspaceoutside any transaction, runs its copy of 00010's pre-flight statement, and recreates the index (its cleanup recreates it again if it is missing). It uses onlyOpen, so nothing changes forDSN. But a package running in parallel on the same database would see the index gone, and under a per-test transaction itsDROP INDEXwould hold ACCESS EXCLUSIVE oncore.pool_assignmentsuntil the rollback. The other tests that apply a migration do so on their own scratch databases.One sentence of the test-database-isolation spec is stale as a result. Its reset requirement says: "Migrating as part of the reset is load-bearing: it leaves every parallel test package's own migration call a no-op, so concurrent packages do not race to create the same objects." No package migrates the shared test databases any more. The next change to that capability rewrites it.
Four tests in internal/fulfillment change a shared row and do not put it back:
TestReconcile_TheFloorCanTakeTheVacatedLadderFirst(grant_resumption_test.go, through itssetPersonalDefaulthelper),TestReconcile_CancellationGuardRespectsGrantHolderandTestReconcile_CancellationDefersToPlanOnNonDefaultLadder(reconcile_conferral_test.go), andTestCancel_ReissuesOrgDefault(reconcile_integration_test.go). Each sets the default plan ladder of thepersonalorganization type, which every package's tests share, and its cleanup clears it to NULL whatever it held before. A package that runs beside them, or after them and expects the seeded default, reads a default it did not set. The plan webhook fixture in internal/integrations/stripe/workflows no longer writes that row.What sharing one database has cost so far: during #166, rows that other tests left changed a test's result at least fifteen times. The ones a contributor can find:
Tests narrowed to their own rows.
TestNewHandlerPagesrecords only the status and location of/partials/member/plansand/partials/member/addons(catalogListRoutes,internal/server/new_handler_db_test.go:80-86). Their bodies list every active ladder and add-on in the database, and the golden failed in a full package run.wholeFeed(internal/server/operator_activity_golden_db_test.go:61-64) reads the whole activity timeline. With a 400-event window,TestActivityFeedPlacesBillingEventsWhenTheyHappenedandTestRecentActivityTiesListTheLaterWrittenFirstpassed alone and failed in the package run, because other tests leave newer committed rows.registerGoldenKeys,internal/server/member_plans_golden_db_test.go:77-80), because the name shown forfedwiki_sitesdepended on which provider an earlier test had registered.Deliberate breaks dropped because their result depended on run order. A deliberate break edits the code to check that a test catches the change.
TestGetPlansLadderAwarecaught "an unmapped price offers no enabled Checkout" only afterTestSwitchRefusesADraftTierThroughTheHandlerhad committed a priced ladder.TestDesiredStateOutboxQueries_DB(internal/integration/delivery_queries_db_test.go:226-228) catches a break that makes the backlog's oldest age negative only when another test has left an older pending row committed. The rows it writes are as old as its own transaction, so their age is zero either way.A test that fails on its second run.
TestGetStripeIntegrationPageDeliveryQueue(internal/server/operator_integrations_stripe_test.go:202) commits a dead-lettered outbox row and webhook event and checks that the page reads "1 event could not be processed". Run again without a reset, it finds the first run's rows too and fails. This happened four times.Tests that act on everyone's rows. The relay tests in
internal/workflows/desiredstaterunProcessDesiredStateOutbox, whose claim (ClaimDesiredStateOutbox) takes any pending row in the database, so they process rows other tests left, and before-and-after comparisons had to reset the database before each side. The four fulfillment tests in the comment above change a default that every package reads.Tests that depend on another package. The eight tests #33 names passed only after another package had registered the providers. A registration that an earlier run leaves stays until the next reset, so only a reset followed by running the package alone shows that a fixture needs it.
The test-data builders commit by design.
worldwrites through the console's own functions and deletes nothing it writes (internal/testkit/world/world.go:12-15), so every test that builds on the shared database with it adds rows the other tests see. MovingTestGetPlansLadderAwareandTestSwitchRefusesADraftTierThroughTheHandleronto it turned two fixtures that ran in a rolled-back transaction into committed rows (f6b81987).The rest were reruns after a reset. The standing cost is
-p 1, a reset before each side of every comparison, and goldens that read only their own fixture's rows.Three FedWiki passes act on every row of the shared test database: settle, the domain reconciliation at boot, and the recount of every pool that now ends each sync and runs at boot. Their tests hold only under
-p 1; the fake Temporal clients in the FedWiki tests answer "running" for runs they do not name, so settle leaves other tests' requests alone. The recount grows with every run that is not followed by a reset: with about 1,800 accumulated usage rows, a sync inside a Temporal test environment passed the environment's three-second default timeout, so the FedWiki workflow tests now give it 30 seconds.