Repository navigation
Assert on silenced console output instead of blanket-suppressing it - #722
Merged
Merged
Conversation
📝 WalkthroughWalkthroughTesting guidance and support helpers now require explicit handling of console output. Tests assert audit events and expected warnings, while Vitest uses a targeted plugin to suppress one third-party sourcemap warning. ChangesTest observability
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant VitestConfig
participant SourcemapWarningPlugin
participant ViteLogger
VitestConfig->>SourcemapWarningPlugin: register plugin
ViteLogger->>SourcemapWarningPlugin: warn(message)
SourcemapWarningPlugin->>SourcemapWarningPlugin: match third-party sourcemap warning
SourcemapWarningPlugin-->>ViteLogger: suppress matching warning
SourcemapWarningPlugin-->>ViteLogger: forward other warnings
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
kody-bot
marked this pull request as ready for review
July 11, 2026 05:18
Contributor
|
🔎 Preview deployed: https://kody-pr-722.kody-a99.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/worker/src/app/handlers/two-factor.node.test.ts (1)
21-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated
auditSpy+vi.mockpattern into a shared test helper.The identical 10-line block is now copy-pasted across all 5 handler test files. The PR stack already establishes shared test-support modules (
packages/worker/src/test-support/console-spies.ts). Extracting acreateAuditLogSpy()helper there would eliminate the duplication and ensure consistent mock setup.♻️ Proposed shared helper
// packages/worker/src/test-support/audit-spy.ts import { vi } from 'vitest' export function createAuditLogSpy() { const auditSpy = vi.hoisted(() => ({ logAuditEvent: vi.fn(async () => undefined), })) function mockAuditLog() { vi.mock('`#app/audit-log.ts`', async (importOriginal) => { const actual = await importOriginal<typeof import('`#app/audit-log.ts`')>() return { ...actual, logAuditEvent: (...args: Array<unknown>) => auditSpy.logAuditEvent(...args), } }) } return { auditSpy, mockAuditLog } }Usage in each test file:
-const auditSpy = vi.hoisted(() => ({ - logAuditEvent: vi.fn(async () => undefined), -})) - -vi.mock('`#app/audit-log.ts`', async (importOriginal) => { - const actual = await importOriginal<typeof import('`#app/audit-log.ts`')>() - return { - ...actual, - logAuditEvent: (...args: Array<unknown>) => auditSpy.logAuditEvent(...args), - } -}) +import { createAuditLogSpy } from '`#test-support/audit-spy.ts`' + +const { auditSpy, mockAuditLog } = createAuditLogSpy() +mockAuditLog()🤖 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 `@packages/worker/src/app/handlers/two-factor.node.test.ts` around lines 21 - 31, Extract the repeated hoisted audit spy and `#app/audit-log.ts` mock into a shared createAuditLogSpy helper under the existing test-support modules. Have the helper return auditSpy and mockAuditLog, preserving the current mocked logAuditEvent behavior, then update all five handler test files to use the helper instead of duplicating the vi.hoisted and vi.mock blocks.
🤖 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 `@packages/worker/src/app/handlers/auth-handler.node.test.ts`:
- Around line 378-385: Add toHaveBeenCalledTimes assertions alongside each
auditSpy.logAuditEvent toHaveBeenCalledWith block in the affected auth-handler
tests: expect 1 call for login-failure, login-success, and signup-failure, and 2
calls for signup-success. Keep the count assertions adjacent to their
corresponding expectations.
In `@packages/worker/src/app/handlers/auth-provider.node.test.ts`:
- Around line 320-327: Add
expect(auditSpy.logAuditEvent).toHaveBeenCalledTimes(1) alongside both OAuth
audit assertions in the relevant tests, including the assertions around the
oauth_signup success event and the second OAuth case, to ensure exactly one
audit event is emitted.
In `@packages/worker/src/app/handlers/two-factor.node.test.ts`:
- Around line 321-342: Add explicit audit call-count assertions in the
setup/enable test around the existing auditSpy.logAuditEvent expectations,
asserting exactly three calls, and in the disable test assert exactly two calls.
Use the existing auditSpy.logAuditEvent spy and place each toHaveBeenCalledTimes
assertion alongside its corresponding event expectations.
In
`@packages/worker/src/mcp/capabilities/integrations/integration-registry-search.node.test.ts`:
- Around line 45-47: The MSW warning suppression is overly broad in the test
setup. Update the argument passed to silenceExpectedConsoleWarns in
integration-registry-search.node.test.ts to match the specific redundant
query-parameter warning text, preserving unrelated MSW warnings as test
failures.
---
Nitpick comments:
In `@packages/worker/src/app/handlers/two-factor.node.test.ts`:
- Around line 21-31: Extract the repeated hoisted audit spy and
`#app/audit-log.ts` mock into a shared createAuditLogSpy helper under the existing
test-support modules. Have the helper return auditSpy and mockAuditLog,
preserving the current mocked logAuditEvent behavior, then update all five
handler test files to use the helper instead of duplicating the vi.hoisted and
vi.mock blocks.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4ee59c1c-9196-439d-8ad1-adb787affa6f
📒 Files selected for processing (28)
docs/contributing/testing-principles.mdpackages/worker/src/app/email/cloudflare-email.node.test.tspackages/worker/src/app/handlers/account-profile.node.test.tspackages/worker/src/app/handlers/account-resend-verification.node.test.tspackages/worker/src/app/handlers/auth-handler.node.test.tspackages/worker/src/app/handlers/auth-provider.node.test.tspackages/worker/src/app/handlers/two-factor.node.test.tspackages/worker/src/app/retention.node.test.tspackages/worker/src/email/inbound.workers.test.tspackages/worker/src/email/outbound.workers.test.tspackages/worker/src/email/system-email-subscriptions.workers.test.tspackages/worker/src/email/system-email.workers.test.tspackages/worker/src/jobs/reconcile-artifacts-pushes.node.test.tspackages/worker/src/jobs/service.node.test.tspackages/worker/src/mcp/capabilities/integrations/integration-registry-search.node.test.tspackages/worker/src/mcp/execute-dynamic-worker-reuse.workers.test.tspackages/worker/src/mcp/observability.workers.test.tspackages/worker/src/mcp/run-kody-registry.node.test.tspackages/worker/src/package-registry/service.node.test.tspackages/worker/src/package-runtime/module-graph.workers.test.tspackages/worker/src/remote-connector/session.node.test.tspackages/worker/src/repo/repo-session-do.node.test.tspackages/worker/src/test-support/console-spies.tspackages/worker/src/test-support/incidental-runtime-warnings.tstools/vite-suppress-sourcemap-warnings.node.test.tstools/vite-suppress-sourcemap-warnings.tsvitest-shared.tsvitest.config.ts
Comment on lines
+378
to
+385
| expect(auditSpy.logAuditEvent).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| category: 'auth', | ||
| action: 'login', | ||
| result: 'failure', | ||
| reason: 'invalid_credentials', | ||
| }), | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add toHaveBeenCalledTimes assertions to match the pattern in other handler tests.
All four assertion blocks use only toHaveBeenCalledWith without toHaveBeenCalledTimes. This means extra unexpected audit events would pass silently — the exact regression class this PR aims to prevent. The account-profile and account-resend-verification tests correctly pair both assertions; this file should follow suit.
For the signup-success case (lines 416-430), the expected count is 2 (signup + invite_use). For the login-failure (378-385), login-success (535-542), and signup-failure (582-588) cases, the expected count is 1 each.
🔧 Proposed additions
// Lines 378-385 — login failure
+expect(auditSpy.logAuditEvent).toHaveBeenCalledTimes(1)
expect(auditSpy.logAuditEvent).toHaveBeenCalledWith(
expect.objectContaining({
category: 'auth',
action: 'login',
result: 'failure',
reason: 'invalid_credentials',
}),
)
// Lines 416-430 — signup success + invite_use
+expect(auditSpy.logAuditEvent).toHaveBeenCalledTimes(2)
expect(auditSpy.logAuditEvent).toHaveBeenCalledWith(
expect.objectContaining({
category: 'auth',
action: 'signup',
result: 'success',
email: 'invited@example.com',
}),
)
expect(auditSpy.logAuditEvent).toHaveBeenCalledWith(
expect.objectContaining({
category: 'auth',
action: 'invite_use',
result: 'success',
}),
)
// Lines 535-542 — login success
+expect(auditSpy.logAuditEvent).toHaveBeenCalledTimes(1)
expect(auditSpy.logAuditEvent).toHaveBeenCalledWith(
expect.objectContaining({
category: 'auth',
action: 'login',
result: 'success',
email,
}),
)
// Lines 582-588 — signup failure
+expect(auditSpy.logAuditEvent).toHaveBeenCalledTimes(1)
expect(auditSpy.logAuditEvent).toHaveBeenCalledWith(
expect.objectContaining({
category: 'auth',
action: 'signup',
result: 'failure',
}),
)Also applies to: 416-430, 535-542, 582-588
🤖 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 `@packages/worker/src/app/handlers/auth-handler.node.test.ts` around lines 378
- 385, Add toHaveBeenCalledTimes assertions alongside each
auditSpy.logAuditEvent toHaveBeenCalledWith block in the affected auth-handler
tests: expect 1 call for login-failure, login-success, and signup-failure, and 2
calls for signup-success. Keep the count assertions adjacent to their
corresponding expectations.
Comment on lines
+320
to
+327
| expect(auditSpy.logAuditEvent).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| category: 'auth', | ||
| action: 'oauth_signup', | ||
| result: 'success', | ||
| reason: 'provider=github', | ||
| }), | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add toHaveBeenCalledTimes(1) to both OAuth audit assertions.
Same gap as auth-handler.node.test.ts — without count assertions, extra unexpected audit events go undetected, which contradicts the PR's objective.
🔧 Proposed additions
// Lines 320-327 — oauth_signup
+expect(auditSpy.logAuditEvent).toHaveBeenCalledTimes(1)
expect(auditSpy.logAuditEvent).toHaveBeenCalledWith(
expect.objectContaining({
category: 'auth',
action: 'oauth_signup',
result: 'success',
reason: 'provider=github',
}),
)
// Lines 354-361 — oauth_login
+expect(auditSpy.logAuditEvent).toHaveBeenCalledTimes(1)
expect(auditSpy.logAuditEvent).toHaveBeenCalledWith(
expect.objectContaining({
category: 'auth',
action: 'oauth_login',
result: 'success',
reason: 'provider=github',
}),
)Also applies to: 354-361
🤖 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 `@packages/worker/src/app/handlers/auth-provider.node.test.ts` around lines 320
- 327, Add expect(auditSpy.logAuditEvent).toHaveBeenCalledTimes(1) alongside
both OAuth audit assertions in the relevant tests, including the assertions
around the oauth_signup success event and the second OAuth case, to ensure
exactly one audit event is emitted.
3 of 4 tasks
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.
Why
Follow-up to #721. When output is silenced in tests, we must assert on the output we expect — and assert the absence of output we don't — so a suppression can never hide a real regression. Several suppressions from the console-noise cleanup were blanket silences that violated that principle.
What changed
Vite warnings: blanket
logLevel: 'error'→ targeted filterlogLevel: 'error'suppressed every Vite warning. Replaced with atools/vite-suppress-sourcemap-warnings.tsplugin that drops only the exactSourcemap for "…/node_modules/…" points to missing source filesmessage; all other warnings (including first-party sourcemap breakage) still surface.tools/vite-suppress-sourcemap-warnings.node.test.tsasserts both directions: the third-party message is dropped, and first-party/unknown warnings pass through.Audit log: silent stubs → one shared spy with assertions
packages/worker/src/test-support/audit-log-spy.tssetup file (wired into thenode-unitproject) routeslogAuditEventthrough a single exportedlogAuditEventSpy, replacing the per-filevi.hoisted+vi.mockblocks that were previously repeated in seven handler test files (auth-handler,auth-provider,two-factor,account-resend-verification,account-profile,password-reset,admin-users).auditEventSummaries()helper returns the orderedaction:resultlist so tests assert the exact audit event sequence — extra unexpected events fail, not just missing ones. Tests also assertnot.toHaveBeenCalled()where no audit is expected (profile reads).admin-capabilities.node.test.tsasserts realaudit_eventsrows, so it opts out withvi.unmock('#app/audit-log.ts');password-reset/admin-userskeep a file-local mock only to pingetRequestIp, routing the sink back through the shared spy.Console silences: blanket
mockImplementation(() => {})→ allowlists or assertionssilenceExpectedConsoleWarns/silenceExpectedConsoleErrorshelpers inconsole-spies.ts: swallow only an explicit allowlist of expected messages and throw on anything else.incidental-runtime-warnings.tsshared allowlist for the recurring bundler/registry-runtime noise ([worker-bundler]experimental notice,mcp-server-refs-load-failed,usage-*-failed,package-runtime-debug-*).expect(consoleWarn).toHaveBeenCalledWith(...)assertions.mockImplementation(() => {})site now has an assertion on the captured calls (verified by scanning the tree).Docs
docs/contributing/testing-principles.mdnow documents both rules: never silence blindly (assert on contract logs, use the allowlist helpers for incidental logs), and use the sharedlogAuditEventSpyfor audit assertions.Testing
npm run validate(format, lint, typecheck, 806 unit tests, 13 Playwright E2E, MCP E2E — all green)Sourcemap … missing source fileswarnings; with it enabled the full unit run has zero, and the unit test proves non-matching warnings still pass through.Sourcemap for,Using secrets defined,ExperimentalWarning,audit-event {.System recap — composes existing primitives (low risk, test-only)
Mode: recap · Base:
main@47c9a24e· Head:35562e0fClassification: composes — no runtime primitive code changes. The diff touches only test files, test-support helpers (
console-spies.ts, newincidental-runtime-warnings.tsandaudit-log-spy.ts), Vitest configuration, and a new Vite test plugin (tools/vite-suppress-sourcemap-warnings.ts). No entry inprimitives.yamlis added or reshaped.Primitives touched
app-uiauth/account handlers,jobs,email,repo-sessions,package-runtime, andcapabilities-executegained assertions on previously-silenced console output; the primitives themselves are unchanged.Change flow
Test harness only: console silencing now goes through allowlist helpers that throw on unexpected messages, and per-file audit-log stubs became one shared setup-file spy with exact-sequence assertions.
Red here means "new test-support module", not a new system primitive.
Summary by CodeRabbit