Split the parents-gate test monolith into themed files - #1684
Conversation
📝 WalkthroughWalkthroughThis PR adds a new parents-gate test suite, shared test helpers, and refactors existing tests to use them. It also updates reservation checkout stubbing and adjusts biome overrides for the expanded test file set. ChangesParents-gate test suite and shared helpers
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/lib/server-parents-gate/helpers.ts`:
- Around line 110-116: The select extraction helper currently assumes a matching
name attribute always exists, so selectOptionsFromHtml can silently return a bad
slice when html.indexOf(selectName) is -1. Update selectOptionsFromHtml to
explicitly handle the missing match case, using a clear failure path or empty
result before slicing, so callers like expectSelectOffers get an immediate,
readable “select not found” signal instead of a misleading near-full-page
string.
In `@test/lib/server-parents-gate/render-compat-data.test.ts`:
- Around line 55-61: The inline datesAttr helper duplicates the same
HTML-parsing pattern already used in the existing helpers utilities, so keep the
test focused by extracting it only if it will be reused. If this per-parent
attribute lookup appears elsewhere, move the logic into helpers.ts alongside
selectOptionsFromHtml/selectOptionsHtml and have the test call that shared
helper instead of defining a local closure.
In `@test/lib/server-parents-gate/shared-and-pay-more.test.ts`:
- Around line 96-107: The assertion in the pay-more child folding test is too
weak because `toContain("£30")` can match larger or different totals. Update the
test in `shared-and-pay-more.test.ts` near `postCalculate` to assert the exact
rendered amount for the quote, using a stricter check around the `html` returned
by `postCalculate` so regressions like £300 or £30.99 will fail. Keep the focus
on the `a pay-more child's submitted price is folded into the order` test and
strengthen the expectation on the exact total rather than a substring.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 92c895e0-ecfc-4a6d-a87f-5487924af803
📒 Files selected for processing (20)
biome.jsontest/lib/server-booking-preserve.test.tstest/lib/server-parents-discovery.test.tstest/lib/server-parents-e2e.test.tstest/lib/server-parents-gate.test.tstest/lib/server-parents-gate/auto-fill.test.tstest/lib/server-parents-gate/customisable-fold.test.tstest/lib/server-parents-gate/daily-fold.test.tstest/lib/server-parents-gate/helpers.tstest/lib/server-parents-gate/qty-matching.test.tstest/lib/server-parents-gate/questions-thank-you.test.tstest/lib/server-parents-gate/render-capacity.test.tstest/lib/server-parents-gate/render-compat-data.test.tstest/lib/server-parents-gate/render-dates.test.tstest/lib/server-parents-gate/render-day-counts.test.tstest/lib/server-parents-gate/render-selector.test.tstest/lib/server-parents-gate/shared-and-pay-more.test.tstest/lib/server-reservation/helpers.tstest/shared/db/questions/helpers.tstest/test-utils/parents.ts
💤 Files with no reviewable changes (1)
- biome.json
…pe; bookableDates → shared bookableStartDates; makeCustomisableDailyParent shared
804fbb6 to
4bcbf0b
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/lib/server-parents-booking/api-book-payments.test.ts`:
- Around line 41-65: The local captureCheckoutIntent helper is duplicating the
shared Stripe checkout stub logic already extracted in the test helpers. Update
this test to use the common helper pattern from stubCheckout/stubCheckoutIntent
instead of re-stubbing stripePaymentProvider.createCheckoutSession here, and if
you still need call counts, extend the shared helper to expose calls() rather
than keeping a separate copy. Use the existing capture/restore behavior from the
shared helper to keep this test aligned with the centralized stub utilities.
In `@test/lib/server-parents-gate/customisable-fold.test.ts`:
- Around line 196-232: Split the combined assertion in customisable-fold.test.ts
into two isolated tests: one covering the rejected mismatch case and one
covering the accepted matching-span fold. Keep the shared setup via makeParent,
firstBookableDate, bookParent, expectRejectedBooking, and expectReserved, but
separate the negative path (day_count "1" with the 3-day child) from the
positive path (day_count "3" folding the child) so each test verifies one
behavior only.
In `@test/lib/server-parents-gate/daily-fold.test.ts`:
- Around line 29-67: Move the file-local setup helpers out of the test and into
the shared helpers module used by the other `#test-utils` fixtures.
`childExcludingParentDay` and `makeDailyGroupWithFiller` should be added to
`test/lib/server-parents-gate/helpers.ts` alongside the existing shared builders
like `firstBookableDate` and `makeDailyChildFilledOnDayA`, then import and use
them from `daily-fold.test.ts`. Keep the test file focused on assertions and
avoid defining new ad-hoc harness utilities inline.
- Around line 121-157: Split the combined scenario in daily-fold.test.ts into
two focused tests: one that verifies a parent booking on dayB succeeds and folds
the daily child correctly, and a separate one that verifies a parent booking on
dayA is rejected because the child is full. Keep the existing helpers and
assertions, but move the success checks around bookParent, expectReserved, and
getAttendeesRaw into a dayB-only test, and the 302/expectFlash rejection checks
plus parentRows validation into a dayA-only test.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e3d0cf88-57fa-4d69-849f-fb5fabd9d224
📒 Files selected for processing (21)
biome.jsontest/lib/server-booking-preserve.test.tstest/lib/server-parents-booking/api-book-payments.test.tstest/lib/server-parents-discovery.test.tstest/lib/server-parents-e2e.test.tstest/lib/server-parents-gate.test.tstest/lib/server-parents-gate/auto-fill.test.tstest/lib/server-parents-gate/customisable-fold.test.tstest/lib/server-parents-gate/daily-fold.test.tstest/lib/server-parents-gate/helpers.tstest/lib/server-parents-gate/qty-matching.test.tstest/lib/server-parents-gate/questions-thank-you.test.tstest/lib/server-parents-gate/render-capacity.test.tstest/lib/server-parents-gate/render-compat-data.test.tstest/lib/server-parents-gate/render-dates.test.tstest/lib/server-parents-gate/render-day-counts.test.tstest/lib/server-parents-gate/render-selector.test.tstest/lib/server-parents-gate/shared-and-pay-more.test.tstest/lib/server-reservation/helpers.tstest/shared/db/questions/helpers.tstest/test-utils/parents.ts
💤 Files with no reviewable changes (1)
- biome.json
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/lib/server-parents-booking/api-book-payments.test.ts`:
- Around line 41-65: The local captureCheckoutIntent helper is duplicating the
shared Stripe checkout stub logic already extracted in the test helpers. Update
this test to use the common helper pattern from stubCheckout/stubCheckoutIntent
instead of re-stubbing stripePaymentProvider.createCheckoutSession here, and if
you still need call counts, extend the shared helper to expose calls() rather
than keeping a separate copy. Use the existing capture/restore behavior from the
shared helper to keep this test aligned with the centralized stub utilities.
In `@test/lib/server-parents-gate/customisable-fold.test.ts`:
- Around line 196-232: Split the combined assertion in customisable-fold.test.ts
into two isolated tests: one covering the rejected mismatch case and one
covering the accepted matching-span fold. Keep the shared setup via makeParent,
firstBookableDate, bookParent, expectRejectedBooking, and expectReserved, but
separate the negative path (day_count "1" with the 3-day child) from the
positive path (day_count "3" folding the child) so each test verifies one
behavior only.
In `@test/lib/server-parents-gate/daily-fold.test.ts`:
- Around line 29-67: Move the file-local setup helpers out of the test and into
the shared helpers module used by the other `#test-utils` fixtures.
`childExcludingParentDay` and `makeDailyGroupWithFiller` should be added to
`test/lib/server-parents-gate/helpers.ts` alongside the existing shared builders
like `firstBookableDate` and `makeDailyChildFilledOnDayA`, then import and use
them from `daily-fold.test.ts`. Keep the test file focused on assertions and
avoid defining new ad-hoc harness utilities inline.
- Around line 121-157: Split the combined scenario in daily-fold.test.ts into
two focused tests: one that verifies a parent booking on dayB succeeds and folds
the daily child correctly, and a separate one that verifies a parent booking on
dayA is rejected because the child is full. Keep the existing helpers and
assertions, but move the success checks around bookParent, expectReserved, and
getAttendeesRaw into a dayB-only test, and the 302/expectFlash rejection checks
plus parentRows validation into a dayA-only test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e3d0cf88-57fa-4d69-849f-fb5fabd9d224
📒 Files selected for processing (21)
biome.jsontest/lib/server-booking-preserve.test.tstest/lib/server-parents-booking/api-book-payments.test.tstest/lib/server-parents-discovery.test.tstest/lib/server-parents-e2e.test.tstest/lib/server-parents-gate.test.tstest/lib/server-parents-gate/auto-fill.test.tstest/lib/server-parents-gate/customisable-fold.test.tstest/lib/server-parents-gate/daily-fold.test.tstest/lib/server-parents-gate/helpers.tstest/lib/server-parents-gate/qty-matching.test.tstest/lib/server-parents-gate/questions-thank-you.test.tstest/lib/server-parents-gate/render-capacity.test.tstest/lib/server-parents-gate/render-compat-data.test.tstest/lib/server-parents-gate/render-dates.test.tstest/lib/server-parents-gate/render-day-counts.test.tstest/lib/server-parents-gate/render-selector.test.tstest/lib/server-parents-gate/shared-and-pay-more.test.tstest/lib/server-reservation/helpers.tstest/shared/db/questions/helpers.tstest/test-utils/parents.ts
💤 Files with no reviewable changes (1)
- biome.json
🛑 Comments failed to post (4)
test/lib/server-parents-booking/api-book-payments.test.ts (1)
41-65: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Consolidate this local
captureCheckoutIntentwith the newly-extracted shared stub helpers.This file re-implements the same "stub
createCheckoutSession, capture the intent, return a restore handle" pattern thattest/lib/server-reservation/helpers.ts:stubCheckoutandtest/lib/server-parents-gate/helpers.ts:stubCheckoutIntentwere just extracted to cover. The PR's own stated goal for this cohort is to eliminate exactly this kind of repeated stub/capture logic (the parents-gate helpers doc even calls out "the same Stripe checkout-stub capture" as a prior jscpd clone). Keeping a third near-identical copy here re-introduces the duplication this PR is otherwise removing; consider extending the shared helper (e.g. to also exposecalls()) instead of a local reimplementation.♻️ Possible direction
-const captureCheckoutIntent = ( - sessionId: string, -): { - capturedIntent: () => CheckoutIntent | undefined; - calls: () => number; - restore: () => void; -} => { - let captured: CheckoutIntent | undefined; - const mock = stub( - stripePaymentProvider, - "createCheckoutSession", - (intent: CheckoutIntent) => { - captured = intent; - return Promise.resolve({ - checkoutUrl: "https://stripe.test/checkout", - sessionId, - }); - }, - ); - return { - calls: () => mock.calls.length, - capturedIntent: () => captured, - restore: () => mock.restore(), - }; -}; +// Reuse stubCheckout(sessionId) from server-reservation/helpers.ts (or +// stubCheckoutIntent from server-parents-gate/helpers.ts) and derive +// `calls()` from `checkout.calls.length` at call sites instead of +// re-declaring the stub here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/lib/server-parents-booking/api-book-payments.test.ts` around lines 41 - 65, The local captureCheckoutIntent helper is duplicating the shared Stripe checkout stub logic already extracted in the test helpers. Update this test to use the common helper pattern from stubCheckout/stubCheckoutIntent instead of re-stubbing stripePaymentProvider.createCheckoutSession here, and if you still need call counts, extend the shared helper to expose calls() rather than keeping a separate copy. Use the existing capture/restore behavior from the shared helper to keep this test aligned with the centralized stub utilities.test/lib/server-parents-gate/customisable-fold.test.ts (1)
196-232: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Split into two tests: reject-mismatch and accept-match.
This test asserts two independent behaviors (mismatched span rejected, matching span folds fine) under one
test(...)block. As per coding guidelines, "Tests must be non-tautological, behavior-focused, isolated, repeatable, and cover one thing per test."♻️ Suggested split
- test("a fixed daily child whose duration differs from the chosen span is rejected; the matching span folds", async () => { + test("a fixed daily child whose duration differs from the chosen span is rejected", async () => { const { parent, child } = await makeParent({ children: [{ daily: true, durationDays: 3 }], parent: { customisableDays: true, daily: true, dayPrices: { 1: 1000, 3: 3000 }, durationDays: 3, name: "Daily base", }, }); const date = await firstBookableDate(parent.id); const rejected = await bookParent(parent.slug, { date, day_count: "1", ...parentField(parent, "1"), }); await expectRejectedBooking( rejected, parent.id, "Daily base has no available options right now.", ); + }); + + test("a fixed daily child whose duration matches the chosen span folds fine", async () => { + const { parent, child } = await makeParent({ + children: [{ daily: true, durationDays: 3 }], + parent: { + customisableDays: true, + daily: true, + dayPrices: { 1: 1000, 3: 3000 }, + durationDays: 3, + name: "Daily base", + }, + }); + + const date = await firstBookableDate(parent.id); const ok = await bookParent(parent.slug, { date, day_count: "3", ...parentField(parent, "1"), }); expectReserved(ok); expect((await getAttendeesRaw(child.id)).length).toBe(1); });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.test("a fixed daily child whose duration differs from the chosen span is rejected", async () => { // A customisable daily parent offering 1 or 3 days, with a fixed 3-day // daily child. A 1-day booking can't fold the 3-day child (its span would // not match the parent's), so the parent is sold out; a 3-day booking // folds the child fine. const { parent, child } = await makeParent({ children: [{ daily: true, durationDays: 3 }], parent: { customisableDays: true, daily: true, dayPrices: { 1: 1000, 3: 3000 }, durationDays: 3, name: "Daily base", }, }); const date = await firstBookableDate(parent.id); const rejected = await bookParent(parent.slug, { date, day_count: "1", ...parentField(parent, "1"), }); await expectRejectedBooking( rejected, parent.id, "Daily base has no available options right now.", ); }); test("a fixed daily child whose duration matches the chosen span folds fine", async () => { const { parent, child } = await makeParent({ children: [{ daily: true, durationDays: 3 }], parent: { customisableDays: true, daily: true, dayPrices: { 1: 1000, 3: 3000 }, durationDays: 3, name: "Daily base", }, }); const date = await firstBookableDate(parent.id); const ok = await bookParent(parent.slug, { date, day_count: "3", ...parentField(parent, "1"), }); expectReserved(ok); expect((await getAttendeesRaw(child.id)).length).toBe(1); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/lib/server-parents-gate/customisable-fold.test.ts` around lines 196 - 232, Split the combined assertion in customisable-fold.test.ts into two isolated tests: one covering the rejected mismatch case and one covering the accepted matching-span fold. Keep the shared setup via makeParent, firstBookableDate, bookParent, expectRejectedBooking, and expectReserved, but separate the negative path (day_count "1" with the 3-day child) from the positive path (day_count "3" folding the child) so each test verifies one behavior only.Source: Coding guidelines
test/lib/server-parents-gate/daily-fold.test.ts (2)
29-67: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move file-local helpers into the shared helpers module.
childExcludingParentDayandmakeDailyGroupWithFillerare ad-hoc test-harness helpers defined directly in this test file. As per coding guidelines, "Use helpers from#test-utilsinstead of defining local test harness utilities." Since this PR already introducestest/lib/server-parents-gate/helpers.tsfor exactly this purpose (it already suppliesfirstBookableDate,makeDailyChildFilledOnDayA, etc.), these two setup helpers should live there instead of inline in the test file, for consistency and reuse.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/lib/server-parents-gate/daily-fold.test.ts` around lines 29 - 67, Move the file-local setup helpers out of the test and into the shared helpers module used by the other `#test-utils` fixtures. `childExcludingParentDay` and `makeDailyGroupWithFiller` should be added to `test/lib/server-parents-gate/helpers.ts` alongside the existing shared builders like `firstBookableDate` and `makeDailyChildFilledOnDayA`, then import and use them from `daily-fold.test.ts`. Keep the test file focused on assertions and avoid defining new ad-hoc harness utilities inline.Source: Coding guidelines
121-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Split into two tests: day-B success and day-A rejection.
This single test asserts two independent behaviors (booking succeeds on the free date, is rejected on the full date). As per coding guidelines, tests should "cover one thing per test."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/lib/server-parents-gate/daily-fold.test.ts` around lines 121 - 157, Split the combined scenario in daily-fold.test.ts into two focused tests: one that verifies a parent booking on dayB succeeds and folds the daily child correctly, and a separate one that verifies a parent booking on dayA is rejected because the child is full. Keep the existing helpers and assertions, but move the success checks around bookParent, expectReserved, and getAttendeesRaw into a dayB-only test, and the 302/expectFlash rejection checks plus parentRows validation into a dayA-only test.Source: Coding guidelines
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
|
Review 2 (commit 4bcbf0b) reported "Actionable comments posted: 4" but GitHub failed to post the inline comments themselves — see the review body's 💤 Comments failed to post (4) block. Posting here for traceability, since those four items were already addressed in the prior commit a0dabef ("Address comments") before this reply pass:
Plus the three posted inline threads on review 1 are now addressed and resolved (see thread replies):
Nothing here requires work outside this PR's scope, so no TODO.md entries resulted. |
Resolved conflicts: - server-attendees.test.ts (+ server-parents-gate.test.ts): main split these monoliths into themed files (#1681, #1684); accepted main's split (deleted the monoliths) and repointed the new themed files' imports of the deleted #shared/db/questions.ts onto the split sub-modules (question-types, attendee-answers/{save,reads}, queries, tables). - server-booking-preserve.test.ts: main refactored to use the assignQuestion test helper, so dropped the now-unused direct setListingQuestions/ answersTable/questionsTable imports.
What changed
The
server-parents-gate.test.tsfile was a single 2,724-line test suite — one of the largest files still grandfathered in the BiomenoExcessiveLinesPerFileexclusion list. This PR splits it into 12 themed test files undertest/lib/server-parents-gate/(each under ~320 lines) plus a smallhelpers.tsfor the patterns shared within the suite.The split
The original file was one
describeWithEnvblock wrapping ~60 flat test calls. Each test was grouped by theme and moved verbatim into its own file, each with its owndescribeWithEnv:auto-fill.test.tsqty-matching.test.tsshared-and-pay-more.test.tscustomisable-fold.test.tsdaily-fold.test.tsquestions-thank-you.test.tsrender-selector.test.tsrender-dates.test.tsrender-day-counts.test.tsrender-capacity.test.tsrender-compat-data.test.tsbiome.jsonno longer excludesserver-parents-gate.test.tsfrom the 1,000-line rule.Shared helpers
Splitting a file this size routinely surfaces duplication that was silently passing inside the monolith (jscpd cannot fully scan very large files). The split surfaced 25 cpd clones; all were fixed by extracting shared mechanisms — never
jscpd:ignore:Added to
test/test-utils/parents.ts(shared across the broader parents suite — these also migrated duplicate helpers inserver-parents-e2e.test.ts,server-booking-preserve.test.ts, andserver-parents-discovery.test.ts):bookParent— posts a booking with the standard test contact (a@b.com/Ada), so the email/name pair is declared onceparentField/childField— build thequantity_*/child_qty_*form fieldsexpectRejectedBooking— the 302 + error flash + zero-rows assertion tailexpectFoldedLine/expectNoBooking— singular delegates to the existingexpectAttendeeCounts(one-or-many: a single item is an array of one)makeTwoDefaultChildren/makeRoomySharedChild/makeCustomisableDailyParent/bookOneOfEachFold— declarative scenario buildersAdded to
test/lib/server-parents-gate/helpers.ts(suite-local):firstBookableDate— delegates to the sharedbookableStartDatesfrom#test-utilsand picks the firstmakeDailyChildFilledOnDayA— the shared "1-cap daily child full on one date" setupstubCheckoutIntent— wraps the sharedstubCheckout(inserver-reservation/helpers.ts) withsetupStripe(); returns the same{ checkout, getCaptured }shape so one mechanism keeps one vocabularyrunContainsCases/selectOptionsFromHtml/expectSelectOffers/expectRendersSoldOut— table-driven render checksRelocated to
test/shared/db/questions/helpers.ts(where its siblings live):assignQuestion— composes the existingcreateQuestion+addAnswer+setListingQuestionstrio (also extendedaddAnswerwith an optionaloverridesparam for the deactivated-answer case)Reused existing helpers instead of creating new ones
The audit step found several cases where my initial helpers duplicated or aliased existing mechanisms. These were reconciled:
weekdayName→ deleted; callers now import the existingweekdayOffrombooking-model-fixtures.tsbookableDates→ deleted; callers now usebookableStartDatesfrom#test-utils(whichorigin/mainindependently extracted — the rebase surfaced this duplicate)stubCheckoutIntentoriginally renamed the accessors (getCaptured→getIntent); now returns the sharedstubCheckoutshape unchanged (no alias vocabulary)makeCustomisableDailyParentspec was promoted to#test-utilsand migrated intoserver-parents-booking/api-book-payments.test.ts(main had the same spec inlined)All existing duplicate helpers in the e2e test (
adaBook,parentField,childField,assertChoose1More) were deleted in favour of the shared ones — no internal compatibility layers, every caller migrated.Verification
deno task precommitpasses (typecheck including test files, lint, cpd at 0%, build, full test suite, mutation testing over changed files).