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

🐛 BUG: When using SELF in an integration test with isolatedStorage=false, waitUntil()'d promises aren't waited for #6887

Closed
csaboka opened this issue Oct 3, 2024 · 2 comments
Labels
bug Something that isn't working vitest Relating to the Workers Vitest integration

Comments

@csaboka
Copy link

csaboka commented Oct 3, 2024

Which Cloudflare product(s) does this pertain to?

Workers Vitest Integration

What version(s) of the tool(s) are you using?

3.79.0 [Wrangler], 0.5.13 [vitest-pool-workers]

What version of Node are you using?

22.9.0

What operating system and version are you using?

Windows 11

Describe the Bug

Observed behavior

When running a test using vitest and setting the "isolatedStorage" option to false, the test does not wait for background tasks queued using ctx.waitUntil(). This only seems to happen when using SELF (i.e. "await SELF.fetch(...)"), not when importing your worker module manually and calling fetch() from there. Awaiting the context using "await waitOnExecutionContext(ctx)" does not make a difference, the await completes immediately without waiting for the background task. Output generated by these background tasks will be associated with the wrong test, and I have also seen some crazy-looking worker errors pop up when I try to keep calling the worker with new requests (not in the minimal reproduction, but in our actual code I tried to test).

Expected behavior

Ideally, "await SELF.fetch(...)" should not return until the request is completely processed, including background work. Barring that, there should still be a way to await completion of background work.

Steps to reproduce

  • Clone the minimal reproduction repo linked below
  • Run "npm ci"
  • Run "npm run test"
  • Note that the "responds with Hello World! (unit style)" test completes in more than a second, while the five "integration style" tests complete almost immediately. The worker is queueing some background work that sleeps for a second, so any test that completes more quickly is wrong.
  • Also note that the "done long-running code" console outputs are not reported for the test that triggered them. (This depends on the exact timing.)

Please provide a link to a minimal reproduction

https://github.com/csaboka/vitest-self-issue

Please provide any relevant error logs

No response

@csaboka csaboka added the bug Something that isn't working label Oct 3, 2024
@github-project-automation github-project-automation bot moved this to Untriaged in workers-sdk Oct 8, 2024
@emily-shen emily-shen added the vitest Relating to the Workers Vitest integration label Oct 9, 2024
@andyjessop
Copy link
Contributor

@csaboka thank you so much for reporting. I don't think this is a bug, but is instead a slight misunderstanding about the SELF.fetch call. This is actually making a fetch request to your worker, rather than calling the method directly (as it is in the unit test). Therefore both the env and the execution context that you have created is ignored.

@github-project-automation github-project-automation bot moved this from Untriaged to Done in workers-sdk Nov 4, 2024
@csaboka
Copy link
Author

csaboka commented Nov 4, 2024

@andyjessop : If this is the case, shouldn't the default example tests be updated to reflect this? The original project created by create-cloudflare passes an environment and context to fetch, but it shouldn't if they are ignored anyway. It should demonstrate the correct way of invoking the worker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something that isn't working vitest Relating to the Workers Vitest integration
Projects
Status: Done
Development

No branches or pull requests

3 participants