Repository navigation
test: cut tests that can't catch a regression, fix the misbuilt ones - #107
Merged
Merged
Conversation
Removes 32 has_table, has_function and has_column assertions from six pgTAP files. A behavioral assertion in the same suite already reads, writes or calls each object, so it fails if the object goes missing. Grants, RLS, trigger and constraint checks stay. Plans go from 451 to 419. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- Delete prompt tests that only assert fixed prompt wording, and control-matrix spot checks that repeat the matrix tests. - Delete tests that grep handler source text instead of calling it, and a cancelled-stream billing test that ai-stream's aborted scenario already covers. - Rewrite the demo-code contract test to call the real create-demo-code handler: it checks the stored hash, that the stored row never reveals the code, and that admin-role codes are refused. - Rewrite the model-catalog test to check the rules behind the list (low effort everywhere, valid defaults) instead of the ten IDs. Deno tests: 341 to 315. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The isAuthorized test only checked the fake database's canned `true`, so swapped document and author arguments would still pass. It now asserts the exact parameters sent to the membership query. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Drop four vercel-deployment-config cases that restate config literals, static copy or dependency keys. The security-header, SPA-fallback and cache-rule cases stay. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
"renders the empty state" and "displays results when store has data" only check that mocked children render. "handles form submission" repeats "keeps one-off results out of every debate", which also checks debate scoping. Other page tests are left alone because open PR #95 changes them. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… duplicate - debate-context: the one-off mode test only rendered a notice. It now sends a request and asserts no debate context or sources go out. - EnvironmentSwitcher: drop a case the sign-out-failure test already covers by clicking the same "Switch to Production" button. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…at cannot fail - animation-polish: the normal-motion test runs the same flow as the reduced-motion test with weaker assertions. - loading-performance: the AppLayout-stays-mounted test is a subset of navigation-performance's delayed-chunk test. - navigation-performance: the es/hu delayed-route tests only check translated labels that route-skeleton.test.tsx already pins. The delayed-chunk test loses a transitionMetrics block whose comparisons are true by construction; its shell-identity checks stay. - home-navigation: the desktop header test only asserts link labels and that old tool links are absent. Specs that open PRs #63, #94, #95, #99 and #100 change are left alone. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The metadata test only checked approximateMinutes > 0, so the "About N minutes" in Sparring setup could drift from the real schedule. It now requires the summed speech times. Drop a test of fixed copy and numbers for the school format. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- startup-shell: the /login and / delayed-entry tests repeat the /app startup-shell test; the shell is the same static HTML on every path. Three `.seo-fallback` assertions targeted an element removed in #49, so they could never fail. - public-pages: drop the logo test that restates the component's own src, width and height. - sparring: the mobile reduced-motion test drops logo attribute, removed-copy and 1px icon-alignment checks. It still checks the default format, the speaking-order toggle and overflow. The es/hu locale loops stay: since i18n.test.ts was removed they are the only check that a translation gap does not crash a page. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (29)
💤 Files with no reviewable changes (14)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThis pull request changes tests across end-to-end, application, service, Supabase function, and database suites. It removes or narrows many existing assertions, adds a request-level demo-code test, and strengthens checks for membership-query parameters, one-off brainstorm requests, sparring-format duration, and model-catalog behavior. ChangesEnd-to-end test coverage
Application and service tests
Supabase function tests
Database test assertions
Estimated code review effort: 3 (Moderate) | ~20 minutes Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 4 systems. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Essentials by visiting https://app.coderabbit.ai/settings/billing. Comment |
This branch was successfully deployed
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.
This PR deletes 74 tests that couldn't catch a real regression and fixes 9 that should have caught one but didn't. I audited every layer: Vitest, Deno, the collaboration service, Playwright and pgTAP. Only test files change: 29 files, +82 / −847 lines. The rules are the ones in #93.
It sits on top of #95, which merged during this work and cut the unit suite from 112 files to 24. Edits to files that #95 deleted were dropped in the rebase. #95's new CORS allowlist tests are kept. Three test files are left alone because open PRs change them:
e2e/workspace-collaboration.spec.ts(#63), ande2e/model-matrix/generate.spec.tsande2e/production/canary.spec.ts(#94). Theverify-debatelabrecipes are untouched.Before and after
Measured against
mainat 5ef8b24. Counts are executed tests. Playwright counts are--listtotals across its three projects, and pgTAP counts are the sum ofplan().src/)supabase/functions/)e2e/)supabase/tests)Wall times were measured on a shared VPS, where load from other worktrees varies:
npm testnpm run test:edgenpm run test:collaborationThe PR smoke set (
npm run test:e2e:smoke) goes from 31 to 29 tests. The two removed tests repeat the/appstartup-shell smoke test, and every route keeps its smoke load test.Delete: 74
A test is deleted when it restates a type, schema or config, checks that something exists, asserts static copy, only renders, only exercises mocks, can never fail, or duplicates another test at the same layer.
Examples to spot-check:
workspaces_rls_test.sql, fourhas_tablechecks sit right before an assertion that casts the same four tables toregclass.control-matrix.test.ts, "ask toggles and modes appear verbatim" checks an all-on and an all-off prompt. Both are rows of the 64-row matrix test directly below it.startup-shell.spec.tshas/loginand/delayed-entry tests that repeat the/appone. The startup shell is the same static HTML on every path.vercel-deployment-config.test.ts"pins the Vercel runtime to Node 22" only comparesengines.nodeto the literal"22.x".Fix: 9
These tests should have caught a regression and didn't. Each one now asserts observable behavior.
Examples to spot-check:
admin/demo-code-contract.test.tsused to grepadmin/index.tsfor variable names. Storing the raw demo code would have passed as long as the identifiers stayed. The test now drives the real handler and asserts:DEMO-XXXX-XXXXcode;code_hashequals the hash of the returned code;role: "admin".Each of three mutations to a temp copy of the handler makes it fail: storing the raw code, deriving the email from it, and dropping the role check.
In
debate-context.test.tsx, the one-off-mode test only rendered a notice. It now sends a request and asserts that nodebateContextand nosourceIdsgo out, which is the isolation guarantee.postgres-store.test.tsonly asserted the fake client's cannedtrue. It now asserts the exact membership query arguments[documentId, epoch, authorId, false].The other fixes:
formats.test.tsnow requires the displayed minutes to equal the summed speech times..seo-fallbackassertions targeted an element removed in perf: fix startup and route loading #49, andnavigation-performancehad a metrics block whose comparisons were true by construction. Both were cut.Keep: everything else
I kept specs that guard auth, RLS, billing, quotas, migrations, concurrency, parsers or fixed bugs, including the ugly ones. Three to check:
workspace_collaboration_test.sqlhas ahas_trigger ... notify_access_revoked_after_deleteexistence check. It is still the only guard on the revocation fence, because pgTAP can't observe a NOTIFY inside a rolled-back transaction.quality-backlog.spec.ts"Sources does not show the stale consultation date" looks like static copy, but it is the regression test for the bug fixed in fix: resolve retained UI bugs #45.authed-pagesandpublic-pagesloops look like the English runs with different strings. ci: three-minute PR check and two previews per PR #95 removedi18n.test.ts, so these loops are now the only check that a Spanish or Hungarian translation gap doesn't crash a page. They run nightly, not in the PR check.docs/testing.md:126still describes thei18n.test.tsthat #95 removed. I left it for a follow-up, because the doc should state the replacement once one is chosen.Verification
GitHub Actions did not run on this head: every job failed in one to two seconds with no steps, which is the out-of-minutes pattern. Both Vercel previews built. These are local results at
9c717f7, not CI results:npm run verifypasses: docs, lint, types, unit, collaboration, edge, build, service worker, and the 29-test smoke set (28 passed;crossfire.spec.tswas flaky and passed on retry).npm run test:e2e, desktop and mobile Chromium) passes: 233 passed, 5 skipped, 4 flaky that passed on retry. The flaky ones were page-load waits while the database gate shared the CPU. Two are inhungarian-parity, which this PR does not touch. Two are unchanged tests inloading-performance.npm test -- --coverage: 265 passed.npm run test:edge: 318 passed.npm run test:collaboration: 30 passed../.superset/db-gate.sh) passes: pgTAP 19 files and 419 tests, then the full-stack browser gate with 71 passed and 1 skipped. An earlier run under load average 37 timed out on three AI-call waits. None of those specs changed, and all three pass here.gpt-6.1-sol, high effort) found no actionable issues. It also ran the seven changed specs on desktop and mobile Chromium (88 passed) and confirmed each pgTAP file's assertion count matches itsplan(). Copilot was out of quota.🤖 Generated with Claude Code