Skip to content

Commit 3290f57

Browse files
matt-aitkenclaude
andcommitted
test(testcontainers): fix assertNonNullable — drop the require("vitest") that collides with OTel auto-instrumentation
The previous fix in this PR (lazy-load `expect` inside assertNonNullable via require("vitest")) was intended to keep the module importable from contexts where vitest isn't initialized at module-load time (a vitest globalSetup, where the worker doesn't exist yet). But require() inside the function body collides with OTel's `@opentelemetry/instrumentation`, which uses `require-in-the-middle` to hook every Node `require()` call. vitest is ESM-only, so once OTel's been touched in the same process, the next `require("vitest")` call throws "Vitest cannot be imported in a CommonJS module using require()". That instrumentation runs as soon as the run-engine — or any code that loads its OTel-traced internals — is imported. That's: - Every run-engine internal test (run-queue, heartbeats, pendingVersion) uses assertNonNullable. - Every webapp test that transitively reaches RunEngine (triggerTask, runsBackfiller, plus any test that imports services using engine internals) does too. Each shard ran ~7/8 failing because some early test loaded run-engine, require-in-the-middle armed, then the next assertNonNullable call exploded — cascading the rest of the shard's tests via vitest's fail-fast on uncaught errors in the same process. Fix: replace the vitest.expect call with a plain throw. Vitest still gets a useful failure (the message shows in the stack trace) without the require() hazard. The test that PR #3438 originally needed this for (a globalSetup that imported assertNonNullable before workers existed) still works — there's no top-level vitest import any more. Verified: triggerTask.test.ts (which previously failed locally with the require-in-the-middle error) now passes 8/8. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 133d468 commit 3290f57

1 file changed

Lines changed: 15 additions & 10 deletions

File tree

  • internal-packages/testcontainers/src

internal-packages/testcontainers/src/utils.ts

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,6 @@ import path from "path";
77
import { isDebug } from "std-env";
88
import { GenericContainer, StartedNetwork, StartedTestContainer, Wait } from "testcontainers";
99
import { x } from "tinyexec";
10-
// `expect` is only used inside assertNonNullable — lazy-loaded via require
11-
// inside the function so this module can be imported in non-test contexts
12-
// (e.g. a vitest globalSetup that starts containers before any worker
13-
// exists, where vitest's expect-init-at-load-time would crash).
1410
import type { TaskContext } from "vitest";
1511
import { ClickHouseContainer, runClickhouseMigrations } from "./clickhouse";
1612
import { MinIOContainer } from "./minio";
@@ -190,12 +186,21 @@ export async function createMinIOContainer(network: StartedNetwork) {
190186
}
191187

192188
export function assertNonNullable<T>(value: T): asserts value is NonNullable<T> {
193-
// Loaded lazily so importers of this module don't pay the vitest top-level
194-
// init cost outside a test worker. See the import note at the top.
195-
// eslint-disable-next-line @typescript-eslint/no-require-imports
196-
const { expect } = require("vitest") as typeof import("vitest");
197-
expect(value).toBeDefined();
198-
expect(value).not.toBeNull();
189+
// Plain throw — *not* `vitest.expect`. Two reasons:
190+
// 1. This module is imported by globalSetup files that run before any
191+
// vitest worker exists, so `import { expect }` from "vitest" at
192+
// top level can crash on init.
193+
// 2. Lazy-loading via `require("vitest")` (the prior fix) collides
194+
// with OTel auto-instrumentation: `@opentelemetry/instrumentation`
195+
// hooks `require()` via `require-in-the-middle`, and vitest is
196+
// ESM-only — the require() throws "Vitest cannot be imported in
197+
// a CommonJS module using require()", failing every test that
198+
// uses `assertNonNullable` after OTel's been touched.
199+
// The plain throw still gives vitest a useful failure (the message is
200+
// shown in the stack trace) without the instrumentation hazard.
201+
if (value === null || value === undefined) {
202+
throw new Error(`assertNonNullable: value was ${value === null ? "null" : "undefined"}`);
203+
}
199204
}
200205

201206
export async function withContainerSetup<T>({

0 commit comments

Comments
 (0)