-
Notifications
You must be signed in to change notification settings - Fork 1k
[vitest-pool-workers] Workflows test support #10494
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
[vitest-pool-workers] Workflows test support #10494
Conversation
🦋 Changeset detectedLatest commit: 1adf6bf The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
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 |
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
wrangler
commit: |
eac0d1c to
a3f94a3
Compare
3c4bf14 to
a671b96
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! This is looking good so far! I don't have a deep understanding of the workflow-shared package, so it would be good for the workflow team to take a closer look at those changes.
I will give it another pass later this week 👍🏼
fixtures/vitest-pool-workers-examples/workflows/test/integration.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two top level comments ans some initial review comments:
- Is
introspectthe right name for this? I feel likeinspectormockwould be simpler and more clearly represent what this does. - We're building a lot of custom APIs here. To what extent have we considered leaning into Vitest's mocking support instead of/in addition to some of these custom mocking APIs?
fixtures/vitest-pool-workers-examples/workflows/test/integration.test.ts
Outdated
Show resolved
Hide resolved
fixtures/vitest-pool-workers-examples/workflows/test/unit.test.ts
Outdated
Show resolved
Hide resolved
| external: [ | ||
| // Node.js built-ins (handled automatically by esbuild but listed for completeness) | ||
| "node:*", | ||
| "cloudflare:*", | ||
| "workerd:*", | ||
| // Virtual/runtime modules | ||
| "__VITEST_POOL_WORKERS_DEFINES", | ||
| "__VITEST_POOL_WORKERS_USER_OBJECT", | ||
| // All npm packages (previously handled by packages: "external") | ||
| "birpc", | ||
| "cjs-module-lexer", | ||
| "devalue", | ||
| "miniflare", | ||
| "semver", | ||
| "semver/*", | ||
| "wrangler", | ||
| "zod", | ||
| "undici", | ||
| "undici/*", | ||
| // Peer dependencies | ||
| "vitest", | ||
| "vitest/*", | ||
| "@vitest/runner", | ||
| "@vitest/snapshot", | ||
| "@vitest/snapshot/*", | ||
| ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you expand on the motivation for this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We couldn't import from workflows-shared due to bundling issues, and @edmundhung helped with this workaround. We just did not want to be copying types from workflows-shared into vitest-pool-workers, but if this change is not ideal I guess we could revert into copying what we need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For context, @cloudflare/workflows-shared wasn't built or published. The vitest-pool-workers build script sets packages: "external", which marks all dependencies as external. So the only way I can see to bundle a dependency here is to remove that setting and provide an explicit external list.
| // Add Workflows Engines DOs bindings to the Runner Worker | ||
| for (const value of Object.values(runnerWorker.workflows ?? {})) { | ||
| const engineName = `${WORKFLOW_ENGINE_BINDING}${value.name.toUpperCase()}`; | ||
| runnerWorker.durableObjects[engineName] = { | ||
| className: "Engine", | ||
| unsafeScriptName: `workflows:${value.name}`, | ||
| unsafeUniqueKey: `miniflare-workflows-${value.name}`, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit worried about this strategy of connecting additional DOs to the runner worker in order to support accessing the workflow engine DO. Did we consider adding an unsafe method to a workflow binding to access the underlying engine instead? i.e. env.WORKFLOW.unsafeGetEngine()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we considered that at first and it did not work - we can't serialize DOs. So, this was the strategy that worked and we all agreed that it was ok to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just discussed with @pombosilva and we are looking into exposing some engine methods through the workflow bindings directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! Done 👍 No more Engine DO bindings to the runner worker
It was the name we decided as a team!
We are able to make the workflow instances explicitly change their behavior. Without manipulating the Engine DO (that users shouldn't even have to worry about that it existis or not), users can't make any mocks |
c51a39e to
bb01a01
Compare
db80f02 to
45b9e7a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huge kudos on this 🚀 - just a few comments below
| // This example shows how to implicitly cleanUp Workflow instances | ||
|
|
||
| // CONFIG with `using` to ensure Workflow instances cleanup: | ||
| await using introspector = await introspectWorkflow(env.MODERATOR); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking: is await using needed? I guess it symbolizes Symbol.asyncDispose - but I wonder if there's a good way to doing it "synchronously" to have better syntax/DX? Maybe the runner DO can store the list of disposes and dispose it at the end of the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edmundhung can we do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decided on not worrying about this for now. Will be an improvement to be done later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually prefer await using for stuff like this because I've found them to be more reliable than afterEach hooks (which I'm guessing would be used in the sync version proposed?)
By reliable, I mean I've had tests hang/give weird errors when doing certain things in afterEach that went away when I switched to await using
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with afterEach is that it would break with isolated storage - without isolated storage works just fine. Isolated storage makes its assertions at the end of each test and therefore we need to enforce dispose before the test ends to avoid errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL! Thank you for explaining
65c1975 to
401fb52
Compare
fixtures/vitest-pool-workers-examples/workflows/test/integration.test.ts
Outdated
Show resolved
Hide resolved
fixtures/vitest-pool-workers-examples/workflows/test/unit.test.ts
Outdated
Show resolved
Hide resolved
2e5c260 to
4ac6f44
Compare
Co-authored-by: Somhairle MacLeòid <[email protected]>
5b9e9a0 to
1adf6bf
Compare
* Add Workflow instances DOs (Engines) to vitest-pool-workers worker * Get waitUntil working * skip sleep * skip sleep works now * InstanceModifier is now inside worflows-shared with direct access to the engine * add dynamic engine binding * add forceEventTimeout test support * Strip out engine bindings from user facing env * Prevent waitUntil from crashing tests after 5s * Add mockEvent * fix: prevent forceEventTimeot from not working if a mockEvent gets called afterwards * Add mockStepResult * Allow list of sleep steps in disableSleeps() * Add mockStepError and cleanUp. Improve waitForStepResult and waitForStatus * Add forceStepTimeout * Make mockEvent not crash if instance has not been created yet * Update abort for testing * prevent the same step from being mocked multiple times * check edge cases for wairForStatus - prevents user from waiting indefinitely * Add workflows plugin name to loopback * Fix force step timeout * Make it possible to import from workflows-shared * Allow step errors and timeouts to be set and be performed in the correct order * Add introspectWorkflow API with proxies * Expose APIs with some documentation * Add workflow tests with introspection * Fix test * Remove Vite comment * Fixing documentation * fixtures READMEs updated * Add changeset * Prettier fix * Update bundle.mjs file * Put workflows-shared as a devDependency * Change waitUntil timeouts to 1 second * Fix createBatch interception from calling this.create * Add review comment suggestions * Apply suggestions from code review - changelog file Co-authored-by: Somhairle MacLeòid <[email protected]> * Add review suggestions (miniflare do plugin, dispose introspectors) * Support isolated storage and remove Engine DO bindings to runnerWorker * Update changeset * Add team review suggestions * Update fixtures example to match a simple workflow example * Remove @ts-expect-error and add some documentation * Change cleanup to dispose. Apply suggestions. --------- Co-authored-by: Sid Chatterjee <[email protected]> Co-authored-by: Somhairle MacLeòid <[email protected]>
Fixes WOR-355 (related to WOR-854, WOR-855, WOR-856, WOR-857, WOR-858, WOR-859, WOR-860, WOR-861 and WOR-862).
What:
New testing features in vitest-pool-workers:
This provides two APIs for testing Workflows with vitest-pool-workers. These are the main entry points into changing Workflow instances behavior during tests:
introspectWorkflowInstance(env.WORKFLOW, "id123"): when the instance id is known beforehand, useful for Workflow unit tests
WorkflowInstanceIntrospectorthat allows to call:modify(fn): function to where modifications are applied (calls rpc functions to workflows-shared, where changes are made to DO storage)waitFoRStepResult(step, result): waits until a step resolves into a result or rejects into an errorwaitForStatus(status): waits until the provided workflow instance reaches a given statedispose(): disposes the workflow instance, so state does not get persisted across tests using the same workflow instance (it is actually deleting the DO storage and then aborting it)introspectWorkflow(env.WORKFLOW): when the instance id is not known beforehand, useful for integration tests
WorkflowIntrospectorthat allows to call:modifyAll(fn): same asmodify(fn)but applies modifications to all workflow instances created after theintrospectWorkflowcall until the introspectorcleanUpcallget(): returns a list ofWorkflowInstanceIntrospectorof all created workflow instances after theintrospectWorkflowcalldispose(): disposes the introspector, meaning that no more Workflow calls will be intercepted, and also disposes all workflow instance introspector createdBoth APIs support disposal of their returned objects by using the
usingkeyword. This will automatically call cleanUp() at the end of the code block (at the end of the test).How:
Miniflare:
Added anunsafeScriptNameto DurableObjectsOptionsSchema of DO plugin, to allow defining the DO serviceName without the 'core:user:' prefix.workflows-shared:
vitest-pool-workers:
For each Workflow binding present in a wrangler file, added a DO namespace binding to Runner Worker with unsafeScriptName (these are the bindings to the Engines DOs)worklfows-shared(to avoid type duplication) by using explicit external list to customize what get bundledsrc/workerwith the new APIsFixtures:
Potential problems:
Changes to files after running tests:
If I successfully run some tests with these new APIs, then make a change to the test file (can as little as just adding a space) and then save it (ctrl+s), the tests will automatically run again (if the vitest interactive process on the terminal does not get killed) and they will fail.
I suspect this is due to the instance cleanUp() function that is effectively aborting the instances DOs, and whenever the tests try to run again in the same vitest process lifetime, it does not construct the DOs again.
Workerd errors appear on terminal:
I've noticed that if I try to run a test that makes an error be thrown, workerd prints it into the console. Let's say I'm doing a test where I am making a step fail (through mockStepError), since the step failed and an error was thrown, workerd will print that out. This might be normal? The tests run accordingly, but can be confusing to see those logs appearing.