Repository navigation
Move low-value tests to their owner boundaries and drop test-only code - #84
Merged
Merged
Conversation
Delete tests that only kept test-only exports alive, duplicated stronger route or component proof, or asserted a negative that could not fail. Remove the production code whose only callers were those tests: jsonHeaders, supportsDocumentExtraction, outcomeForStatus, getRequestIdentity, parsePositiveInteger, and the unread cailIdentityJwt context value with its getter. Retarget the identity verification cases at resolveRequestIdentity with the same case count, and replace the app-leg storage test with coverage of the gateway leg the middleware actually forwards, including the different-subject rejection. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A test audit pass. It removes tests that only kept test-only exports alive, tests that duplicate stronger route or component proof, and one negative assertion that could not fail. It also removes the production code whose only callers were those tests. It adds no new product behavior.
Changes, grouped by what they removed
Dead production code with test-only callers
jsonHeaders(lib/http.ts) had no production caller since the March Workers migration. The wholelib/http.test.tsgoes with it.readFormDataandbinaryBodycases repeated route-level proof that stays in place:regressions.test.ts: "stores the exact bytes of a valid multipart upload"regressions.test.ts: "rejects malformed multipart framing without writing a file"regressions.test.ts: "SS-21: accepts a real PNG body on the thumbnail route and preserves its bytes"preview.test.ts: "preserves published HTML bytes when no mount rewrite is needed"supportsDocumentExtraction(lib/document.ts) had no caller. The agent callsextractDocumentText, and "rejects unsupported document types" still covers the PDF-only rule.outcomeForStatus(lib/logging.ts) was a one-line wrapper. Its test assertion now calls the owner,terminalForStatus(200).outcome.getRequestIdentity(lib/cail-identity.ts) was a test-only wrapper that returnednull. All 18 identity cases remain, now onresolveRequestIdentity: rejections assert{status:"invalid"}, which the wrapper could not tell apart fromabsent, and successes assert the verified identity.parsePositiveInteger(types.ts) had no callers anywhere.cailIdentityJwtcontext value andgetCailIdentityJwt: since e1cf4b6, model calls forward only the gateway leg. Nothing read the stored app-leg token.packages/, includingroutes/agents.tsand the agent/Durable Object code, found only the setter, the getter, and a type field.Duplicate tests
fleet-projection.test.ts: the test "recognizes every canonical action route…" repeated the action inventory already pinned byobservability-contract.test.ts"defines queryable build and publish action seams". Its only unique check,ROUTE_TEMPLATE_REvalidity, moved into that test.AgentChat.test.ts: "schedules a reconnect with backoff after an unexpected socket close" ran the same first reconnect as "keeps reconnecting when a reconnect attempt errors without logging socket details", which goes further. The one unique assertion, no new socket before the timer fires, moved into the retained test.Rewritten at the owner boundary
session.test.ts: "stores the verified canonical token for downstream calls" is replaced by two tests through the realauthMiddleware. Before this, nothing tested the forwarded gateway leg.cail:gatewaytoken is stored and returned bygetCailGatewayJwt(c).c.set("cailGatewayJwt", …)fails the first test, and ignoringgatewayLeg === "invalid"fails the second.site-builder.test.ts(buildProjectContext):not.toContain("Uploaded documents in the project: assets/legacy.docx")could never fail, because files are sorted and a leaked.docxwould followcv.pdfin that line."supported documents"only pinned wording removed in Repair Site Studio uploads, tools, and recovery #68.extract_document_textline and asserts that it containsassets/cv.pdfand notlegacy.docx.agents/project-context.tschanged to also admit.docx, the old assertions passed. The new test fails withexpected 'Uploaded documents in the project: as…' not to contain 'legacy.docx'. The source was restored afterward.LOC
Test counts: app goes from 760 to 750 (−9 http, −1 document, −1 fleet-projection, +1 session). Frontend goes from 225 to 224.
Kept on purpose (false positives)
api/errors.test.tsmessage assertions check passthrough of server messages. "collapses plain Errors to the generic message" is a leak guard.Preview.test.tsalert text and.loading-overlaytest the preview-ready protocol and therole=alertrecovery.HandleClaimDialog,ProjectHistoryDialog,AgentChatandsite-builderguards against leaks or project-switch races, or passes through server messages.boundary-logging"Service event recorded." guards against free-text log bodies.observability-contract.test.tsback the deploy job's release probes.index.test.ts, plus theprotected-files,agent-identityandcsrftests, cover routing and security.README.txttext inr2.test.tsis part of the archive-contents contract.AgentChat.test.tslines 2263 and 2306 each cover a different failure mode.Open question (not changed here)
Several parts of
lib/observability/contract.tsandlib/observability/action-attempt.tshave no production reader:createCloudflareHealthCheckSpec,auditTelemetryLifecyclePairs,summarizeDurableActionReliabilityandCAIL_LOG_ENVIRONMENTSare called only by tests.collection,fleetProjection,access,serviceLevelsanddashboardViewssections ofOBSERVABILITY_CONTRACTare never read at runtime.docs/cail-log-alignment.mddescribes these as an offline source contract and lifecycle auditor. Should they stay as a documented operator recipe, or be removed with their tests (about 300 production lines)? This PR leaves them untouched.A smaller follow-up: after this change, the
tokenfield ofresolveRequestIdentity's verified result is read only by tests. The doc comment on that function still says downstream model calls forward it.Validation
bunx vitest runover the 10 affected app files, includingregressionsandpreview: 267 of 267 passed.bunx tsc --noEmitinpackages/app: exit 0.bunx vitest run src/lib/components/AgentChat.test.ts: 60 of 60 passed.bun run check: rc=0.git diff --check: clean.🤖 Generated with Claude Code