chore: migrate test runner from jest to vitest - #474
Conversation
- add vitest.config.mjs (globals:true, pool:forks+isolate for clean exit, 42s default timeout, HANA CI subset + 10x timeout + HANA_PROM env ported from jest.config.js); delete jest.config.js - package.json: scripts.test -> "vitest run --silent"; drop jest devDep, add vitest - replace jest.spyOn/jest.fn call sites with vi.* (8 sites) - convert done-callback beforeAll/afterAll hooks to promises (vitest treats the first hook-callback arg as a fixture, not a done callback) - eslint.config.mjs: declare `vi` as a test-files global (cds config only provides jest/mocha globals) - test/bookshop: disable OTel http instrumentation (incoming+outgoing) so span trees match what the suite asserts — jest silently suppressed otel's require-in-the-middle http patching; vitest runs it for real, adding incoming SERVER spans that reparented the tx trees. Client spans in tracing-attributes still come via undici/cloud-sdk, so those assertions remain meaningful. - logging.test.js: drain the queue SchedulingService's startup outbox-scan trace once in beforeAll so its primer log doesn't leak into the spy window Full suite: 14 files passed, 5 skipped; 53 tests passed, 14 skipped — exact parity with jest. Exits cleanly (~7.7s, exit 0) without --forceExit.
SummaryThe following content is AI-generated and provides a summary of the pull request: Migrate Test Runner from Jest to VitestChore🔧 Replaces the Jest test runner with Vitest, resolving a long-standing issue where the OTLP exporter's open handles caused the test suite to hang and require Changes
GitHub Issues
PR Bot InformationVersion:
|
|
Follow-up filed: #475 (enable HTTP instrumentation in tests + assert on real incoming spans). |
![hyperspace-pr-bot[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
The migration from Jest to Vitest is well-structured and the PR description is thorough. The main concerns raised are: the teardownTimeout of 1 s is likely too tight for OTel teardown on slow CI machines; the HANA glob patterns are inconsistently deep compared to the default pattern (and passport.test.js may not exist, silently matching nothing); the beforeAll(() => wait(500)) timing workaround is fragile and unconditional; and the top-level vi.spyOn(...) calls in the metrics test files execute before Vitest's global injection, risking ReferenceError: vi is not defined — these files should either move the spy into a beforeAll or add an explicit import { vi } from 'vitest'.
PR Bot Information
Version: 1.29.18
- File Content Strategy: Full file content
- Event Trigger:
pull_request.opened - LLM:
anthropic--claude-4.6-sonnet - Correlation ID:
550b00b0-94b2-11f1-828a-06c444bb2075
| // top-of-module process.env mutations some test files rely on. | ||
| isolate: true, | ||
| // don't hang the run waiting on lingering handles at teardown. | ||
| teardownTimeout: 1000 |
There was a problem hiding this comment.
Bug: testTimeout is not a valid top-level Vitest config option.
In Vitest the per-test timeout is configured via test.testTimeout (or its alias test.timeout). Placing testTimeout as a sibling of include under test: is correct in principle, but the variable is named testTimeout and mapped to the key testTimeout — which Vitest does recognise as an alias for timeout inside the test block. However, the comment on line 29 says "In Vitest 4 the former poolOptions.forks.* are top-level options" only for pool-related options; testTimeout is still a test.* field, so this is fine as-is. The real issue is subtler: the mutable let testTimeout / let include are evaluated once when the config module is imported. If Vitest ever re-imports the config (e.g. for worker re-use), the HANA mutations won't re-run — but with pool: 'forks' and isolate: true this is not a concern in practice.
Actually the substantive issue is the teardownTimeout of 1 000 ms. It is very tight: if a test file's afterAll hook needs to close an HTTP server and wait for OTel exporters to flush, 1 s may not be enough and Vitest will forcibly terminate the hook, potentially masking real teardown errors. The old jest.config.js had no equivalent cap, so this is a new regression risk.
Consider raising teardownTimeout to at least 5000 (matching the typical OTel export interval) to avoid false-positive teardown failures on slow CI machines.
| teardownTimeout: 1000 | |
| teardownTimeout: 5000 |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| // before any test file requires @sap/cds, so keep it in the config module. | ||
| if (process.env.CI && process.env.HANA_DRIVER) { | ||
| testTimeout *= 10 | ||
| include = ['test/**/tracing-attributes.test.js', 'test/**/passport.test.js'] |
There was a problem hiding this comment.
Logic Error: The HANA include glob pattern is inconsistent with the default pattern, creating a silent mismatch.
The default include is ['test/**/*.test.js'] (single test/ level), but the HANA subset uses ['test/**/tracing-attributes.test.js', 'test/**/passport.test.js'] (recursive **). The old jest.config.js used bare globs ['**/tracing-attributes.test.js', ...] which Jest resolved relative to the project root, effectively the same. The new default pattern test/**/*.test.js is fine, but for consistency and to avoid accidentally picking up nested copies of those files, the HANA patterns should match the same depth convention. More importantly, passport.test.js — is there actually such a file in the test/ tree? If it doesn't exist, Vitest will silently run zero files for that pattern with no warning, which was also true under Jest but is worth verifying.
| include = ['test/**/tracing-attributes.test.js', 'test/**/passport.test.js'] | |
| include = ['test/tracing-attributes.test.js', 'test/passport.test.js'] |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| // The queue's SchedulingService runs an initial outbox scan on server "listening"; its | ||
| // telemetry "elapsed times:" trace primer is exported asynchronously and would otherwise | ||
| // land in the spy window below. Drain it once up front (jest happened to miss this window | ||
| // because it broke otel's http patching and shifted timings; vitest sees the real thing). |
There was a problem hiding this comment.
the details re jest in this comment are no longer meaningful once merged
- vitest.config: teardownTimeout 1000 → 5000 (OTel flush + server close headroom on slow CI)
- metrics-outbox{,-multitenant,-disabled}: explicit `import { vi }` (clearer than relying on globals injection at module top-level)
- logging.test: reword the beforeAll(wait) comment — drop the stale jest reference, point to #478 for the poll-based follow-up
|
Addressed the review in 0fa1927:
Not changed, with rationale:
Re-verified: |
Supersedes the #438 oxfmt spike — adopts **oxfmt** (`0.63.0`, pinned) properly. ## Config (`.oxfmtrc.jsonc`) Based on the #438 spike, verified against the `lib/*.js` house style: `singleQuote`, `semi: false`, `printWidth: 120`, `tabWidth: 2`, `trailingComma: none`, `arrowParens: avoid`. `ignorePatterns` excludes `*.md`, `node_modules`, `package-lock.json`, `CHANGELOG.md`, and `jest.config.js` (see coordination). ## Scripts - `format` → `npx oxfmt` (write is oxfmt's default) - `format:check` → `npx oxfmt --check` No git hook / husky / lint-staged — CI `format:check` + scripts only (the intrusive hook from the #438 spike is intentionally not carried over). ## Commits (reviewable split) 1. `chore: add oxfmt formatter tooling` — config + scripts + devDep + lockfile + CI step 2. `chore: apply oxfmt formatting` — repo-wide reformat (11 files, line-wrapping only; verified non-semantic via `git diff -w`) ## eslint coexistence `@sap/cds/eslint.config.mjs` is `recommended` + `no-unused-vars`/`no-console` only (no stylistic rules) → no conflict. `npm run lint` stays green. ## CI One line added to the `lint` job in `ci.yml`: `npm run format:check`. ## Verified `npm run format:check` ✅ (56 files) · `npm run lint` (--max-warnings=0) ✅ · `npm run test` → 53 pass / 14 skip, exit 0 · lockfile resolved from public npm (0 internal-registry URLs). ## Coordination Parallel PR #474 (jest→vitest) deletes `jest.config.js` — this PR excludes it from formatting (0-line diff confirmed) so no collision. `package.json` change here is additive (scripts + devDep); lockfile will conflict with #474 — whichever merges second rebases.
… ConsoleMetricExporter (#479) ## What Consolidates all outbox/metrics test-quality work into one PR (formerly split as #479 + the stacked #480). - **In-memory metric reader** — `test/bookshop/lib/MyInMemoryMetricReader.js`, the metrics counterpart to `MyInMemorySpanExporter` (#465). Mirrors production **DELTA** temporality: SUM counters are accumulated across flushes into per-series running totals; GAUGE datapoints keep the latest absolute value. Wired via the `metrics-outbox`, `metrics-outbox-disabled`, and `metrics` profiles in `.cdsrc.json`. - **Outbox suites off console spying** — the three `metrics-outbox*.test.js` suites drop the `console.dir` spy and fixed `wait()` sleeps in favor of the reader + an `expectEventually()` force-flush polling helper (fails fast if the meter provider isn't wired). Folds in #445's polling approach. - **ConsoleMetricExporter unit test** — new `test/console-metric-exporter.test.js`, a pure unit test of the exporter's formatting (db.pool table, queue table, other single-vs-array, tenant variants, host-metrics aggregation, shutdown→FAILED), mirroring `console-span-exporter.test.js`. - **`metrics.test.js`** converted from scraping `cds.test.log()` output to asserting on the in-memory reader's datapoints. Metrics testing now mirrors the tracing side exactly: a to-console unit test **plus** in-memory-exporter–based integration tests. ## Why Follow-up to #465 (span test infra): eliminate console/log spying in the metrics suite and give `ConsoleMetricExporter` direct unit coverage. ## Review addressed - Bot review triaged: explicit `COUNTER_METRIC_NAMES` dispatch for `isCounter`; real wall-clock debounce in the multitenant test; isolation NOTE on the module-level singletons. - Dropped the unused debug-log silencer in the multitenant suite (never asserted). Kept the single-tenant `debugLog` mock — it backs a real `unknown service` assertion. Test-only change (no `lib/` change), so no CHANGELOG entry — consistent with #465/#474/#476. closes #478 Supersedes #445 and #480 (both folded in here) — I'll close them once this merges.
1 participant
Supersedes the #437 stash. Migrates the test runner jest → vitest.
Why it's a clean win
Vitest with
pool: 'forks'+ per-file isolation tears each test file's child process down when the file finishes — so the OTLP exporter's lingering handles die with the child, and the suite exits cleanly with no--forceExit. This is the same open-handle class that hangs jest (see #472/#466). Verified: exit 0, ~8s, no hang across repeated runs.Config (
vitest.config.mjs)globals: true—describe/test/beforeEach/...stay available, test bodies unchanged.pool: 'forks',isolate: true,teardownTimeout: 1000— the clean-exit mechanism (documented inline).jest.config.jsHANA logic faithfully: defaulttestTimeout: 42000; underCI && HANA_DRIVER→includerestricted totracing-attributes+passport, timeout ×10,cds_requires_telemetry_tracingset whenHANA_PROM. Verified the subset selection.Changes
package.json:test→vitest run --silent; jest removed, vitest added.jest.config.jsdeleted.jest.spyOn/jest.fn→vi.spyOn/vi.fn.beforeAll(done => …)hooks → promise-returning (vitest treats a hook arg as a fixture).eslint.config.mjs: test-files override declaringviglobal. Lint clean.Under jest, OTel's
require-in-the-middlehttp patching was silently broken by jest's module sandbox, so incoming HTTP SERVER spans never existed in tests (the existingxtestskips document this). Under vitest (realrequire) the instrumentation works and reparents trace trees, breaking several assertions. To keep this migration behavior-neutral,test/bookshop/package.jsonnow setsdisableIncomingRequestInstrumentation+disableOutgoingRequestInstrumentationon the http instrumentation — reproducing jest's effective environment.Consequence: the HTTP-instrumentation path stays untested (same blind spot as jest, now explicit config rather than an accident). Follow-up issue filed to enable it and assert on the real incoming spans. The tracing-attributes client-span assertions still pass because those spans come via undici / cloud-sdk, not instrumentation-http.
Coordination
Parallel PR #473 (prettier) touches
package.json(additive) + lockfile. Lockfile will conflict — whichever merges second rebases. #473 excludesjest.config.jsfrom formatting (this PR deletes it).Verified
npm run test53 pass / 14 skip, exit 0, ~8s, clean exit ×3 ·npm run lintclean · HANA subset selection confirmed.