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

Implement Vitest testing environment for Miniflare 3 #4795

Merged
merged 71 commits into from
Feb 16, 2024

Conversation

mrbbot
Copy link
Contributor

@mrbbot mrbbot commented Jan 19, 2024

Closes all tickets in RM-17030 and RM-17070. Ref #4490.

What this PR solves / how to test:


This PR reintroduces Miniflare 2's Vitest unit testing environment to Miniflare 3! 🎉

Miniflare 3 runs your code inside workerd, rather than Node. This means we can no longer access Workers runtime APIs directly inside Node, and instead need to run tests inside a workerd process.
We do this by implementing a Vitest Custom Pool, rather than a Test Environment. Pools can completely customise how test are run, whereas environments can only customise the global scope. Many thanks to @sheremet-va for implementing custom pools, and their continued support throughout this project.
Our pool starts Miniflare instances running the regular Vitest worker script, and uses WebSockets rather than inter-thread/process communication for message passing.

This worker script expects to run inside a Node-like environment. To support this, we've implemented a "module fallback" service that provides Node-like disk-backed module resolution for require()s and import statements in workerd. Many thanks to @jasnell for implementing the fallback service API and unsafe-eval bindings this project also depends on.

Right now, the pool only supports vitest@1.1.3. I'll upgrade to the latest Vitest version soon.

Features

🟢 Implemented, 🟡 Planned, 🔴 Not Planned

  • 🟢 Unit testing any function in a Worker
  • 🟢 Calling fetch(), scheduled() and queue() exported handlers directly
  • 🟢 Waiting until waitUntil()ed Promises have settled
  • 🟢 Integration testing of Workers via service bindings
  • 🟢 Multi-worker support
  • 🟢 Accessing bindings, with support for seeding data
  • 🟢 Isolated per-test storage
  • 🟢 Directly calling Durable Object instance methods
  • 🟢 Directly accessing Durable Object state
  • 🟢 Immediately executing Durable Object alarms
  • 🟢 Listing Durable Objects
  • 🟢 Mocking outbound fetch() requests
  • 🟡 Testing Pages functions
  • 🟡 Easily apply D1 migrations
  • Provided by Vitest:
    • 🟢 Fast re-runs with HMR
    • 🟢 Custom Reporters, Vitest UI, IDE integrations
    • 🟢 Snapshots
    • 🟢 Fake Timers
    • 🟢 Function Mocking
    • 🟡 Module Mocking
    • 🟡 Step Through Debugging of Tests
    • 🟡 Coverage
    • 🔴 Custom environments and runners inside workerd
  • 🔴 Unit testing service workers
  • 🔴 Jest support

Changes from vitest-environment-miniflare

  • Unit testing service workers is now unsupported
  • Only vitest@1.1.3 is currently supported
  • The package is now @cloudflare/vitest-pool-workers
  • environment: "miniflare" is now pool: "@cloudflare/vitest-pool-workers" in Vitest config
  • environmentOptions: { ... } is now poolOptions: { workers: { miniflare: { ... } } } in Vitest config
  • Configuration is no longer loaded from wrangler.toml by default
  • getMiniflareBindings() is now import { env } from "cloudflare:test"
  • getMiniflareDurableObjectStorage() is now import { runInDurableObject } from "cloudflare:test", then runInDurableObject(stub, (instance, state) => { doSomethingWith(state.storage); })
  • getMiniflareDurableObjectState() is now import { runInDurableObject } from "cloudflare:test", then runInDurableObject(stub, (instance, state) => { doSomethingWith(state); })
  • getMiniflareDurableObjectInstance() is now import { runInDurableObject } from "cloudflare:test", then runInDurableObject(stub, (instance) => { doSomethingWith(instance); })
  • runWithMiniflareDurableObjectGates(doSomething) is now import { runInDurableObject } from "cloudflare:test", then runInDurableObject(stub, doSomething)
  • getMiniflareFetchMock() is now import { fetchMock } from "cloudflare:test"
  • new ExecutionContext() is now import { createExecutionContext } from "cloudflare:test"
  • getMiniflareWaitUntil() is now import { getWaitUntil } from "cloudflare:test"
  • flushMiniflareDurableObjectAlarms() is now import { runDurableObjectAlarm } from "cloudflare:test", then runDurableObjectAlarm(stub)
  • getMiniflareDurableObjectIds() is now import { listDurableObjectIds } from "cloudflare:test"

Basic Config API Docs

Docs
// vitest.config.ts
import fs from "node:fs/promises";
import { defineWorkersPoolOptions, kCurrentWorker } from "@cloudflare/vitest-pool-workers/config";
import { defineConfig } from "vitest/config";

export default defineConfig({
  test: {
    pool: "@cloudflare/vitest-pool-workers",
    poolOptions: {
      workers: defineWorkersPoolOptions({
        /**
         * Entrypoint to worker run in the same isolate/context as tests.
         * Only required if `kCurrentWorker` `serviceBindings` defined,
         * or Durable Objects without explicit `scriptName` used.
         * Note this goes through Vite transforms and can be TypeScript.
         * Note also `import module from "src/index.ts"` inside tests
         * gives exactly the same `module` instance as it used internally
         * for self-service and Durable Object bindings.
         * @optional
         */
        main: "src/index.ts",
        /**
         * Enables per-test isolated storage. If enabled, any writes to storage
         * performed in a test will be undone at the end of the test. The test
         * storage environment is copied from the containing suite, meaning
         * `beforeAll()` hooks can be used to seed data. If this is disabled,
         * all tests will share the same storage.
         * TODO: decide if we want to allow this to be disabled
         * @default false
         */
        isolatedStorage: true,
        /**
         * Runs all tests in this project serially in the same worker, using
         * the same module cache. This can significantly speed up tests if
         * you've got lots of small test files.
         * @default false
         */
        singleWorker: true,
        /**
         * Called at the start of each test run with the same `env` object as
         * `import { env } from "cloudflare:test"` inside tests. This function
         * will always be called with nothing stored in any of the bindings.
         * This function may be called multiple times per test run if
         * `singleWorker` is `false` and `isolatedStorage` is `true`.
         * @optional
         */
        async setupEnvironment(env) {
          const value = await fs.readFile("value.txt", "utf8");
          await env.TEST_NAMESPACE.put("key", value);
        },
        /**
         * Miniflare `WorkerOptions` for configuring the test environment.
         * Note configuration isn't automatically loaded from `wrangler.toml`,
         * so bindings will need to be specified here.
         * @optional
         */
        miniflare: {
          // `rootPath` is implicitly set to the dirname of the `vitest.config.ts`
          // file here. All path-valued options are resolved relative to the
          // nearest `rootPath`.
          kvNamespaces: ["TEST_NAMESPACE"],
          serviceBindings: {
            // `kCurrentWorker` here binds to the `main` worker running in the
            // test runner's isolate.
            SELF: kCurrentWorker,
            // `other` binds to the auxiliary worker with `name: "other"`
            OTHER: "other",
          },
          // If no `scriptName` is specified in the Durable Object designator,
          // it binds to the `main` worker running in the test runner's isolate,
          // and allows Durable Object test helpers to be used with stubs from
          // the namespace.
          durableObjects: { OBJECT: "TestObject" }
          workers: [
            {
              rootPath: "other-src",
              name: "other",
              modules: true,
              // Resolves to `${dirname(".../vitest.config.ts")}/other-src/index.mjs`
              scriptPath: "index.mjs"
            }
          ]
        },
      }),
    },
  },
});

Basic cloudflare:test API Docs

Docs

env: CloudflareTestEnv

2nd argument passed to modules-format exported handlers. Contains bindings configured in top-level miniflare pool options.

fetchMock: import("undici").MockAgent

Declarative interface for mocking outbound fetch() requests. Deactivated by default and reset before running each test file. Only mocks fetch() requests for the current test runner worker. Auxiliary workers should mock fetch()es with the regular fetchMock/outboundService options.

runInDurableObject<O extends DurableObject, R>(stub: DurableObjectStub, callback: (instance: O, state: DurableObjectState) => R | Promise<R>): Promise<R>

Runs callback inside the Durable Object pointed-to by stub's context. Conceptually, this temporarily replaces your Durable Object's fetch() handler with callback, then sends a request to it, returning the result. This can be used to call/spy-on Durable Object instance methods or seed/get persisted data.

runDurableObjectAlarm(stub: DurableObjectStub): Promise<boolean>

Immediately runs and removes the Durable Object pointed-to by stub's alarm if one is scheduled. Returns true if an alarm ran, and false otherwise.

listDurableObjectIds(namespace: DurableObjectNamespace): Promise<DurableObjectIds>

Gets the IDs of all objects that have been created in the namespace. Respects isolatedStorage if enabled, i.e. objects created in a different test won't be returned.

createExecutionContext(): ExecutionContext

Creates an instance of ExecutionContext for use as the 3rd argument to modules-format exported handlers.

getWaitUntil<T extends unknown[]>(ctx: ExecutionContext): Promise<T>

Waits for all ExecutionContext#waitUntil()ed Promises to settle. Only accepts instances of ExecutionContext returned by createExecutionContext().

createScheduledController(options?: FetcherScheduledOptions): ScheduledController

Creates an instance of ScheduledController for use as the 1st argument to modules-format scheduled() exported handlers.

getScheduledResult(ctrl: ScheduledController, ctx: ExecutionContext): Promise<FetcherScheduledResult>

Gets the "no retry" state of the ScheduledController, and waits for all ExecutionContext#waitUntil()ed Promises to settle. Only accepts instances of ScheduledController returned by createScheduledController(), and instances of ExecutionContext returned by createExecutionContext().

createMessageBatch(queueName: string, messages: ServiceBindingQueueMessage[]): MessageBatch

Creates an instance of MessageBatch for use as the 1st argument to modules-format queue() exported handlers.

getQueueResult(batch: MessageBatch, ctx: ExecutionContext): Promise<FetcherQueueResult>

Gets the ack/retry state of messages in the MessageBatch, and waits for all ExecutionContext#waitUntil()ed Promises to settle. Only accepts instances of MessageBatch returned by createMessageBatch(), and instances of ExecutionContext returned by createExecutionContext().

Miniflare Changes

This PR includes a bunch of changes in Miniflare to support the custom Vitest pool.
These changes are contained within commits starting with [miniflare].
User facing changes to non unsafe* options/methods have changesets.

  • Add support for the module fallback service
  • Return non-WebSocket responses for failed WebSocket upgrading fetch()es
  • Export formatZodError() method
  • Add support for "sticky" blobs that aren't garbage collected
  • Add support for getting persistence paths
  • Add support for "colo-local" ephemeral Durable Objects
  • Bind this to Miniflare instance in function-valued custom services
  • Use z.input() for user facing options types
  • Allow easy service bindings to current worker with kCurrentWorker symbol
  • Add support for rootPaths
  • Fix DurableObjectStub detection when Durable Object JS RPC is enabled

Maintainer Setup Instructions

  1. Make sure you're using pnpm@8 (lower versions don't correctly handle the vitest peer-dependency requirement)
  2. Run pnpm install in the monorepo root
  3. Run pnpm build in the monorepo root
  4. Run pnpm exec vitest --version in packages/vitest-pool-workers and verify this outputs vitest/1.1.3
  5. Run pnpm test in packages/vitest-pool-workers
  6. Play around in packages/vitest-pool-workers/test

Pre-Release Setup Instructions

Clone https://github.com/mrbbot/vitest-pool-workers-prerelease-getting-started and follow the instructions in the README.

Reviewing Guidance

This is a pretty chunky PR. I've done my best to keep individual features to separate commits, but some later commits are fixups to earlier ones. Most commits have descriptions explaining what they're adding/changing. Very happy to walkthrough how everything fits together if that would be helpful too, but as an outline:

  • packages/vitest-pool-workers
    • src
      • config: entrypoint for the @cloudflare/vitest-pool-workers/config sub-export
      • mock-agent: entrypoint for cloudflare:mock-agent worker library, subset of undici required to support MockAgent
      • pool: code running inside Node.js
        • config.ts: schemas for poolOptions.workers
        • index.ts: entrypoint for the @cloudflare/vitest-pool-workers package
        • loopback.ts: custom service for implementing snapshot storage/stacked storage
        • module-fallback.ts: fallback service implementation providing Node-like import/require module resolution
      • worker: code running inside workerd
        • lib: polyfills for modules unsupported by workerd, files in this directory map directly to imports inside workerd (i.e. lib/cloudflare/test.ts becomes cloudflare:test)
        • durable-objects.ts: Durable Object test helpers (runInDurableObject(), runDurableObjectAlarm())
        • env.ts: environment test helpers (env)
        • events.ts: exported handler test helpers (createExecutionContext(), getWaitUntil(), createScheduledController(), getScheduledResult(), createMessageBatch(), getQueueResult())
        • fetch-mock.ts: fetch mocking test helpers (fetchMock)
        • import.ts: code for importing arbitrary modules in a Worker using Vite's pipeline, mainly used for importing the configured main module
        • index.ts: entrypoint for the Worker that gets executed inside workerd to run tests, contains the Durable Object that actually imports the Vitest runner to run tests
    • test
      • basic: tests built-in Vitest functionality
      • kv: tests basic functionality of all features
      • stacked: stress test for isolated storage
      • cloudflare-test.d.ts: user facing cloudflare:test types, should probably be generated automatically

Right now, we have a few example tests in packages/vitest-pool-workers/test. I'm planning to add a wider suite of tests here soon, in addition to some "meta-tests" that verify things like snapshots work correctly (tests fail when snapshot is incorrect, snapshots can be updated, etc.).

Changesets have only been included for the miniflare package. We're not quite ready to release @cloudflare/vitest-pool-workers yet, so it's been marked private: true, and won't be included in the regular release process.


Author has addressed the following:

  • Tests
    • Included
    • Not necessary because:
  • Changeset (Changeset guidelines)
    • Included (for Miniflare changes only, not quite ready to release @cloudflare/vitest-pool-workers yet)
    • Not necessary because:
  • Associated docs
    • Issue(s)/PR(s):
    • Not necessary because: not quite ready to release @cloudflare/vitest-pool-workers yet, upcoming meeting with PCX to discuss

Note for PR author:

We want to celebrate and highlight awesome PR review! If you think this PR received a particularly high-caliber review, please assign it the label highlight pr review so future reviewers can take inspiration and learn from it.

@mrbbot mrbbot requested a review from a team as a code owner January 19, 2024 19:15
Copy link

changeset-bot bot commented Jan 19, 2024

🦋 Changeset detected

Latest commit: d8be18e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
miniflare Minor
@cloudflare/pages-shared Patch
@cloudflare/vitest-pool-workers Patch
wrangler Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Jan 23, 2024

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7933607154/npm-package-wrangler-4795

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/4795/npm-package-wrangler-4795

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7933607154/npm-package-wrangler-4795 dev path/to/script.js
Additional artifacts:
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7933607154/npm-package-create-cloudflare-4795 --no-auto-update
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7933607154/npm-package-cloudflare-kv-asset-handler-4795
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7933607154/npm-package-miniflare-4795
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7933607154/npm-package-cloudflare-pages-shared-4795
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7933607154/npm-package-cloudflare-vitest-pool-workers-4795

Note that these links will no longer work once the GitHub Actions artifact expires.


wrangler@3.28.2 includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20240129.2
workerd 1.20240129.0 1.20240129.0
workerd --version 1.20240129.0 2024-01-29

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

@mrbbot mrbbot force-pushed the bcoll/vitest-pool-workers branch from 5d47b27 to 1038bee Compare January 23, 2024 17:36
Copy link

codecov bot commented Jan 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (86d94ff) 70.33% compared to head (d8be18e) 70.39%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4795      +/-   ##
==========================================
+ Coverage   70.33%   70.39%   +0.06%     
==========================================
  Files         297      297              
  Lines       15458    15458              
  Branches     3966     3966              
==========================================
+ Hits        10872    10882      +10     
+ Misses       4586     4576      -10     

see 7 files with indirect coverage changes

.prettierignore Outdated
@@ -37,3 +37,4 @@ fixtures/**/dist/**

# Miniflare shouldn't be formatted with the root `prettier` version
packages/miniflare
packages/vitest-pool-workers
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When developing this, my IDE was configured to use Prettier 3 rather than the monorepo's Prettier 2. I'll need to reformat this code using the monorepo's version/config before landing. To make it easier to rebase this PR though, I've temporarily excluded vitest-pool-workers, so we don't end up with a bunch of conflicts from fixups.

@mrbbot mrbbot force-pushed the bcoll/vitest-pool-workers branch from 1038bee to 98dd95e Compare January 24, 2024 17:23
.changeset/poor-carrots-bathe.md Outdated Show resolved Hide resolved
packages/miniflare/src/runtime/config/workerd.capnp Outdated Show resolved Hide resolved
packages/miniflare/src/shared/types.ts Show resolved Hide resolved
import type { WorkersProjectOptions } from "../pool/config";
import type { Awaitable, inject } from "vitest";

// Vitest will call `structuredClone()` to verify data is serialisable.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally, we'd just drop support for Node 16, but our CI currently runs all tests with that. We could split the Vitest pool tests into a separate job maybe?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would be up for splitting the CI tests out and running in a more modern Node.js to avoid this polyfill.

packages/vitest-pool-workers/test/basic/vitest.config.ts Outdated Show resolved Hide resolved
@petebacondarwin
Copy link
Contributor

In the "Maintainer Setup Instructions" I see:

  1. Run pnpm build in packages/vitest-pool-workers (will want to add this to our Turborepo setup)

But from what I can tell running pnpm build already builds this package. Is this instruction outdated?

@petebacondarwin
Copy link
Contributor

I note that for this step:

  1. Run pnpm exec vitest --version in packages/vitest-pool-workers and verify this outputs vitest/1.1.3

If you run that command in the root of the monorepo you will get

vitest/0.34.6

Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

OK I have read through all the vitest-pool-workers files (except for the example tests) and I haven't yet looked at the miniflare changes.
But thought it was worth dumping this first set of comments.
Nothing serious as far as I can tell so far.
I love the use of assertions throughout.

packages/vitest-pool-workers/package.json Outdated Show resolved Hide resolved
packages/vitest-pool-workers/package.json Show resolved Hide resolved
packages/vitest-pool-workers/test/cloudflare-test.d.ts Outdated Show resolved Hide resolved
import type { WorkersProjectOptions } from "../pool/config";
import type { Awaitable, inject } from "vitest";

// Vitest will call `structuredClone()` to verify data is serialisable.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would be up for splitting the CI tests out and running in a more modern Node.js to avoid this polyfill.

packages/vitest-pool-workers/src/worker/index.ts Outdated Show resolved Hide resolved
@@ -9,7 +9,12 @@ import { zAwaitable } from "../../shared";

// Zod validators for types in runtime/config/workerd.ts.
// All options should be optional except where specifically stated.
// TODO: autogenerate these with runtime/config/workerd.ts from capnp
// TODO: autogenerate these with runtime/config/workerd.ts from cap
Copy link
Contributor

Choose a reason for hiding this comment

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

capnp -> cap ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Undone in 709fd43

.changeset/poor-carrots-bathe.md Outdated Show resolved Hide resolved
.changeset/rotten-suns-nail.md Show resolved Hide resolved
.changeset/sharp-mayflies-flow.md Show resolved Hide resolved
}
}

export default function (ctx: Vitest): ProcessPool {
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I can tell from the docs, this function could potentially be called multiple times if the server configuration changes (https://vitest.dev/advanced/pool.html#api). While I don't know how often that's likely to happen, it feels like it would be safer to initialise the spec cache here rather than at the top level, to ensure it's cleared on re-calls?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was able to reproduce an error with the pre-release, strange it doesn't watch the config in dev:
Screenshot 2024-01-31 at 20 11 40
Will get this fixed 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in ba86a74. It looks like Vitest will call this function on re-runs too, in which case we do want to reuse instances. It looks like close() is only called when config changes/exiting though, not on re-runs, so updated that method to reset the instances.

workersProject.testFiles.add(testFile);

parsedProjectOptions.add(project);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not entirely sure what this block of code is doing. From the comments, it seems like there's a cache for projects across re-runs, but I'm not sure how parsedProjectOptions plays into that. Is it a cache for project options per run, in case a project is specified in multiple specs? What's the purpose of a top-level cache if project, options, and relativePath are re-generated per run?

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 makes sure...

  1. allProjects contains all workersProjects
  2. Each workersProject workers project has up-to-date information for each call to runTests
  3. We only parse project options once per project (this is what parsedProjectOptions is for). Note specs may contain the same project multiple times (e.g. [[PROJECT_1, "a.spec.ts"], [PROJECT_1, "b.spec.ts"], [PROJECT_2, "c.spec.ts"], ...]).

parsedProjectOptions isn't used beyond this for loop. Note also workersProjects are of type Project which includes an optional mf property for Miniflare instances that can be reused between runTests calls.

Comment on lines 643 to 650
const filesByProject = groupBy(
specs,
([project]) => project,
([, file]) => file
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, but this may benefit clarity wise from just being a for loop:

Suggested change
const filesByProject = groupBy(
specs,
([project]) => project,
([, file]) => file
);
const filesByProject = new Map()
for ([project, file] of specs) {
let projectEntry = filesByProject.get(project) ?? []
projectEntry.push(file)
filesByProject.set(project, projectEntry)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 292cb60

// serialisable. `getSerializableConfig()` may also return references to
// the same objects, so override it with a new object.
config.poolOptions = {
threads: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we error if a user has provided threads.isolate = true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we're setting this here because we're using the same node:worker_threads worker script from the threads pool, and we want to disable its isolation behaviour. We're not using the threads pool, it's just the script is meant for it, so reads its config from poolOptions.threads.

Users can use multiple pools in the same project (see https://vitest.dev/config/#poolmatchglobs-0-29-4), and we don't want to prohibit them from using options of other pools.

@mrbbot mrbbot force-pushed the bcoll/vitest-pool-workers branch from 2f43dfc to 27bfdb5 Compare January 31, 2024 15:05
@mrbbot
Copy link
Contributor Author

mrbbot commented Jan 31, 2024

But from what I can tell running pnpm build already builds this package. Is this instruction outdated?

Yep, this is outdated. I've updated the description to omit this.

@mrbbot
Copy link
Contributor Author

mrbbot commented Feb 8, 2024

I think I've now fixed up all your comments so far @petebacondarwin and @penalosa, except:

  • I haven't split the testing jobs into a separate Node 18 workflow. Given the recent discussions about Node version support, it seems like we might be doing this soon anyways.
  • @penalosa I haven't added anything to prevent polyfills being imported in worker source code/tests. I had a think about how we could do this, and I think it's more complicated than it seems. We could block them being imported directly in worker source/tests, but we wouldn't be able to block importing them from a transitive npm dependency without recording the import chain. But then how does that work if the first module to import it is Vitest? How could we block subsequent imports, since modules will only be loaded once. Maybe some level of blocking is better than none?
  • Still need to reformat everything once we're ready to land this.

Vitest will dispose of the pool and recreate it when the Vitest config
changes. This was causing `Miniflare` instances to be disposed, then
reused when the pool was recreated. This change moves the project
cache from the module-level to a "pool-level" local variable.
Our mock agent implementation depends on `undici` internals. This
change ensures we pin our version, so we don't accidentally upgrade
to an incompatible version.
…ices

Switch to passing `Miniflare` instance as parameter instead of `this`
Update not-implemented messages
Adds a `listDurableObjectIds()` test helper that behaves similarly to
`getMiniflareDurableObjectIds()` from Miniflare 2's test environments.
Importantly, this change makes the Zod output type of `hyperdrives`
assignable to the input type. This means we can pass the output of
parsing options schemas back to the `new Miniflare()` constructor.
Also adds a few more `hyperdrives` tests to verify this doesn't change
behaviour.
Exposing `kCurrentWorker` to users for integration testing wasn't very
nice. This is a common use case so this change ensures it's always
bound and adds it directly to the `cloudflare:test` module.
This function aims to allow users to wait on side-effecting
`waitUntil`s, then assert on their effects. `workerd` ignores the
resolved value of `Promise`s, so exposing these may make it trickier
to change the implementation of this function in the future.
`durableObjectBindingDesignators` includes the same and more
information, so there's no need to pass this.
This makes it explicit which flags we're enabling, and helps avoid
behaviour differences when running `wrangler dev` or deployed code.
Clarify `StackedStorageState` is per `Miniflare`-instance
@mrbbot mrbbot force-pushed the bcoll/vitest-pool-workers branch from 49b9299 to d8be18e Compare February 16, 2024 16:45
@mrbbot mrbbot merged commit 027f971 into main Feb 16, 2024
17 checks passed
@mrbbot mrbbot deleted the bcoll/vitest-pool-workers branch February 16, 2024 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants