Add tests for the Bunny CDN domain API#1879
Conversation
Cover the existing bunnyCdnApi domain contract: DNS zone loading, subdomain availability, custom hostname setup with forced SSL, CNAME registration with certificate retry and cleanup, and DNS record deletion. Adds a tiny restoreStubsAfterEach test helper so each test restores its stubs cleanly.
📝 WalkthroughWalkthroughChangesThe PR adds BDD coverage for Bunny CDN DNS and domain operations, including request wiring, availability checks, custom-domain validation, subdomain registration, certificate retries, DNS cleanup, deletion, and labeled failures. It also adds a helper that restores registered stubs after each test. Bunny CDN domain tests
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/shared/bunny-cdn/domain.test.ts`:
- Around line 88-98: Extract reusable stub helpers for bunnyCdnApi.getDnsZone
and bunnyCdnApi.validateCustomDomain, following the existing stubPullZoneId and
stubAvailableSubdomain pattern. Replace each duplicated inline stub setup in the
affected tests with these helpers while preserving the existing response data
and test behavior.
- Around line 183-218: Update the failure-result assertion in the parameterized
“reports a setup failure” test to use toEqual with the expected { error, ok }
result shape instead of toMatchObject, matching the established assertion
pattern nearby.
🪄 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: 05f8dc0b-1dc3-44cd-82ca-f235783815f8
📒 Files selected for processing (2)
test/shared/bunny-cdn/domain.test.tstest/test-utils/mocks.ts
| test("builds and checks the full subdomain name", async () => { | ||
| stubs.push( | ||
| stub(bunnyCdnApi, "getDnsZone", () => | ||
| Promise.resolve({ | ||
| ok: true, | ||
| zone: zone([ | ||
| { Id: 1, Name: "used.tickets", Type: 2, Value: "target" }, | ||
| ]), | ||
| }), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Extract shared stub helpers for getDnsZone/validateCustomDomain to match existing pattern.
The file already extracts stubPullZoneId/stubAvailableSubdomain for repeated stub(bunnyCdnApi, ..., () => Promise.resolve(...)) setups, but the same pattern for getDnsZone (lines 90-98, 115-118) and validateCustomDomain (lines 223-225, 320-322) is duplicated inline instead of reusing a similar helper.
♻️ Proposed helper additions
+const stubDnsZone = (
+ stubs: Restorable[],
+ result: { ok: true; zone: ReturnType<typeof zone> } | { error: string; ok: false },
+): void => {
+ stubs.push(stub(bunnyCdnApi, "getDnsZone", () => Promise.resolve(result)));
+};
+
+const stubValidateCustomDomain = (
+ stubs: Restorable[],
+ result: { ok: true } | { error: string; ok: false } = { ok: true },
+): void => {
+ stubs.push(stub(bunnyCdnApi, "validateCustomDomain", () => Promise.resolve(result)));
+};As per coding guidelines, **/*.{ts,tsx} files should "Eliminate code duplication with shared helpers or currying; do not evade jscpd with structural changes or jscpd:ignore, except for import blocks or unavoidable boilerplate."
Also applies to: 113-123, 220-226, 320-322
🤖 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/shared/bunny-cdn/domain.test.ts` around lines 88 - 98, Extract reusable
stub helpers for bunnyCdnApi.getDnsZone and bunnyCdnApi.validateCustomDomain,
following the existing stubPullZoneId and stubAvailableSubdomain pattern.
Replace each duplicated inline stub setup in the affected tests with these
helpers while preserving the existing response data and test behavior.
Source: Coding guidelines
| for (const [label, responses, expected] of [ | ||
| [ | ||
| "hostname", | ||
| [new Response("add failed", { status: 500 })], | ||
| "Add hostname failed (500): add failed", | ||
| ], | ||
| [ | ||
| "certificate", | ||
| [ | ||
| new Response(null, { status: 204 }), | ||
| new Response("cert failed", { status: 500 }), | ||
| ], | ||
| "Load free certificate failed (500): cert failed", | ||
| ], | ||
| [ | ||
| "forced SSL", | ||
| [ | ||
| new Response(null, { status: 204 }), | ||
| new Response(null, { status: 204 }), | ||
| new Response("ssl failed", { status: 500 }), | ||
| ], | ||
| "Set force SSL failed (500): ssl failed", | ||
| ], | ||
| ] as const) { | ||
| test(`reports a ${label} setup failure`, async () => { | ||
| stubPullZoneId(stubs); | ||
| using _fetch = stubFetch(responses[0], ...responses.slice(1)); | ||
|
|
||
| const result = await bunnyCdnApi.validateCustomDomain( | ||
| "custom.example.test", | ||
| ); | ||
|
|
||
| expect(result).toMatchObject({ error: expected, ok: false }); | ||
| expect(errors.contains(expected)).toBe(true); | ||
| }); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Prefer toEqual over toMatchObject for the failure-result assertion.
Line 215 uses toMatchObject({ error: expected, ok: false }), which passes even if the result contains extra unexpected fields. The equivalent check at line 179 for the same result shape uses toEqual, so this is a weaker, less mutation-resistant assertion than the established pattern in this file.
🐛 Proposed fix
- expect(result).toMatchObject({ error: expected, ok: false });
+ expect(result).toEqual({ error: expected, ok: false });As per coding guidelines, test/**/*.ts files should "use strong mutation-resistant assertions."
📝 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.
| for (const [label, responses, expected] of [ | |
| [ | |
| "hostname", | |
| [new Response("add failed", { status: 500 })], | |
| "Add hostname failed (500): add failed", | |
| ], | |
| [ | |
| "certificate", | |
| [ | |
| new Response(null, { status: 204 }), | |
| new Response("cert failed", { status: 500 }), | |
| ], | |
| "Load free certificate failed (500): cert failed", | |
| ], | |
| [ | |
| "forced SSL", | |
| [ | |
| new Response(null, { status: 204 }), | |
| new Response(null, { status: 204 }), | |
| new Response("ssl failed", { status: 500 }), | |
| ], | |
| "Set force SSL failed (500): ssl failed", | |
| ], | |
| ] as const) { | |
| test(`reports a ${label} setup failure`, async () => { | |
| stubPullZoneId(stubs); | |
| using _fetch = stubFetch(responses[0], ...responses.slice(1)); | |
| const result = await bunnyCdnApi.validateCustomDomain( | |
| "custom.example.test", | |
| ); | |
| expect(result).toMatchObject({ error: expected, ok: false }); | |
| expect(errors.contains(expected)).toBe(true); | |
| }); | |
| } | |
| for (const [label, responses, expected] of [ | |
| [ | |
| "hostname", | |
| [new Response("add failed", { status: 500 })], | |
| "Add hostname failed (500): add failed", | |
| ], | |
| [ | |
| "certificate", | |
| [ | |
| new Response(null, { status: 204 }), | |
| new Response("cert failed", { status: 500 }), | |
| ], | |
| "Load free certificate failed (500): cert failed", | |
| ], | |
| [ | |
| "forced SSL", | |
| [ | |
| new Response(null, { status: 204 }), | |
| new Response(null, { status: 204 }), | |
| new Response("ssl failed", { status: 500 }), | |
| ], | |
| "Set force SSL failed (500): ssl failed", | |
| ], | |
| ] as const) { | |
| test(`reports a ${label} setup failure`, async () => { | |
| stubPullZoneId(stubs); | |
| using _fetch = stubFetch(responses[0], ...responses.slice(1)); | |
| const result = await bunnyCdnApi.validateCustomDomain( | |
| "custom.example.test", | |
| ); | |
| expect(result).toEqual({ error: expected, ok: false }); | |
| expect(errors.contains(expected)).toBe(true); | |
| }); | |
| } |
🤖 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/shared/bunny-cdn/domain.test.ts` around lines 183 - 218, Update the
failure-result assertion in the parameterized “reports a setup failure” test to
use toEqual with the expected { error, ok } result shape instead of
toMatchObject, matching the established assertion pattern nearby.
Source: Coding guidelines
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2400c700fb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| test("continues when the custom hostname is already registered", async () => { | ||
| stubPullZoneId(stubs); | ||
| using _fetch = stubCustomDomainRequests( |
There was a problem hiding this comment.
Because this fetch stub handle is discarded, the test only checks { ok: true }; if validateCustomDomain started treating pullzone.hostname_already_registered as a terminal success and skipped the certificate and forced-SSL requests, this test would still pass because the queued responses do not have to be consumed. Keep the stub handle and assert the three provider calls so the "continues" behavior is actually locked in.
Useful? React with 👍 / 👎.
| ); | ||
| using _fetch = stubFetch(new Response("not json")); | ||
|
|
||
| await bunnyCdnApi.registerBunnySubdomain("child"); |
There was a problem hiding this comment.
Since the result is ignored here, this test still passes if registerBunnySubdomain starts returning success, or skips validateCustomDomain, whenever Bunny's DNS response has no parseable record id; deleted would remain empty either way. Assert the returned { error: "certificate pending", ok: false } before checking no cleanup so this case stays tied to the failing certificate path.
Useful? React with 👍 / 👎.
| first = noContent(), | ||
| ): ReturnType<typeof stubFetch> => stubFetch(first, noContent(), noContent()); | ||
|
|
||
| describeWithEnv("Bunny domain API", { env: ENV }, () => { |
There was a problem hiding this comment.
This new suite largely retests the same Bunny domain behavior already covered in test/lib/bunny-cdn/domain.test.ts (for example the DNS-zone, availability, custom-hostname, CNAME, retry, and cleanup paths), so every future domain change now has to keep two parallel test files in sync instead of one shared set of cases. Please move or refine the existing coverage under the mirrored test/shared/bunny-cdn/ location rather than adding a second suite beside it.
Useful? React with 👍 / 👎.
|
|
||
| export const restoreStubsAfterEach = (stubs: { restore(): void }[]): void => { | ||
| afterEach(() => { | ||
| for (const active of stubs.splice(0)) active.restore(); |
There was a problem hiding this comment.
Because splice(0) empties the tracking array before any restore runs, a single restore() failure (for example from a stub that was already manually restored) aborts the loop and leaves the later stubs installed with no way for a later afterEach to find them. That can make following tests run under stale API replacements, so attempt every queued restore before rethrowing or remove entries only after each restore succeeds.
Useful? React with 👍 / 👎.
|
Closing as superseded: #1862 merged the same domain test and helper into main while this extraction was under review. No separate merge is needed. |
1 participant
Adds
test/shared/bunny-cdn/domain.test.ts, covering the existingbunnyCdnApidomain contract onmainend-to-end:buildSubdomainRecordName), including the early-out when the zone lookup itself fails.Each test stubs
fetchand the surroundingbunnyCdnApimethods, then asserts the exact URL, method, body, and headers sent to Bunny, plus the labelled error result the caller sees. This locks the provider request shape and the error labelling in place.The tested APIs already exist on
main; this PR adds no production code. It also adds a tinyrestoreStubsAfterEachhelper totest/test-utils/mocks.tsso each test in the suite can collect its stubs in an array and have them restored in one place — the test uses it.Scope
test/shared/bunny-cdn/domain.test.ts(352 lines).restoreStubsAfterEachintest/test-utils/mocks.ts(6 lines).Verification
nix develop -c deno task test:files test/shared/bunny-cdn/domain.test.ts— 19 tests pass.nix develop -c deno task precommit— passes.Summary by CodeRabbit