split riverdrivertest.go

[bgentry]

@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]

@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.

[bgentry]

@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 (clientID → testClientID 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).

[brandur]

Cool. Definitely nice to have the AI review as it'd be really hard to tell from the diff here if something was lost in the refactor.

I'm up for trying it! My only small concern is that you now have job-related operations broken into files that aren't necessarily suggestive from their name (i.e. you'd have to think for a bit to see that something like JobCount would be found in job_read.go). We should maybe look at an alternate scheme if that gets annoying.

[bgentry]

Yeah, I'm open to any structure. I was compelled to split job queries beyond one file because they're basically half the overall line count. I don't have any preference on exact names or dividing lines though.

I feel pretty good about it after multiple agents reviewing via different models and techniques. Agree it would have to be done in small obvious pieces otherwise.