Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade @cloudflare/vitest-pool-workers to Vitest v2+ #6232

Merged
merged 9 commits into from
Sep 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/soft-beans-explode.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"@cloudflare/vitest-pool-workers": minor
---

feat: Support Vitest v2.0.x

This drops support for Vitest@1, due to the issues described in [workers-sdk#6071](https://github.com/cloudflare/workers-sdk/issues/6071)
2 changes: 1 addition & 1 deletion fixtures/asset-config/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
"@cloudflare/workers-types": "^4.20240821.1",
"run-script-os": "^1.1.6",
"undici": "^5.28.4",
"vitest": "1.5.0",
"vitest": "catalog:default",
"wrangler": "workspace:*"
},
"volta": {
Expand Down
6 changes: 3 additions & 3 deletions fixtures/asset-config/vitest.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,15 +32,15 @@ export default defineWorkersConfig({
// always "externalised", meaning they're imported directly by Node.
server: {
deps: {
// Vitest automatically adds `/^(?!.*(?:node_modules)).*\.mjs$/` as an
// `inline` RegExp: https://github.com/vitest-dev/vitest/blob/v1.5.0/packages/vitest/src/node/config.ts#L236
// Vitest automatically adds `/^(?!.*node_modules).*\.mjs$/` as an
// `inline` RegExp: https://github.com/vitest-dev/vitest/blob/v2.0.5/packages/vitest/src/constants.ts#L9
// We'd like `packages/vitest-pool-workers/dist/pool/index.mjs` to be
// externalised though. Unfortunately, `inline`s are checked before
// `external`s, so there's no nice way we can override this. Instead,
// we prevent the extra `inline` being added in the first place.
inline: new FilteredPushArray((item) => {
const str = `${item}`;
return str !== "/^(?!.*(?:node_modules)).*\\.mjs$/";
return str !== "/^(?!.*node_modules).*\\.mjs$/";
}),
external: [
/packages\/vitest-pool-workers\/dist/,
Expand Down
4 changes: 2 additions & 2 deletions fixtures/local-mode-tests/tests/logging.test.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import path from "node:path";
import { setTimeout } from "node:timers/promises";
import util from "node:util";
import { afterEach, beforeEach, expect, it, vi } from "vitest";
import { afterEach, beforeEach, expect, it, Mock, vi } from "vitest";
import { unstable_dev } from "wrangler";

let output = "";
function spyOnConsoleMethod(name: keyof typeof console) {
vi.spyOn(console, name).mockImplementation((...args: unknown[]) => {
(vi.spyOn(console, name) as Mock).mockImplementation((...args: unknown[]) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference this is needed because the name variable here does not result in the spyOn() function being narrowed enough for it to generate a sensible return type, resultng in never.

output += util.format(...args) + "\n";
});
}
Expand Down
3 changes: 2 additions & 1 deletion fixtures/vitest-pool-workers-examples/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
"type": "module",
"scripts": {
"check:type": "node tsc-all.mjs",
"list": "vitest --config vitest.workers.config.ts list",
"test": "vitest --config vitest.workers.config.ts --reporter basic",
"test:ci": "vitest run --config vitest.workers.config.ts --reporter basic"
},
Expand All @@ -16,7 +17,7 @@
"miniflare": "workspace:*",
"toucan-js": "^3.3.1",
"typescript": "^5.5.2",
"vitest": "1.5.0",
"vitest": "catalog:default",
"wrangler": "workspace:*"
},
"volta": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,15 @@ export default defineConfig({
// always "externalised", meaning they're imported directly by Node.
server: {
deps: {
// Vitest automatically adds `/^(?!.*(?:node_modules)).*\.mjs$/` as an
// `inline` RegExp: https://github.com/vitest-dev/vitest/blob/v1.5.0/packages/vitest/src/node/config.ts#L236
// Vitest automatically adds `/^(?!.*node_modules).*\.mjs$/` as an
// `inline` RegExp: https://github.com/vitest-dev/vitest/blob/v2.0.5/packages/vitest/src/constants.ts#L9
// We'd like `packages/vitest-pool-workers/dist/pool/index.mjs` to be
// externalised though. Unfortunately, `inline`s are checked before
// `external`s, so there's no nice way we can override this. Instead,
// we prevent the extra `inline` being added in the first place.
inline: new FilteredPushArray((item) => {
const str = item.toString();
return str !== "/^(?!.*(?:node_modules)).*\\.mjs$/";
return str !== "/^(?!.*node_modules).*\\.mjs$/";
}),
external: [
/packages\/vitest-pool-workers\/dist/,
Expand Down
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@
"turbo": "^1.10.14",
"typescript": "^5.5.2",
"vite": "^5.0.12",
"vitest": "^1.2.2"
"vitest": "catalog:default"
},
"packageManager": "[email protected]",
"engines": {
Expand All @@ -75,7 +75,7 @@
"@types/react-transition-group>@types/react": "^18",
"@cloudflare/elements>@types/react": "^18",
"capnpc-ts>typescript": "4.2.4",
"vitest@1.5.0>@types/node": "$@types/node"
"vitest>@types/node": "$@types/node"
},
"patchedDependencies": {
"[email protected]": "patches/[email protected]",
Expand Down
2 changes: 2 additions & 0 deletions packages/create-cloudflare/e2e-tests/helpers.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import assert from "assert";
import {
createWriteStream,
mkdirSync,
Expand Down Expand Up @@ -284,6 +285,7 @@ export const waitForExit = async (
export const createTestLogStream = (ctx: TaskContext) => {
// The .ansi extension allows for editor extensions that format ansi terminal codes
const fileName = `${normalizeTestName(ctx)}.ansi`;
assert(ctx.task.suite, "Suite must be defined");
return createWriteStream(path.join(getLogPath(ctx.task.suite), fileName), {
flags: "a",
});
Expand Down
14 changes: 12 additions & 2 deletions packages/miniflare/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,12 @@ import zlib from "zlib";
import exitHook from "exit-hook";
import { $ as colors$ } from "kleur/colors";
import stoppable from "stoppable";
import { Dispatcher, getGlobalDispatcher, Pool } from "undici";
import {
Dispatcher,
getGlobalDispatcher,
Pool,
Response as UndiciResponse,
} from "undici";
import SCRIPT_MINIFLARE_SHARED from "worker:shared/index";
import SCRIPT_MINIFLARE_ZOD from "worker:shared/zod";
import { WebSocketServer } from "ws";
Expand Down Expand Up @@ -830,7 +835,12 @@ export class Miniflare {
// Should only define custom service bindings if `service` is a function
assert(typeof service === "function");
try {
const response = await service(request, this);
let response: UndiciResponse | Response = await service(request, this);

if (!(response instanceof Response)) {
response = new Response(response.body, response);
}

// Validate return type as `service` is a user defined function
// TODO: should we validate outside this try/catch?
return z.instanceof(Response).parse(response);
Expand Down
12 changes: 6 additions & 6 deletions packages/vitest-pool-workers/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -67,19 +67,19 @@
"@cloudflare/workers-types": "^4.20240909.0",
"@types/node": "20.8.3",
"@types/semver": "^7.5.1",
"@vitest/runner": "1.3.x - 1.5.x",
"@vitest/snapshot": "1.3.x - 1.5.x",
"@vitest/runner": "catalog:default",
"@vitest/snapshot": "catalog:default",
"capnp-ts": "^0.7.0",
"capnpc-ts": "^0.7.0",
"ts-dedent": "^2.2.0",
"typescript": "^5.5.2",
"undici": "^5.28.4",
"vitest": "1.5.0"
"vitest": "catalog:default"
},
"peerDependencies": {
"@vitest/runner": "1.3.x - 1.5.x",
"@vitest/snapshot": "1.3.x - 1.5.x",
"vitest": "1.3.x - 1.5.x"
"@vitest/runner": "2.0.x",
"@vitest/snapshot": "2.0.x",
"vitest": "2.0.x"
Comment on lines +80 to +82
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These don't use the catalog because I'm hopeful we can make it a range in future

},
"volta": {
"extends": "../../package.json"
Expand Down
4 changes: 2 additions & 2 deletions packages/vitest-pool-workers/src/config/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ function createConfigPlugin(): Plugin<WorkersConfigPluginAPI> {
},
},
// Run after `vitest:project` plugin:
// https://github.com/vitest-dev/vitest/blob/8014614475afa880f4e583b166bb91dea5415cc6/packages/vitest/src/node/plugins/workspace.ts#L26
// https://github.com/vitest-dev/vitest/blob/v2.0.5/packages/vitest/src/node/plugins/workspace.ts#L34
config(config) {
config.resolve ??= {};
config.resolve.conditions ??= [];
Expand All @@ -140,7 +140,7 @@ function createConfigPlugin(): Plugin<WorkersConfigPluginAPI> {
ensureArrayIncludes(config.resolve.conditions, requiredConditions);

// Vitest sets this to an empty array if unset, so restore Vite defaults:
// https://github.com/vitest-dev/vitest/blob/v1.5.0/packages/vitest/src/node/plugins/index.ts#L77
// https://github.com/vitest-dev/vitest/blob/v2.0.5/packages/vitest/src/node/plugins/index.ts#L93
ensureArrayIncludes(config.resolve.mainFields, requiredMainFields);

// Apply `package.json` `browser` field remapping in SSR mode:
Expand Down
2 changes: 1 addition & 1 deletion packages/vitest-pool-workers/src/pool/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ export async function parseProjectOptions(
let workersPoolOptions = poolOptions?.workers ?? {};
try {
if (typeof workersPoolOptions === "function") {
// https://github.com/vitest-dev/vitest/blob/v1.5.0/packages/vitest/src/integrations/inject.ts
// https://github.com/vitest-dev/vitest/blob/v2.0.5/packages/vitest/src/integrations/inject.ts
const inject = <K extends keyof ProvidedContext>(
key: K
): ProvidedContext[K] => {
Expand Down
43 changes: 38 additions & 5 deletions packages/vitest-pool-workers/src/pool/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,21 +56,45 @@ import type {
import type { Readable } from "node:stream";
import type { MessagePort } from "node:worker_threads";
import type {
ResolvedConfig,
RunnerRPC,
RuntimeRPC,
SerializedConfig,
WorkerContext,
} from "vitest";
import type { ProcessPool, Vitest, WorkspaceProject } from "vitest/node";

// https://github.com/vitest-dev/vitest/blob/v1.5.0/packages/vite-node/src/client.ts#L386
interface SerializedOptions {
main?: string;
durableObjectBindingDesignators?: Map<
string /* bound name */,
DurableObjectDesignator
>;
isolatedStorage?: boolean;
}

// https://github.com/vitest-dev/vitest/blob/v2.0.5/packages/vite-node/src/client.ts#L468
declare const __vite_ssr_import__: unknown;
assert(
typeof __vite_ssr_import__ === "undefined",
"Expected `@cloudflare/vitest-pool-workers` not to be transformed by Vite"
);

function structuredSerializableStringify(value: unknown): string {
// Vitest v2+ sends a sourcemap to it's runner, which we can't serialise currently
// Deleting it doesn't seem to cause any problems, and error stack traces etc...
// still seem to work
// TODO: Figure out how to serialise SourceMap instances
if (
value &&
typeof value === "object" &&
"r" in value &&
value.r &&
typeof value.r === "object" &&
"map" in value.r &&
value.r.map
) {
delete value.r.map;
}
return devalue.stringify(value, structuredSerializableReducers);
}
function structuredSerializableParse(value: string): unknown {
Expand Down Expand Up @@ -663,7 +687,7 @@ async function runTests(
mf: Miniflare,
workerName: string,
project: Project,
config: ResolvedConfig,
config: SerializedConfig,
files: string[],
invalidates: string[] = []
) {
Expand Down Expand Up @@ -740,7 +764,9 @@ async function runTests(
const rules = project.options.miniflare?.modulesRules;
const compiledRules = compileModuleRules(rules ?? []);

const localRpcFunctions = createMethodsRPC(project.project);
const localRpcFunctions = createMethodsRPC(project.project, {
cacheFs: false,
});
const patchedLocalRpcFunctions: RuntimeRPC = {
...localRpcFunctions,
async fetch(...args) {
Expand Down Expand Up @@ -953,6 +979,7 @@ export default function (ctx: Vitest): ProcessPool {
// serialisable. `getSerializableConfig()` may also return references to
// the same objects, so override it with a new object.
config.poolOptions = {
// @ts-expect-error Vitest provides no way to extend this type
threads: {
// Allow workers to be re-used by removing the isolation requirement
isolate: false,
Expand All @@ -974,7 +1001,7 @@ export default function (ctx: Vitest): ProcessPool {
// project, so we know whether to call out to the loopback service
// to push/pop the storage stack between tests.
isolatedStorage: project.options.isolatedStorage,
},
} satisfies SerializedOptions,
};

const mf = await getProjectMiniflare(ctx, project);
Expand Down Expand Up @@ -1046,6 +1073,12 @@ export default function (ctx: Vitest): ProcessPool {
// assert(testModule && thingModule);
// thingModule.importers.add(testModule);
},
async collectTests(_specs, _invalidates) {
// TODO: This is a new API introduced in Vitest v2+ which we should consider supporting at some point
throw new Error(
"The Cloudflare Workers Vitest integration does not support the `.collect()` or `vitest list` APIs"
);
},
async close() {
// `close()` will be called when shutting down Vitest or updating config
log.debug("Shutting down runtimes...");
Expand Down
14 changes: 13 additions & 1 deletion packages/vitest-pool-workers/src/pool/module-fallback.ts
Original file line number Diff line number Diff line change
Expand Up @@ -305,11 +305,23 @@ async function viteResolve(
if (workerdBuiltinModules.has(id)) {
return `/${id}`;
}
if (id.startsWith("node:")) {
throw new Error("Not found");
}

id = `node:${id}`;
if (workerdBuiltinModules.has(id)) {
return `/${id}`;
}
throw new Error("Not found");

// If we get this far, we have something that:
// - looks like a built-in node module but wasn't imported with a `node:` prefix
// - and isn't provided by workerd natively
// In that case, _try_ and load the identifier with a `node:` prefix.
// This will potentially load one of the Node.js polyfills provided by `vitest-pool-workers`
// Note: User imports should never get here! This is only meant to cater for Vitest internals
// (Specifically, the "tinyrainbow" module imports `node:tty` as `tty`)
return id;
}
return resolved.id;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
import { VitestTestRunner } from "vitest/runners";
import workerdUnsafe from "workerd:unsafe";
import type { CancelReason, Suite, Test } from "@vitest/runner";
import type { ResolvedConfig, WorkerGlobalState, WorkerRPC } from "vitest";
import type { SerializedConfig, WorkerGlobalState, WorkerRPC } from "vitest";

// When `DEBUG` is `true`, runner operations will be logged and slowed down
// TODO(soon): remove this
Expand Down Expand Up @@ -114,7 +114,7 @@ export default class WorkersTestRunner extends VitestTestRunner {
readonly state: WorkerGlobalState;
readonly isolatedStorage: boolean;

constructor(config: ResolvedConfig) {
constructor(config: SerializedConfig) {
super(config);

// @ts-expect-error `this.workerState` has "private" access, how quaint :D
Expand Down
2 changes: 1 addition & 1 deletion packages/vitest-pool-workers/src/worker/lib/node/fs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,4 @@ export function statSync(_path: string) {

export const promises = {};

export default {};
export default { existsSync };
7 changes: 3 additions & 4 deletions packages/vitest-pool-workers/test/console.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,7 @@ test.skipIf(process.platform === "win32")(
});
const result = vitestDev();
await waitFor(() => {
expect(result.stdout).toMatch("stdout | index.test.ts:2:9\nglobal");
expect(result.stdout).toMatch("stdout | index.test.ts:4:10\ndescribe");
expect(result.stdout).toMatch("stdout | index.test.ts\nglobal\ndescribe");
expect(result.stdout).toMatch(
"stdout | index.test.ts > thing > does something\ntest"
);
Expand All @@ -42,10 +41,10 @@ test.skipIf(process.platform === "win32")(
`,
});
await waitFor(() => {
expect(result.stdout).toMatch("stdout | index.test.ts:2:9\nnew global");
expect(result.stdout).toMatch(
"stdout | index.test.ts:4:10\nnew describe"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be a change in the Vitest output to remove line numbers (verified by upgrading Wrangler to Vitest v2+ and seeing the change)

"stdout | index.test.ts\nnew global\nnew describe"
);

expect(result.stdout).toMatch(
"stdout | index.test.ts > new thing > does something else\nnew test"
);
Expand Down
5 changes: 4 additions & 1 deletion packages/vitest-pool-workers/test/global-setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,10 @@ export default async function ({ provide }: GlobalSetupContext) {
},
};
await fs.writeFile(packageJsonPath, JSON.stringify(packageJson));
childProcess.execSync("pnpm install", { cwd: tmpPath, stdio: "inherit" });
childProcess.execSync("pnpm install", {
cwd: tmpPath,
stdio: "inherit",
});

provide("tmpPoolInstallationPath", tmpPath);

Expand Down
2 changes: 1 addition & 1 deletion packages/workers-shared/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
"rimraf": "^6.0.1",
"toucan-js": "^3.3.1",
"typescript": "^5.5.4",
"vitest": "1.5.0"
"vitest": "catalog:default"
},
"engines": {
"node": ">=16.7.0"
Expand Down
2 changes: 1 addition & 1 deletion packages/workers.new/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
"@types/node": "20.8.3",
"miniflare": "workspace:*",
"typescript": "^5.5.2",
"vitest": "1.5.0",
"vitest": "catalog:default",
"wrangler": "workspace:*"
},
"volta": {
Expand Down
Loading
Loading