test: harden silently-passing tests, prune framework-only and duplicate ones#1339
Conversation
…te ones Audit-driven cleanup of low-value and misleading tests: - Fix silent-skip tests that passed when the asserted behavior was broken: device-flow Token Revoke (missing expect before the success guard), theme-unit-test gutenberg conversions (assertions gated on if(post)), and two e2e specs (publish status, plugins/wp-import "no error") that passed on a blank page. - Rewrite plugins/hooks.test.ts sorting/beforeSave tests to actually execute the pipeline and assert ordering, dependency order, and content chaining instead of only checking getHookCount. - Strengthen field structural validators (image/file/portableText) and the preview DO no-op transaction test with real reject/assert cases. - Delete input-rules.test.ts (tested vanilla TipTap, not EmDash) and remove tautological/duplicate tests (auth-atproto descriptor invariants, do-config passthrough dupes, SignupPage/MediaDetailPanel/invite-flow/ settings-pages duplicates).
|
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-playground | f7b7479 | Jun 04 2026, 11:04 AM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-cache | f7b7479 | Jun 04 2026, 11:04 AM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
docs | f7b7479 | Jun 04 2026, 11:04 AM |
github-actions
Bot
added
review/needs-review
Scope checkThis PR changes 651 lines across 18 files. Large PRs are harder to review and more likely to be closed without review. If this scope is intentional, no action needed. A maintainer will review it. If not, please consider splitting this into smaller PRs. See CONTRIBUTING.md for contribution guidelines. |
There was a problem hiding this comment.
Pull request overview
Cleans up and hardens the test suite by removing low-signal/tautological tests and tightening assertions in unit/integration/e2e coverage. In addition, it updates the Cloudflare astro dev SSR Vite pre-bundling list (with a changeset) to prevent re-optimize/reload cascades on first authenticated/admin/MCP requests.
Changes:
- Harden multiple tests that previously could pass without exercising the intended behavior (adds positive anchors and reject/assertion guards).
- Remove/merge framework-only or duplicate tests across admin, auth-atproto, and Cloudflare packages.
- Expand Cloudflare SSR
optimizeDeps.includeto pre-bundle late-discovered auth/MCP/admin deps and add a patch changeset documenting the fix.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/tests/unit/plugins/hooks.test.ts | Reworks hook pipeline tests to execute real hook behavior using an in-memory SQLite/Kysely context. |
| packages/core/tests/unit/fields/all-fields.test.ts | Adds negative/reject cases so schemas are required to throw on invalid inputs. |
| packages/core/tests/integration/wordpress-migration/theme-unit-test.test.ts | Replaces conditional assertions with explicit fixture precondition assertions and adds a loop guard to prevent silently asserting nothing. |
| packages/core/tests/integration/auth/device-flow.test.ts | Adds explicit success === true expectations before narrowing guards in revoke tests. |
| packages/core/src/astro/integration/vite-config.ts | Adds additional Cloudflare SSR optimizeDeps.include entries to prevent dev-time re-optimize cascades for auth/MCP/admin-only imports. |
| packages/cloudflare/tests/do-config.test.ts | Removes redundant passthrough tests. |
| packages/cloudflare/tests/db/do-dialect.test.ts | Adds an assertion ensuring preview transaction methods don’t issue SQL (read-only behavior). |
| packages/auth-atproto/tests/auth.test.ts | Removes tautological “descriptor invariants” tests that only assert constant shapes. |
| packages/admin/tests/editor/input-rules.test.ts | Deletes framework-only TipTap tests that don’t validate EmDash behavior. |
| packages/admin/tests/components/SignupPage.test.tsx | Prunes duplicate step-advance tests while keeping behavioral assertions. |
| packages/admin/tests/components/MediaDetailPanel.test.tsx | Prunes duplicate caption-visibility coverage while keeping behavior checks. |
| e2e/tests/wordpress-import.spec.ts | Anchors “no crash” checks with a positive heading assertion to avoid blank-page false positives. |
| e2e/tests/settings-pages.spec.ts | Removes a duplicate “pipeline section” e2e test by folding checks into the primary test. |
| e2e/tests/plugins.spec.ts | Anchors the regression guard with a positive “plugin count/content rendered” assertion. |
| e2e/tests/invite-flow.spec.ts | Strengthens invite error assertions by checking the “Back to login” link is present. |
| e2e/tests/content-crud.spec.ts | Makes publish action unconditional and asserts the UI flips to “Unpublish”. |
| demos/cloudflare/emdash-env.d.ts | Updates generated demo collection typings (adds page template; expands featured_image shape). |
| .changeset/fix-cf-dev-optimize-cascade.md | Adds a patch changeset describing the Cloudflare dev optimizeDeps fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // A real in-memory DB is needed for the context factory so hooks can | ||
| // actually execute (getContext throws without one). | ||
| let db: Kysely<DbSchema>; | ||
|
|
||
| beforeEach(() => { | ||
| const sqliteDb = new Database(":memory:"); | ||
| db = new Kysely<DbSchema>({ dialect: new SqliteDialect({ database: sqliteDb }) }); | ||
| }); | ||
|
|
||
| afterEach(async () => { | ||
| await db.destroy(); | ||
| }); |
| // Find a post in the "Classic" category | ||
| const classicPost = wxrData.posts.find((p) => p.categories.includes("classic")); |
| // Auth deps imported only on auth/login/callback routes, so | ||
| // the initial page scan misses them. Pre-bundle to avoid a | ||
| // re-optimize + reload cascade on first authenticated request. | ||
| "emdash > @oslojs/crypto/hmac", | ||
| "emdash > @oslojs/crypto/subtle", | ||
| "emdash > @oslojs/crypto/rsa", | ||
| "emdash > arctic", | ||
| // MCP SDK — server/index.js statically imports ajv (CJS-only). |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
github-actions
Bot
added
review/awaiting-author
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
The test-hardening work is genuinely good. The device-flow narrowing guards, the theme-unit-test if (post) removals, the unconditional e2e publish flow with Unpublish verification, the synthetic embed test, the hooks rewrite against a real in-memory DB context, and the deletion of framework-only TipTap tests are all real improvements that make the test suite more trustworthy.
However, this PR is not test-only despite its own description, title, and changeset checkbox saying otherwise. It smuggles in three non-test changes:
packages/core/src/astro/integration/vite-config.ts— addsoptimizeDeps.includeentries for Cloudflare dev SSR.demos/cloudflare/emdash-env.d.ts— manually edits generated type declarations..changeset/fix-cf-dev-optimize-cascade.md— a changeset for the above production change.
This violates AGENTS.md scope discipline ("No drive-by refactors… in unrelated files") and is materially misleading to reviewers — the PR description causes reviewers to skip the production diff thinking there is none.
The emdash-env.d.ts file in particular has a header that says "Generated by EmDash on dev server start / Do not edit manually", yet it was hand-edited. If the new fields are needed, the generator should be updated, not the output.
Recommendation: split the vite-config.ts change and its changeset into a separate bug-fix PR. Revert the manual edit of emdash-env.d.ts and regenerate it properly (or move it to its own PR). The test changes can then land cleanly.
(The technical correctness of each new optimizeDeps.include entry is unverified without a build, since the sandbox does not have node_modules installed.)
| "emdash > @emdash-cms/auth > @oslojs/webauthn", | ||
| // Auth deps imported only on auth/login/callback routes, so | ||
| // the initial page scan misses them. Pre-bundle to avoid a |
There was a problem hiding this comment.
[needs fixing] This PR is explicitly described as test-only with no production code changes, yet it adds production build-configuration entries to the Cloudflare SSR optimizeDeps.include list. This violates AGENTS.md scope discipline ("No drive-by refactors, no bulk lint/type cleanups, no 'while I'm here' edits in unrelated files"). The vite-config.ts change and its changeset should be split into a separate bug-fix PR so reviewers can evaluate it on its own merits.
…lector, drop stray demos types - hooks.test.ts: close the better-sqlite3 handle in afterEach (matches repo pattern) - theme-unit-test: select a classic-category post with no wp: block markers so the classic-HTML fallback path is genuinely exercised - revert an unrelated regenerated demos/cloudflare/emdash-env.d.ts that was swept in accidentally; merged latest main so the already-merged #1336 vite-config/changeset no longer show in this PR's diff
github-actions
Bot
added
review/needs-rereview
|
Thanks @copilot — addressed all three:
|
There was a problem hiding this comment.
Prior review issue resolved: the non-test production changes (vite-config.ts, emdash-env.d.ts, and the changeset) have been dropped from the diff, and the PR is now genuinely test-only.
The test-hardening work is solid and the right thing to do:
- The device-flow
expect(...success).toBe(true)guards fix silently-passing narrowing blocks (matching the rest of the file). - The theme-unit-test removals of
if (post)skips andtoBeGreaterThanOrEqual(0)tautologies are real improvements; the synthetic embed markup and the tightened classic-editor selector both genuinely exercise the converter instead of skating by. - The e2e publish flow now asserts the status flip to "Unpublish", and the negative-only "does not show error" tests are anchored with positive content assertions so a blank page can't pass.
- The hooks rewrite runs against a real in-memory DB context and verifies priority ordering, dependency ordering, and content chaining — previously it only checked registration counts.
- Deleting the framework-only TipTap input-rules tests, duplicate SignupPage/MediaDetailPanel/settings-pages/invite-flow tests, and tautological descriptor-shape invariants is good cleanup.
I checked all 15 changed files against call sites, sibling implementations, and repo conventions. No logic bugs, regressions, or AGENTS.md violations remain. Good to land.
github-actions
Bot
added
review/approved
What does this PR do?
Audit-driven cleanup of low-value and misleading tests. The audit surfaced a long list of suspect tests; this PR acts only on the ones that are genuinely broken or worthless, after verifying each against the code under test. (Many flagged tests turned out to be legitimate, mocks at a network/dependency boundary that still assert real logic, and were left untouched.)
Fixed tests that passed even when the asserted behavior was broken:
integration/auth/device-flow.test.ts— the Token Revoke suite usedif (!result.success) return;with no precedingexpect, so a failed device-code/token exchange returned green. Addedexpect(...success).toBe(true)before the narrowing guard (matching the rest of the file).integration/wordpress-migration/theme-unit-test.test.ts— ~7 conversions gated all assertions onif (post), silently passing if the fixture lookup missed. Now assert the post exists (fixture verified to contain the blocks) and convert; the embed test (fixture has none) feeds synthetic markup; the content-integrity loop asserts it verified at least one post.e2e/content-crud.spec.ts"publish action changes status" — clicked Publish behindif (isVisible())and never checked the result. Now publishes unconditionally and asserts the action flips to "Unpublish".e2e/plugins.spec.ts/e2e/wordpress-import.spec.ts"does not show error" — negative-only assertions that passed on a blank page. Anchored each with a positive content assertion.Made registration-only tests actually exercise behavior:
unit/plugins/hooks.test.ts— the "hook sorting" and "content:beforeSave" tests only checkedgetHookCount. Rewritten to run the pipeline against a real in-memory DB context and assert priority order, dependency ordering, and content transformation/chaining. (The capability-enforcement tests were kept, they verify real registration gating.)Strengthened accept-only / no-assertion tests:
unit/fields/all-fields.test.ts— image/file/portableText "validate structure" tests only asserted valid input did not throw; added reject cases.cloudflare/db/do-dialect.test.ts— the no-op transaction test had no assertion; now asserts the read-only path issues no SQL to the stub.Removed framework-only / tautological / duplicate tests:
admin/tests/editor/input-rules.test.ts— it built a vanilla TipTap editor from bareStarterKitand tested the framework, not any EmDash code.auth-atproto/tests/auth.test.ts— removed the "descriptor shape invariants" block (asserted that hardcoded constants are non-empty strings).cloudflare/tests/do-config.test.ts— removed passthrough tests redundant with the full-descriptor assertions.SignupPage,MediaDetailPanel,invite-flow,settings-pages— removed/merged exact or same-name duplicate tests.No production code changed; this is test-only.
Closes #
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change) — ran the changed suites (core plugins/fields/auth, auth-atproto, cloudflare, admin SignupPage/MediaDetailPanel); e2e specs use standard Playwright APIs and run in CIpnpm formathas been runAI-generated code disclosure
Screenshots / test output
Targeted runs after changes: core plugins dir 592 passed, device-flow + theme-unit-test 51 passed, fields 41 passed, auth-atproto 13 passed, cloudflare do-config/do-dialect 11 passed, admin SignupPage + MediaDetailPanel 25 passed.
pnpm lint:quickclean,pnpm typecheckclean across all packages.