Make every test helper name say what it does at the call site, and fold the duplicate fixtures into their traits

Every test helper whose name did not say what it does now says it. 115 files, 1,450 insertions against 1,602 deletions. Nothing a test asserts changed, with the single exception noted under Found while reading.

Fixtures wearing a name a shared trait owns

36 of them. Seven were live: those classes use SingleBookingHoldTrait and redeclare one of its methods, so the trait's version silently never applied. One would have broken rather than misled:

AreaVenueTrait::createBookableSlot(Venue, Configuration, Grade, Section)
SlotBookingPlacementTest::createBookableSlot(Venue, Grade, ?Configuration)

Different arity with two arguments swapped, and the trait calls it from its own fixtures. That same class's createSeatedVenueAndSlot() built a venue, a grade, a section and places, and no slot at all.

Thirteen were a shared fixture written out again and are gone — PoolOpTest at 0.94 identical to the trait, ApiPlacementTest and VenuePlacementProviderTest at 0.96 to each other, and five copies of one tariff builder that turned out to be a single method once measured against each other rather than against the trait.

The rest keep their bodies, because they do not collapse. The venue-with-places family is 26 methods at a median pairwise similarity of 0.51 (9 of 325 pairs above 0.80); the mapped family is 10 at 0.42 (1 of 55). One builder for either would take a capacity, a place count, a section count, a grade count, a mode and a label. Three of them also measure query budgets, where a fixture shared with tests that measure nothing can move a number for a reason unrelated to the code.

Names that needed the signature to be understood

24 bare verbs with no object across 155 call sites — act(), reset(), take(), open(), build(), read(), hold(), check(). Nouns pretending to be calls: areas(), offers(), page(), basket(). Names their own return type contradicted: getQuantity() handing back a NodeElement, cacheablePage() a Response, getSlotState() the slot's offers. Adjectives standing in for entities: $open and $restricted holding a tariff, $booker holding a Response, and $autoResource sitting one line from $manualSettlementResource where manual meant two different things.

26 helpers returned a value with no declared return type. All 26 already carried it in a @return docblock, so this was promotion rather than judgement, and phpstan checked every one.

A transaction is not an order

The engine stores a transaction; an order is yoyaku_order's decoration of it. 24 helpers were named for the order while typed BookingTransactionInterface. The module boundary sorts them: a module that does not declare yoyaku_order has no order, so 18 say transaction now — including the core trait's reloadTransaction() and loadTransactionBookings() — and the six in modules that do declare it keep the word.

That frees the name where it belongs: OrderTest::reloadOrder() is honest again, asserting OrderInterface over the trait's transaction, and the wrapper that existed only because a core trait had taken the name is gone.

Two service ids nobody had named

PaymentPolicyResolverTrait and VenueMapBuilderTrait. The second id was written out 38 times across 15 files, twice behind a private getter and the rest through the container by hand.

Found while reading

#3619712 shipped a rename that was wrong. Its verb sweep renamed PoolOpTest::session() to createSlotWithTariffs() — a method that creates no slot, returns a Session, and wore a name three other classes use for a slot fixture. It is getRequestSession() here.

One assertion in the suite could not fail: CheckoutLockTest::testNonBookingRunIsIgnored() ended in assertTrue(TRUE) while its docblock claimed the run was left alone. Fixed here rather than filed, because the test was reporting coverage it did not have. The run is now given a bystander order it does not name, and the test asserts that order is still pending immediately after the opening event: a subscriber reaching for the current or the newest transaction rather than the one its own run names passes every other test in this class, and locks the bystander here. The placement matters, and it is where the first attempt at this went wrong. The opening and the closing are a round trip, as testClosingUnlocksTheOrder shows, so a wrong lookup locks the bystander on the opening and unlocks it again on the closing: an assertion made after both events sees pending either way and catches nothing. An assertNull on the correlator pins the premise, so a fixture that started binding a transaction fails outright rather than quietly turning this into a second pass over the ordinary path.

Proved by injecting that exact bug into CheckoutLockSubscriber::resolveOrder(), falling back to the newest transaction when the correlator returns none. With the assertion after both events the class passed 1/1; with it after the opening the class failed on this method alone.

Still wants its own issue: letting a scenario report every independent check it failed rather than only the first. OfferStepperTest has that mechanism privately and six sibling browser tests do not.

resetBeforeScenario() is not a collision

35 classes declare it and all 35 are right: ScenarioRunTrait ships an empty body and its docblock asks a test to override it.

Checked

phpcs and phpstan clean over the module root throughout. Kernel classes were run in batches as the work landed — 39/39, then 20/20, 23/23, 107/107, 13/13 and 1/1 — covering every touched class plus every user of each trait whose shared code changed. The runner warns it may fall back to the docroot, so the worktree was proved to be what ran by injecting a deliberate failure and watching it fire.

AI-Generated: Yes (Claude Code took the measurements, made these changes and drafted this description. I reviewed them before pushing.)