Skip to content

split riverdrivertest.go#1138

Open
bgentry wants to merge 1 commit intomasterfrom
bgentry/split-driver-test
Open

split riverdrivertest.go#1138
bgentry wants to merge 1 commit intomasterfrom
bgentry/split-driver-test

Conversation

@bgentry
Copy link
Contributor

@bgentry bgentry commented Jan 31, 2026

@brandur hit a point where I felt like this file is just too unwieldy. Instead of a giant 5000 line file, this splits them into smaller files grouped by entity. For job queries where we have many tests, break them into files by query type.

Let me know if you think this would be an improvement broadly. It's easy to tweak as it's a mostly AI-driven refactor. FWIW I also spun up a separate session and had it cross check every removed line to verify it was moved without being altered (other than setup details)—all good there. ✅

@bgentry bgentry requested a review from brandur January 31, 2026 21:21
@bgentry bgentry force-pushed the bgentry/split-driver-test branch 2 times, most recently from 5dad2d4 to f64e602 Compare January 31, 2026 21:51
Comment on lines +106 to +118
t.Run("IndexThatDoesNotExistIgnore", func(t *testing.T) {
t.Parallel()

// Postgres runs the drop with `CONCURRENTLY` so this must use a full
// schema rather than a transaction block.
driver, schema := driverWithSchema(ctx, t, nil)

err := driver.GetExecutor().IndexDropIfExists(ctx, &riverdriver.IndexDropIfExistsParams{
Index: "does_not_exist",
Schema: schema,
})
require.NoError(t, err)
})
Copy link
Contributor Author

@bgentry bgentry Jan 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brandur hmm, this test was actually just an empty block before the split and Codex attempted to fix it. However it can't work in a tx as the other tests do because you can't run DROP INDEX CONCURRENTLY inside a transaction.

I filled it out using the same pattern as the above test to grab a full schema instead of txn.

Instead of a giant 5000 line file, split into smaller files grouped by
entity. For job queries where we have many tests, break them into files
by query type.
@bgentry bgentry force-pushed the bgentry/split-driver-test branch from f64e602 to ebfd00d Compare February 8, 2026 05:38
@bgentry
Copy link
Contributor Author

bgentry commented Feb 8, 2026

@brandur after rebasing post #1140 I wanted to make sure we kept that coverage and did another analysis, this time with Opus 4.6. It found one minor issue that was fixed but otherwise 👍

Final Report: riverdrivertest.go Split Review

Branch: bgentry/split-driver-test (1 commit: f2461ef)

Result: No coverage lost. One issue found and fixed.

The split of the 4,898-line riverdrivertest.go into 12 files is clean and faithful:

  • 75/75 top-level test blocks present in both master and HEAD
  • 196/196 unique t.Run test names preserved (plus 23 net-new tests in driver_client_test.go)
  • 63/75 blocks are byte-for-byte identical
  • 11 blocks differ only cosmetically (clientIDtestClientID rename, exec, _ :=exec := setup simplification)
  • 1 block (IndexDropIfExists/IndexThatDoesNotExistIgnore) gained new coverage — previously an empty stub, now has a real test
  • go build, go vet, go test, and golangci-lint all pass cleanly

Issue found and fixed

The Begin/NestedTransactions test in executor_tx.go was missing the final 3 assertions that verified rolling back tx1 makes job1 invisible from the parent executor. These have been restored and tests pass.

Unrelated change

internal/jobexecutor/job_executor_test.go:858 — a one-line relaxation of a stack trace path assertion (dropped the river/ module prefix).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant