-
-
Notifications
You must be signed in to change notification settings - Fork 535
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
feat: support concurrent test runs via "server.boundary" #2000
Conversation
src/node/SetupServerApi.ts
Outdated
* `server.use()`) will be scoped to this boundary only. | ||
* @param callback A function to run (e.g. a test) | ||
*/ | ||
public boundary<Fn extends (...args: Array<unknown>) => unknown>( |
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.
neat!
should every call to use
create some kind of boundary
internally - can we avoid leaking that to the end user? (sorry if this is still early)
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 and no. We can use .enterWith()
from AsyncLocalStorage
on each server.use()
, and while it would solve some use cases, it would break the other.
The problem is that relying on async storage internally makes it hard to support sequential tests. Here's an example:
const server = setupServer()
beforeAll(() => {
// This will have a different execution id...
server.use(...someHandlers)
server.listen()
})
it('test', () => {
// ...than this.
server.use(...anotherHandlers)
// So the request handling logic that needs the list
// of all handlers won't be able to access the context
// (and the handlers) introduced in "beforeAll".
fetch()
})
The issue here is that the context created by the server.use()
call in beforeAll
will be different and inaccessible by the context created by sever.use()
in the test ( they have different execution async ids).
I also have concerns that using
.enterWith()
on eachserver.use()
call creates nested execution contexts (imagine you have multiple.use()
calls in a single test), which may have negative impact on runtime performance.
It is for this reason I'm proposing an explicit server.boundary()
API. This API is effectively a wrapper around .run()
function from async storage and it doesn't create a new execution context (at least as far as I understand). In any case, since it's not bound to individual .use()
calls, you can have as many calls as you wish within a single boundary and they will remain within the same execution context.
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.
that makes sense. it's too bad that this leaks though, but I don't think that's a big deal.
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 think this is very good that it leaks. It makes a server boundary a conscious choice. Just as running tests in parallel is a conscious choice (no testing framework does it by default).
Server boundary is also incredibly powerful outside of testing too! Imagine you want to quickly fail all requests coming from a sub-tree in your app:
server.boundary(() => {
http.all('*', () => HttpResponse.error())
subtree()
})()
This will fail all network requests within subtree()
but have no effect on the network outside of this boundary. Looks like a neat debugging tool to have! (And since handlers are "transparent" by default, it makes it twice as useful)
@@ -52,7 +52,7 @@ | |||
"check:exports": "node \"./config/scripts/validate-esm.js\"", | |||
"test": "pnpm test:unit && pnpm test:node && pnpm test:browser && pnpm test:native", | |||
"test:unit": "vitest", | |||
"test:node": "vitest --config=./test/node/vitest.config.ts", | |||
"test:node": "vitest run --config=./test/node/vitest.config.ts", |
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.
Note: our integration tests in Node.js cannot be run in watch mode because we have one test that builds the library. It triggers the entire test run anew.
@@ -58,8 +58,10 @@ export class SetupWorkerApi | |||
isMockingEnabled: false, | |||
startOptions: null as any, | |||
worker: null, | |||
currentHandlers: () => { |
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 making this a getter the way it was always supposed to be.
If this API makes it, I will write a blog post about it. The more I throw different use cases at it, the more I love it! |
This looks really interesting! Recently, I have been thinking about use-cases for Personally, I mostly use MSW to mock network requests in Playwright tests. My setups use either test or worker fixtures to init and reset MSW, depending on requirements. Workers are Playwright's concept for concurrent tests, with each worker being a different process. Since tests and their handlers are already isolated through workers, I have been wondering whether or not |
@christoph-fricke, thank you so much for sharing your feedback!
As far as I remember, Playwright uses workers to parallelize test files but workers have no effect on the actual browser runtime. When you use MSW with Playwright, you enable it as a part of the browser's runtime, either in your application or in the browser directly via Playwright fixtures. With Playwright, you achieve network isolation because every tab is a separate runtime. This means that This is why |
4a1c983
to
5106393
Compare
I think it depends? You can use playwright, and mock the network from the browser with service workers (which I agree shouldn't need this). But you can also intercept network requests from the playwright node process, in which case this could be used in playwright tests too - so it depends how/where you're applying mocks in your playwright configuration |
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 really liking this. What testing/next steps do we need to push it forward?
I'm missing context on this. What is "playwright node process"? I believe Playwright has two parts:
If I understand you right, you mean you can use |
It's fully ready. I've written a blog post on this as well to introduce this change and dive a bit into its technical implementation. The next step is to wait for my wife to finish the illustration for the blog post and we can get it released! This is likely to happen next Monday. Exciting! Until then, I can use some feedback on the Server Boundary API docs. If you have a minute, Matt, could you share your opinion on that page? Is everything explained clearly? |
Yeah - you can use playwright's request interception to intercept the request from the node side instead of from the browser side. I'm not really sure why you would want to do that instead of just letting the service worker handle it - but you could do that https://playwright.dev/docs/mock |
@mattcosta7, but that's just API semantics. Regardless of what you choose to use in Playwright, the only environment where those requests actually happen is still the browser. Even with the |
Ahh - interesting, this could have been a misunderstanding on my part! |
Will do - mostly likely tomorrow morning |
No worries! The node/browser bindings can get quite confusing. Afaik, everything you access from Playwright is created just for you. All of those are bindings that communicate with the browser process. |
@kettanaito I think the docs looks good! this is a minor nit/question: I wonder if we should / need to call out that while the handlers are scoped to the boundary, any user objects defined outside of the boundary are not. This might be not-necessary, since it's basically just how scope works in the language, but I wonder if we'll receive issues like const someThing = new SomeThing()
const server = setupServer()
test('', () => {
server.boundary(() => {
server.use(
http.post('https://example.com/login', () => {
sometThing.field = 1
return new HttpResponse.json(someThing, { status: 500 })
})
)
})
expect(someThing.field).toBe(1)
})
test('', () => {
server.boundary(() => {
server.use(
http.get('https://example.com/login', () => {
return new HttpResponse.json(someThing, { status: 500 })
})
)
})
expect(someThing.field).toBeUndefined()
}) super trivial example, but run concurrently these can race easily it seems likely state could leak into boundaries this way, and it might be good to call out that users should avoid leaking state when using boundaries and concurrency I think this is generally just an indicator of how they should be setting up their tests to handle concurrency, but it might be worth warning about too. |
@mattcosta7, that's a good point. Let's omit this on the ground of being common knowledge but if we ever get an issue about someone expecting that from the server boundary, we can add a notice to the docs. The post is ready, planned to be released on Monday. 🚀 |
@kettanaito That only right for CSR apps. I prefer using Playwright's network mocking API and delegate requests to MSW as described by you both. Kinda what playwright-msw is doing, as I prefer to test against a production build with zero test related code in it. However, testing SSR apps requires mocking requests that happen in a Node.js process. A colleague and I found quite a nice solution for testing Remix apps, which fully isolates tests with a mocked network thanks to Playwright's and MSW's flexible APIs. The trick is to use MSW Node.js API to intercept requests in worker processes and create the remix server in the same worker. Playwright fixtures are ideal for achieving this. Thanks to the strong worker isolation of Playwright (OS processes), this pattern still does not need |
@christoph-fricke, makes sense. We are also exploring |
Released: v2.2.0 🎉This has been released in v2.2.0! Make sure to always update to the latest version ( Predictable release automation by @ossjs/release. |
Just FYI this fixes a bug where |
Changes
The
server.boundary()
API is a Node.js-only API that provides encapsulation to whichever given callback function by executing it within anAsyncLocalStorage
context. This API is designed to support using MSW in concurrent test runs since no changes to request handlers (e.g. usingserver.use()
) will ever leave the boundary function.server.boundary()
has its practical use outside of testing as well. You can use it in any concurrent logic that performs network request, if you wish to handle those requests differently on a per-boundary basis.Usage
You must wrap each test function in a concurrent test suite in the
server.boundary()
function to ensure proper isolation.Server boundary can also be used anywhere else in your application to affect the network only within the given function scope. For example, you can debug what network requests a subtree of your application makes:
Todos
server.boundary()
. Every behavior must be verified in tests.node:async_events
there, so a dependency on it inSetupServerApi
may throw on runtime. 4a1c983server.boundary()