chore: overhaul test system#338
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Thank you for following the naming conventions! 🙏 |
Contributor
📝 WalkthroughWalkthroughThis PR refactors CI into lint/typecheck/test/coverage jobs (adds top-level permissions), adds a scheduled/manual mutation workflow, introduces many Vitest test helpers and a slow-test reporter, updates Vitest/ESLint configs and package scripts, reorganizes and adds numerous tests (core, framework, workers, Next), and refactors framework tests to assert drained events instead of parsing console logs. ChangesTest Infrastructure, Coverage, and Mutation Testing Overhaul
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/evlog/test/nitro-v3/nitro-v3.test.ts (1)
27-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve and restore pre-existing
EVLOG_TEST_LOGinstead of always deleting it.Line 27 overwrites a global env var, and Line 43 unconditionally removes it. That can leak state into other tests when this variable is already set by the runner/shell. Save the previous value and restore it in teardown.
Suggested fix
describe.sequential('Nitro v3 Server with evlog', () => { + const previousEvlogTestLog = process.env.EVLOG_TEST_LOG let nitro: Awaited<ReturnType<typeof createNitro>> let devServer: ReturnType<typeof createDevServer> let server: Awaited<ReturnType<ReturnType<typeof createDevServer>['listen']>> beforeAll(async () => { process.env.EVLOG_TEST_LOG = '1' @@ afterAll(async () => { - delete process.env.EVLOG_TEST_LOG + if (previousEvlogTestLog === undefined) { + delete process.env.EVLOG_TEST_LOG + } else { + process.env.EVLOG_TEST_LOG = previousEvlogTestLog + } await devServer?.close() await nitro?.close() })🤖 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/evlog/test/nitro-v3/nitro-v3.test.ts` around lines 27 - 43, The test currently overwrites process.env.EVLOG_TEST_LOG in the setup and unconditionally deletes it in the afterAll, which can clobber external state; modify the setup around createNitro(...) to capture the previous value (e.g., const _prevEvlog = process.env.EVLOG_TEST_LOG) before assigning '1', and in the afterAll restore it (process.env.EVLOG_TEST_LOG = _prevEvlog) or delete it only if _prevEvlog is undefined — update the test file where process.env.EVLOG_TEST_LOG is set near createNitro(...) and where it’s cleared in afterAll to perform this save/restore logic.packages/evlog/test/http/http.test.ts (1)
6-16: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winReplace manual test infrastructure with standardized helpers from
test/helpers/.This file re-implements helpers instead of using the standardized ones provided. Per coding guidelines, tests must import real source helpers rather than re-implement them:
createTestContext(lines 6-16) duplicatesmakeContext()fromevents.ts. Replace with:import { makeContext, makeEvent } from '../helpers/events'and callmakeContext(makeEvent(id)).- Manual
vi.spyOn(globalThis, 'fetch')(line 21) should usemockFetch()fromfetch.tsinstead.- Manual
vi.useFakeTimers()/vi.useRealTimers()setup can usewithFakeTimers()helper fromtimers.ts(though the mixed timer scenario at line 91 needs careful handling).These patterns ensure consistency across the test suite and avoid duplicating logic.
🤖 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/evlog/test/http/http.test.ts` around lines 6 - 16, Replace the duplicated test helpers with the centralized helpers: remove the local createTestContext and instead import makeContext and makeEvent from ../helpers/events and call makeContext(makeEvent(id)) wherever createTestContext was used; replace the manual vi.spyOn(globalThis, 'fetch') usage with the mockFetch helper from ../helpers/fetch; replace explicit vi.useFakeTimers()/vi.useRealTimers() setup with the withFakeTimers helper from ../helpers/timers (and handle the mixed timer scenario around the code near the previous line 91 by selectively using real timers only where needed while using withFakeTimers for the rest).packages/evlog/test/frameworks/express.test.ts (1)
329-344: 🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoffConsider removing
{ repeats: 3 }if the test passes consistently without it.The test uses
waitForDrainCalls(drain)(line 342), which properly awaits drain calls withvi.waitForpolling. This async wait mechanism addresses race conditions better than repeated test runs. If this wait reliably prevents flakiness, the{ repeats: 3 }option adds unnecessary test execution overhead—the test will run 3 times per suite run even when passing.This test is the only one in the file using
{ repeats: 3 }; similar tests like "drain error does not break request" (line 318) do not need it. If stability is confirmed without repeats, removing this option would speed up the test suite.🤖 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/evlog/test/frameworks/express.test.ts` around lines 329 - 344, The test "enrich error does not prevent drain" currently uses the Vitest repeat option `{ repeats: 3 }` but also awaits drain completion via the helper waitForDrainCalls(drain); remove the `{ repeats: 3 }` option from the it(...) declaration so the test runs once, relying on waitForDrainCalls(drain) to handle async/race conditions; specifically edit the test definition that references enrich (vi.fn(...)), drain (from createPipelineSpies()) and waitForDrainCalls to drop the repeats option..github/workflows/ci.yml (1)
16-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin GitHub Actions to immutable commit SHAs instead of mutable version tags.
All
uses:references in this workflow (and others in.github/workflows/) use version tags like@v6,@v4which are mutable and create supply-chain risk. Replace each with a pinned full 40-character commit SHA.For example:
actions/checkout@5a4ac9002d0be2fb38bd78470d3c7be5ac1d6157 # v6This applies to lines 16–23 (checkout, pnpm/action-setup, setup-node) and recurs at 34–41, 56–63, 77–84, 99–108.
🤖 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 @.github/workflows/ci.yml around lines 16 - 23, The workflow uses mutable version tags for actions (e.g., actions/checkout@v6, pnpm/action-setup@v6, actions/setup-node@v6); replace each `uses:` value with the corresponding immutable 40-character commit SHA for actions/checkout, pnpm/action-setup, and actions/setup-node (you can append the original tag as a comment for clarity, e.g. "# v6"); update every occurrence in this file (and other files under .github/workflows/ where those actions appear) so no `@vX` style tags remain and verify the SHAs point to the intended releases before committing.
🤖 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 @.github/workflows/ci.yml:
- Around line 94-97: Add a workflow-level restrictive permissions block and only
elevate for the publish job if necessary: add a top-level "permissions" key with
"contents: read" to the workflow, and if the publish job ("publish") or the
action "pkg-pr-new" needs additional scopes, add a job-level "permissions" entry
scoped only for that job (e.g., increase to contents: write or other specific
scopes) rather than leaving defaults permissive; update the "publish" job to
include that job-level permissions block when required.
In `@packages/evlog/stryker.config.json`:
- Around line 1-31: Add a short explicative comment to the Stryker config
clarifying the mutation scope rationale: update the top-level metadata (e.g. the
existing "_" field or add a new "_comment" property) to state that only core
modules listed in "mutate" (logger.ts, pipeline.ts, redact.ts, error.ts,
catalog.ts, audit.ts, utils.ts, shared routes/headers/middleware) are targeted
and that framework integrations/adapters are intentionally excluded due to
runtime cost and covered by integration tests; keep the comment brief and next
to the "mutate" array so future maintainers understand the selection.
In `@packages/evlog/test/core/logger.test.ts`:
- Around line 667-685: The test manually manages fake timers around the logger
emit (vi.useFakeTimers/try/finally); replace this with the shared test helper by
wrapping the timered portion in the withFakeTimers helper from
test/helpers/timers.ts so timer setup/teardown is centralized. Locate the test
that calls initLogger and createRequestLogger, remove
vi.useFakeTimers()/vi.useRealTimers() and instead call withFakeTimers(() => {
const logger = createRequestLogger(...); vi.advanceTimersByTime(60);
logger.emit(); expect(infoSpy).toHaveBeenCalledTimes(1); }); ensuring you import
withFakeTimers at the top of the test file.
In `@packages/evlog/test/core/redact-integration.test.ts`:
- Around line 69-79: The test currently wires its own drainedEvents array,
custom drain callback (in the initLogger call) and a fixed sleep
(setTimeout(..., 10)) — replace those ad-hoc patterns with the shared test
helpers: use the drain spy from test/helpers (instead of drainedEvents + drain
callback) and the fake timers/timer helpers from test/helpers to advance time
deterministically rather than setTimeout; also replace any manual spy/timer
wiring used around the assertions at lines ~173-175 with the corresponding
helpers (e.g., drainSpy / useFakeTimers / advanceTimers) so the test uses the
shared drain/timer/spy utilities for deterministic CI-friendly behavior while
keeping the same assertions that reference initLogger and the events it emits.
In `@packages/evlog/test/frameworks/express.test.ts`:
- Around line 19-37: The mount function in the express test uses unsafe double
type assertions to call supertest methods; replace the double-cast by storing
the method-selected agent directly (use let agent =
request(app)[method](req.path)) and call agent.set(...) in the header loop, or
if TypeScript still complains, declare a narrow agent type (e.g., a local type
with set method) or add a targeted `@ts-expect-error` with a comment; update the
mount -> fire logic to use the direct agent variable instead of the two-step
unknown casts so operations on agent are type-safe (refer to mount, fire,
request(app), and agent).
In `@packages/evlog/test/helpers/fetch.ts`:
- Around line 28-33: The URL extraction in getFetchCall is wrong for Request
objects because rawUrl.toString() yields “[object Request]”; update the
extraction logic in getFetchCall to explicitly handle Request instances by
returning rawUrl.url, handle URL objects via rawUrl.toString() (or rawUrl.href),
and keep string arguments as-is (use typeof rawUrl === 'string' ? rawUrl :
rawUrl.url/ .toString()). Locate the code around the variables rawUrl and init
in getFetchCall (the const call = spy.mock.calls[...] and const [rawUrl, init =
{}] = call lines) and add the conditional branching so Request objects yield
their .url property.
In `@packages/evlog/test/helpers/framework.ts`:
- Around line 104-115: assertHttpEventEmitted currently uses findEventViaDrain
to pick the first event matching path, which can hide duplicate emissions;
change it to collect all drained events matching the predicate (use the same
drain source as findEventViaDrain or iterate drainFn.mock.calls / findAll
equivalent), assert that exactly one match exists
(expect(matches.length).toBe(1) with a helpful message including expected.path),
then perform the existing method/status/level checks on matches[0] and return
that single event; update references to assertHttpEventEmitted and keep
findEventViaDrain usage only if it can return all matches, otherwise replace
with an explicit filter over the drained events to enforce unique-match
semantics.
In `@packages/evlog/test/helpers/slowReporter.ts`:
- Line 3: SLOW_TEST_BUDGET_MS is set directly from
Number(process.env.EVLOG_SLOW_TEST_BUDGET_MS ?? 500) which yields NaN for
invalid env values and disables slow-test reporting; update the initialization
to parse and validate the env value (e.g., parseInt/Number then check
Number.isFinite or isNaN) and fall back to the default 500 when the parsed value
is invalid, keeping the identifier SLOW_TEST_BUDGET_MS and ensuring the default
is preserved.
In `@packages/evlog/test/nitro/plugin-enrichment.test.ts`:
- Around line 17-48: The test currently bypasses real Nitro lifecycle by
manually calling callEnrichAndDrain and nitroApp.hooks.callHook('evlog:enrich' /
'evlog:drain'); instead, drive the flow through Nitro's real request handling
(e.g., use the Nitro app test driver like app.handle/app.inject or supertest) so
hooks run via the framework. Replace callEnrichAndDrain usage with sending an
actual test request to the Nitro app that triggers the evlog plugin, then assert
on the emitted WideEvent captured from the plugin/store; keep references to the
hook names ('evlog:enrich', 'evlog:drain') and the emittedEvent/WideEvent
handling but remove manual hook invocation and error-catching in favor of
awaiting the real request lifecycle.
- Around line 164-166: Replace the hardcoded sleep ("await new Promise(resolve
=> setTimeout(resolve, 10))") with the shared test helper from test/helpers that
deterministically waits for pending drains (e.g., flushPromises or
waitForDrain); import the helper at top of the test file and call it in place of
the setTimeout so the "Wait for drain promise to settle" step uses the
deterministic helper rather than a fixed timeout.
In `@packages/evlog/test/README.md`:
- Around line 61-62: The fenced code block in packages/evlog/test/README.md that
lists the test/ directory (the block containing lines like "test/" and
"helpers/") is missing a language identifier (MD040); update the opening
backticks to include a language tag (for example change "```" to "```text" or
"```bash") so the block becomes fenced with a language, preserving the existing
block contents and closing backticks exactly as-is.
- Around line 141-144: Update the single-file test command in the README:
replace the incorrect command string "pnpm exec vitest run test/redact.test.ts"
with the correct path "pnpm exec vitest run test/core/redact.test.ts" so the
documented command matches the repository tree and runs the intended test file.
In `@packages/evlog/test/toolkit/toolkit.test.ts`:
- Around line 428-431: Tests are using ad-hoc vi.waitFor polling to assert async
drains (standaloneDrain and pluginDrain); replace those vi.waitFor blocks with
the shared async drain/setup helper(s from test/helpers) by importing the
helper(s) and calling the helper to await drain completion and assertions (use
the helper that waits for drain spies rather than manual vi.waitFor), and update
both occurrences where vi.waitFor checks standaloneDrain and pluginDrain to use
the shared helper(s).
In `@packages/evlog/vitest.config.ts`:
- Around line 43-52: Add a short policy note describing when and how to bump the
coverage thresholds (the thresholds object and its properties statements,
branches, functions, lines in vitest.config.ts) to the repository docs (e.g.,
AGENTS.md or test/README.md): state the current practice of keeping thresholds
~3 points below measured baseline, require a PR to include updated coverage
report when increasing values, and mention who should approve the bump and any
CI checks to run; keep the guidance brief and link back to vitest.config.ts as
the source of truth.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 16-23: The workflow uses mutable version tags for actions (e.g.,
actions/checkout@v6, pnpm/action-setup@v6, actions/setup-node@v6); replace each
`uses:` value with the corresponding immutable 40-character commit SHA for
actions/checkout, pnpm/action-setup, and actions/setup-node (you can append the
original tag as a comment for clarity, e.g. "# v6"); update every occurrence in
this file (and other files under .github/workflows/ where those actions appear)
so no `@vX` style tags remain and verify the SHAs point to the intended releases
before committing.
In `@packages/evlog/test/frameworks/express.test.ts`:
- Around line 329-344: The test "enrich error does not prevent drain" currently
uses the Vitest repeat option `{ repeats: 3 }` but also awaits drain completion
via the helper waitForDrainCalls(drain); remove the `{ repeats: 3 }` option from
the it(...) declaration so the test runs once, relying on
waitForDrainCalls(drain) to handle async/race conditions; specifically edit the
test definition that references enrich (vi.fn(...)), drain (from
createPipelineSpies()) and waitForDrainCalls to drop the repeats option.
In `@packages/evlog/test/http/http.test.ts`:
- Around line 6-16: Replace the duplicated test helpers with the centralized
helpers: remove the local createTestContext and instead import makeContext and
makeEvent from ../helpers/events and call makeContext(makeEvent(id)) wherever
createTestContext was used; replace the manual vi.spyOn(globalThis, 'fetch')
usage with the mockFetch helper from ../helpers/fetch; replace explicit
vi.useFakeTimers()/vi.useRealTimers() setup with the withFakeTimers helper from
../helpers/timers (and handle the mixed timer scenario around the code near the
previous line 91 by selectively using real timers only where needed while using
withFakeTimers for the rest).
In `@packages/evlog/test/nitro-v3/nitro-v3.test.ts`:
- Around line 27-43: The test currently overwrites process.env.EVLOG_TEST_LOG in
the setup and unconditionally deletes it in the afterAll, which can clobber
external state; modify the setup around createNitro(...) to capture the previous
value (e.g., const _prevEvlog = process.env.EVLOG_TEST_LOG) before assigning
'1', and in the afterAll restore it (process.env.EVLOG_TEST_LOG = _prevEvlog) or
delete it only if _prevEvlog is undefined — update the test file where
process.env.EVLOG_TEST_LOG is set near createNitro(...) and where it’s cleared
in afterAll to perform this save/restore logic.
🪄 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: 52b55481-9eb5-4e91-8b89-162e51c809f2
⛔ Files ignored due to path filters (2)
packages/evlog/test/toolkit/__snapshots__/api-surface.test.ts.snapis excluded by!**/*.snappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (60)
.github/workflows/ci.yml.github/workflows/mutation.yml.gitignoreAGENTS.mdapps/docs/eslint.config.mjsapps/just-use-evlog/eslint.config.mjseslint.config.mjspackage.jsonpackages/evlog/package.jsonpackages/evlog/stryker.config.jsonpackages/evlog/test/README.mdpackages/evlog/test/adapters/config.test.tspackages/evlog/test/core/audit.test.tspackages/evlog/test/core/catalog.test.tspackages/evlog/test/core/client-console.test.tspackages/evlog/test/core/error.test.tspackages/evlog/test/core/fork.test.tspackages/evlog/test/core/identity.test.tspackages/evlog/test/core/logger-browser.test.tspackages/evlog/test/core/logger-request-logger.test.tspackages/evlog/test/core/logger.test.tspackages/evlog/test/core/middleware.test.tspackages/evlog/test/core/pipeline.test.tspackages/evlog/test/core/redact-integration.test.tspackages/evlog/test/core/redact.test.tspackages/evlog/test/core/utils.test.tspackages/evlog/test/frameworks/elysia.test.tspackages/evlog/test/frameworks/express.test.tspackages/evlog/test/frameworks/fastify.test.tspackages/evlog/test/frameworks/hono.test.tspackages/evlog/test/frameworks/nestjs-real-runtime.test.tspackages/evlog/test/frameworks/nestjs.test.tspackages/evlog/test/frameworks/react-router.test.tspackages/evlog/test/frameworks/sveltekit.test.tspackages/evlog/test/helpers/events.tspackages/evlog/test/helpers/fetch.tspackages/evlog/test/helpers/framework.tspackages/evlog/test/helpers/frameworkMatrix.tspackages/evlog/test/helpers/slowReporter.tspackages/evlog/test/helpers/timers.tspackages/evlog/test/http/http.test.tspackages/evlog/test/http/shared-http-identity.test.tspackages/evlog/test/http/stream-server.test.tspackages/evlog/test/http/stream.test.tspackages/evlog/test/next/stream.test.tspackages/evlog/test/nitro-v3/fixture/plugins/test-drain.tspackages/evlog/test/nitro-v3/fixture/plugins/test-enrich.tspackages/evlog/test/nitro-v3/fixture/plugins/test-sampling.tspackages/evlog/test/nitro-v3/nitro-v3.test.tspackages/evlog/test/nitro/errorHandler.test.tspackages/evlog/test/nitro/plugin-enrichment.test.tspackages/evlog/test/nitro/plugin.test.tspackages/evlog/test/toolkit/api-surface.test.tspackages/evlog/test/toolkit/barrel.test.tspackages/evlog/test/toolkit/enrichers.test.tspackages/evlog/test/toolkit/toolkit.test.tspackages/evlog/test/workers-logger.test.tspackages/evlog/test/workers/logger.test.tspackages/evlog/test/workers/preset-dist-imports.test.tspackages/evlog/vitest.config.ts
💤 Files with no reviewable changes (2)
- packages/evlog/test/adapters/config.test.ts
- packages/evlog/test/workers-logger.test.ts
Comment on lines
+428
to
+431
| await vi.waitFor(() => { | ||
| expect(standaloneDrain).toHaveBeenCalledTimes(1) | ||
| expect(pluginDrain).toHaveBeenCalledTimes(1) | ||
| }) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use shared test helpers for async drain/setup synchronization.
Line 428 and Line 444 use ad-hoc vi.waitFor polling. Please switch these checks to the existing helper(s) in test/helpers/ for consistency and lower flake risk across suites.
As per coding guidelines: packages/evlog/test/**/*.test.{ts,tsx}: Use the helpers in test/helpers/ (drain spies, fake timers, fetch mock, framework matrix) in tests.
Also applies to: 444-444
🤖 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/evlog/test/toolkit/toolkit.test.ts` around lines 428 - 431, Tests
are using ad-hoc vi.waitFor polling to assert async drains (standaloneDrain and
pluginDrain); replace those vi.waitFor blocks with the shared async drain/setup
helper(s from test/helpers) by importing the helper(s) and calling the helper to
await drain completion and assertions (use the helper that waits for drain spies
rather than manual vi.waitFor), and update both occurrences where vi.waitFor
checks standaloneDrain and pluginDrain to use the shared helper(s).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/evlog/test/http/http.test.ts (1)
171-174: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winReuse
mockFetch()in thecreateHttpLogDrainsetup too.This describe still hand-rolls the same resolved
fetchspy thattest/helpers/fetch.tswas introduced to centralize. Switching the setup tomockFetch()keeps both HTTP suites on the same helper path.Suggested fix
beforeEach(() => { vi.useFakeTimers() - vi.spyOn(globalThis, 'fetch').mockResolvedValue(new Response(null, { status: 200 })) + mockFetch() })As per coding guidelines
packages/evlog/test/**/*.test.ts: “Use helpers fromtest/helpers/for tests: drain spies, fake timers, fetch mock, framework matrix — see decision table intest/README.md”.🤖 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/evlog/test/http/http.test.ts` around lines 171 - 174, The test duplicates a manual fetch spy in the beforeEach; replace that with the shared helper by calling mockFetch() instead of vi.useFakeTimers(); vi.spyOn(...).mockResolvedValue... — ensure you still call vi.useFakeTimers() if needed and invoke mockFetch() in the beforeEach for the createHttpLogDrain describe block so both suites use the centralized test/helpers/fetch.ts helper (refer to mockFetch() and createHttpLogDrain).
♻️ Duplicate comments (2)
packages/evlog/test/nitro/plugin-enrichment.test.ts (1)
17-48:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDrive the Nitro lifecycle through a real request path, not manual hook calls
Lines 17-48 still simulate plugin flow by directly calling
hooks.callHook(...). That bypasses framework wiring and keeps this as a unit-style simulation instead of a framework-fidelity integration test. Please switch this suite to a real Nitro request driver path and assert via shared test helpers (drain spy/timer helpers) rather than a localcallEnrichAndDrainshim.#!/bin/bash # Verify this test still uses manual hook simulation and lacks a real request driver call. sed -n '1,260p' packages/evlog/test/nitro/plugin-enrichment.test.ts rg -nP "callEnrichAndDrain|hooks\.callHook\('evlog:(enrich|drain)'\)|\b(app\.handle|app\.inject|supertest|Test\.createTestingModule)\b" packages/evlog/test/nitro/plugin-enrichment.test.tsAs per coding guidelines:
Framework tests must use the framework's real request driver (supertest, app.inject, app.handle, Test.createTestingModule, ...)andUse helpers from test/helpers/ for tests.🤖 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/evlog/test/nitro/plugin-enrichment.test.ts` around lines 17 - 48, The test currently bypasses Nitro by using the callEnrichAndDrain helper which directly calls nitroApp.hooks.callHook('evlog:enrich' ...) and hooks.callHook('evlog:drain' ...); replace this shim with a real framework-driven request: boot the Nitro app test instance and exercise it via its request driver (e.g., app.handle/app.inject or supertest) so the plugin lifecycle runs naturally, and assert outcomes via the shared test helpers (drain spy/timer helpers in test/helpers/) instead of local spies; update references to callEnrichAndDrain and any direct hooks.callHook('evlog:enrich'|'evlog:drain') calls to use the real request flow and the existing helper assertions.packages/evlog/test/core/redact-integration.test.ts (1)
68-90: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse the shared drain helpers instead of ad-hoc
vi.fn()+vi.waitFor.This assertion path is still hand-rolling the drain spy/wait logic even though the suite now has shared helpers for it. Please switch this test to the helper-based pattern (
waitForDrainCalls/findEventViaDrain) so it stays aligned with the rest of the test system.As per coding guidelines
packages/evlog/test/**/*.test.ts: “Use helpers fromtest/helpers/for tests: drain spies, fake timers, fetch mock, framework matrix — see decision table intest/README.md”.🤖 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/evlog/test/core/redact-integration.test.ts` around lines 68 - 90, Replace the ad-hoc drain spy and wait logic in this test with the shared test helpers: import and use waitForDrainCalls and findEventViaDrain from the test/helpers module instead of creating drain = vi.fn() and using vi.waitFor; initialize the logger the same way with initLogger (retaining redact config) but pass the helper-provided drain spy, call createLogger().emit(), await waitForDrainCalls(1) and retrieve the drained event with findEventViaDrain(0) and assert on drained.user.email and drained.note; keep existing assertions and redact expectations unchanged.
🤖 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/evlog/test/core/redact-integration.test.ts`:
- Around line 301-307: The test uses hard-coded JWT-like and Stripe-like literal
strings that can be flagged as real secrets; modify the test to construct those
fixture values at runtime from obviously fake fragments (e.g., join short static
segments) instead of embedding full secret-shaped literals so the masking
behavior is still exercised; update the test that calls redactEvent(...) with
resolveRedactConfig({ builtins: ['jwt'] }) and the other affected test(s) around
lines 313-320 to build the token and key placeholders dynamically and use those
generated placeholders in the event object passed to redactEvent.
- Around line 173-177: The afterEach cleanup currently assigns
process.env.NODE_ENV = originalNodeEnv which stringifies undefined and leaves
"undefined" set; change the cleanup in the afterEach hook (the block using
originalNodeEnv, vi.restoreAllMocks(), and initLogger()) to remove NODE_ENV when
originalNodeEnv is undefined and otherwise restore it: if originalNodeEnv is
undefined delete process.env.NODE_ENV; else set process.env.NODE_ENV =
originalNodeEnv; keep the vi.restoreAllMocks() and initLogger() calls as-is.
In `@packages/evlog/test/README.md`:
- Around line 170-173: Update the "CI topology" paragraph in
packages/evlog/test/README.md to reflect the current pipeline defined in
.github/workflows/ci.yml: remove the description about a `prepare` job and the
shared packages/evlog/dist artifact, and instead document that the workflow runs
independent jobs for `lint`, `typecheck`, sharded `test` (x4), `coverage`, and
`publish` (describe their order/triggering if applicable), and note that
mutation testing is handled separately by the mutation workflow
(`.github/workflows/mutation.yml`) on a weekly cron and on-demand; make sure to
reference the exact job names `lint`, `typecheck`, `test`, `coverage`, and
`publish` so contributors can locate them in the workflow file.
---
Outside diff comments:
In `@packages/evlog/test/http/http.test.ts`:
- Around line 171-174: The test duplicates a manual fetch spy in the beforeEach;
replace that with the shared helper by calling mockFetch() instead of
vi.useFakeTimers(); vi.spyOn(...).mockResolvedValue... — ensure you still call
vi.useFakeTimers() if needed and invoke mockFetch() in the beforeEach for the
createHttpLogDrain describe block so both suites use the centralized
test/helpers/fetch.ts helper (refer to mockFetch() and createHttpLogDrain).
---
Duplicate comments:
In `@packages/evlog/test/core/redact-integration.test.ts`:
- Around line 68-90: Replace the ad-hoc drain spy and wait logic in this test
with the shared test helpers: import and use waitForDrainCalls and
findEventViaDrain from the test/helpers module instead of creating drain =
vi.fn() and using vi.waitFor; initialize the logger the same way with initLogger
(retaining redact config) but pass the helper-provided drain spy, call
createLogger().emit(), await waitForDrainCalls(1) and retrieve the drained event
with findEventViaDrain(0) and assert on drained.user.email and drained.note;
keep existing assertions and redact expectations unchanged.
In `@packages/evlog/test/nitro/plugin-enrichment.test.ts`:
- Around line 17-48: The test currently bypasses Nitro by using the
callEnrichAndDrain helper which directly calls
nitroApp.hooks.callHook('evlog:enrich' ...) and hooks.callHook('evlog:drain'
...); replace this shim with a real framework-driven request: boot the Nitro app
test instance and exercise it via its request driver (e.g.,
app.handle/app.inject or supertest) so the plugin lifecycle runs naturally, and
assert outcomes via the shared test helpers (drain spy/timer helpers in
test/helpers/) instead of local spies; update references to callEnrichAndDrain
and any direct hooks.callHook('evlog:enrich'|'evlog:drain') calls to use the
real request flow and the existing helper assertions.
🪄 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: d2d49a63-fc72-45ff-a675-76ebffae8265
📒 Files selected for processing (13)
.github/workflows/ci.ymlpackages/evlog/stryker.config.jsonpackages/evlog/test/README.mdpackages/evlog/test/core/logger-request-logger.test.tspackages/evlog/test/core/logger.test.tspackages/evlog/test/core/redact-integration.test.tspackages/evlog/test/frameworks/express.test.tspackages/evlog/test/helpers/fetch.tspackages/evlog/test/helpers/slowReporter.tspackages/evlog/test/http/http.test.tspackages/evlog/test/nitro-v3/nitro-v3.test.tspackages/evlog/test/nitro/plugin-enrichment.test.tspackages/evlog/vitest.config.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/evlog/test/README.md`:
- Line 170: Update the README wording that currently claims "five independent
jobs in parallel" to accurately describe orchestration: state that the first
four jobs (lint, typecheck, test, coverage) run in parallel and the publish job
runs sequentially after those four complete successfully; also adjust the
sentence referring to publish to mention it "needs" or "depends on" the above
jobs instead of implying full parallel independence. Ensure the job names lint,
typecheck, test, coverage and publish are used so the change is unambiguous.
🪄 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: dc5530ad-3081-480d-ac02-aae0becab7d4
📒 Files selected for processing (3)
packages/evlog/test/README.mdpackages/evlog/test/core/redact-integration.test.tspackages/evlog/test/http/http.test.ts
|
|
||
| ## CI structure | ||
|
|
||
| `.github/workflows/ci.yml` runs five independent jobs in parallel on every PR: |
Contributor
There was a problem hiding this comment.
Clarify job orchestration: parallel vs. sequential.
Line 170 states "five independent jobs in parallel" but line 176 says publish "needs all of the above," which means the jobs are not all independent or parallel. The first four jobs (lint, typecheck, test, coverage) run in parallel, then publish runs sequentially after all four succeed.
📝 Proposed clarification
-`.github/workflows/ci.yml` runs five independent jobs in parallel on every PR:
+`.github/workflows/ci.yml` runs five jobs on every PR:
- `lint` — `pnpm run dev:prepare` + `pnpm run lint`
- `typecheck` — `pnpm run dev:prepare` + `pnpm run typecheck`
- `test` — `pnpm run dev:prepare` + `pnpm run build:package`, then `vitest run --shard=N/4` across a 4-way matrix
- `coverage` — `pnpm run dev:prepare` + `pnpm run build:package` + `pnpm --filter evlog run test:coverage` (enforces the thresholds in `vitest.config.ts`)
-- `publish` — needs all of the above; runs `pkg-pr-new publish` for evlog + nuxthub on PR / main / manual dispatch
+- `publish` — runs after the above four jobs succeed; runs `pkg-pr-new publish` for evlog + nuxthub on PR / main / manual dispatch
+
+The first four jobs run in parallel. `publish` waits for all of them.Also applies to: 176-176
🤖 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/evlog/test/README.md` at line 170, Update the README wording that
currently claims "five independent jobs in parallel" to accurately describe
orchestration: state that the first four jobs (lint, typecheck, test, coverage)
run in parallel and the publish job runs sequentially after those four complete
successfully; also adjust the sentence referring to publish to mention it
"needs" or "depends on" the above jobs instead of implying full parallel
independence. Ensure the job names lint, typecheck, test, coverage and publish
are used so the change is unambiguous.
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
1 participant
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.
🔗 Linked issue
📚 Description
📝 Checklist
Summary by CodeRabbit