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

feat: support concurrent test runs via "server.boundary" #2000

Merged
merged 9 commits into from
Feb 12, 2024

Conversation

kettanaito
Copy link
Member

@kettanaito kettanaito commented Jan 26, 2024

Changes

The server.boundary() API is a Node.js-only API that provides encapsulation to whichever given callback function by executing it within an AsyncLocalStorage context. This API is designed to support using MSW in concurrent test runs since no changes to request handlers (e.g. using server.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.

it.concurrent('one', server.boundary(async () => {
  await fetch('/resource') // 200 OK
}))

it.concurrent('two', server.boundary(async () => {
  // This request handler override will only affect the network
  // calls that happen within this server boundary. Neat!
  server.use(http.get('/resource', () => HttpResponse.error()))
  await fetch('/resource') // Failed to fetch
}))

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:

server.boundary(() => {
  http.all('*', ({ request }) => console.log(request.method, request.url))
  subtree()
})()

Todos

  • Test server.boundary(). Every behavior must be verified in tests.
  • Ensure React Native build isn't broken. I doubt there's node:async_events there, so a dependency on it in SetupServerApi may throw on runtime. 4a1c983
  • Document server.boundary()

@kettanaito kettanaito changed the title feat: support concurrent mode in nodejs feat: support concurrent mode via "server.boundary" Jan 26, 2024
* `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>(
Copy link
Contributor

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)

Copy link
Member Author

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 each server.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.

Copy link
Contributor

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.

Copy link
Member Author

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",
Copy link
Member Author

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: () => {
Copy link
Member Author

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.

@kettanaito
Copy link
Member Author

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!

@christoph-fricke
Copy link
Contributor

christoph-fricke commented Feb 1, 2024

This looks really interesting! Recently, I have been thinking about use-cases for AsyncLocalStorage, mostly using it as a dependency context mechanism. server.boundary looks like a really good use case for it. I like it!

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 server.boundary is relevant when using MSW in Playwright. Don't get me wrong - I like it and think it is useful for MSW in general. But is it needed when using MSW in Playwright? :)

@kettanaito
Copy link
Member Author

@christoph-fricke, thank you so much for sharing your feedback!

Since tests and their handlers are already isolated through workers, I have been wondering whether or not server.boundary is relevant when using MSW in Playwright

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 worker.use() in Tab 1 has no effect on the network in Tab 2 because those two runtimes of the same app has registered the same worker, and keep their own currentHandlers list in-memory (which is not shared).

This is why server.boundary() is a Node.js-specific API. The browser runtime has no such problem and there's also no AsyncLocalStorage in the browser.

@mattcosta7
Copy link
Contributor

@christoph-fricke, thank you so much for sharing your feedback!

Since tests and their handlers are already isolated through workers, I have been wondering whether or not server.boundary is relevant when using MSW in Playwright

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 worker.use() in Tab 1 has no effect on the network in Tab 2 because those two runtimes of the same app has registered the same worker, and keep their own currentHandlers list in-memory (which is not shared).

This is why server.boundary() is a Node.js-specific API. The browser runtime has no such problem and there's also no AsyncLocalStorage in the browser.

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

Copy link
Contributor

@mattcosta7 mattcosta7 left a 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?

@kettanaito
Copy link
Member Author

But you can also intercept network requests from the playwright node process

I'm missing context on this. What is "playwright node process"? I believe Playwright has two parts:

  • Node.js process, which is your test. It establishes a message channel with the browser runtime.
  • Browser process, the actual running browser runtime.

If I understand you right, you mean you can use setupServer in the Playwright test to intercept requests you make in that test... But I fail to see when you would do those requests. If you use Playwright, your tested code runs in the browser process. Otherwise, you don't really need Playwright.

@kettanaito
Copy link
Member Author

I'm really liking this. What testing/next steps do we need to push it forward?

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?

@mattcosta7
Copy link
Contributor

But you can also intercept network requests from the playwright node process

I'm missing context on this. What is "playwright node process"? I believe Playwright has two parts:

  • Node.js process, which is your test. It establishes a message channel with the browser runtime.
  • Browser process, the actual running browser runtime.

If I understand you right, you mean you can use setupServer in the Playwright test to intercept requests you make in that test... But I fail to see when you would do those requests. If you use Playwright, your tested code runs in the browser process. Otherwise, you don't really need Playwright.

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

@kettanaito
Copy link
Member Author

@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 .route() API in Playwright, no requests will happen in the Node.js process. It's just a binding that reports requests Playwright intercepts in the browser and forwards them to that binding. You wouldn't need the server boundary in this scenario.

@mattcosta7
Copy link
Contributor

@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 .route() API in Playwright, no requests will happen in the Node.js process. It's just a binding that reports requests Playwright intercepts in the browser and forwards them to that binding. You wouldn't need the server boundary in this scenario.

Ahh - interesting, this could have been a misunderstanding on my part!

@mattcosta7
Copy link
Contributor

I'm really liking this. What testing/next steps do we need to push it forward?

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?

Will do - mostly likely tomorrow morning

@kettanaito
Copy link
Member Author

Ahh - interesting, this could have been a misunderstanding on my part!

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 kettanaito changed the title feat: support concurrent mode via "server.boundary" feat: support concurrent test runs via "server.boundary" Feb 8, 2024
@mattcosta7
Copy link
Contributor

mattcosta7 commented Feb 9, 2024

@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.

@kettanaito
Copy link
Member Author

@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. 🚀

@christoph-fricke
Copy link
Contributor

@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 .route() API in Playwright, no requests will happen in the Node.js process. It's just a binding that reports requests Playwright intercepts in the browser and forwards them to that binding. You wouldn't need the server boundary in this scenario.

@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 server.boundary(). A rough POC implementation of this approach can be found here: https://github.com/luchsamapparat/remix-playwright-msw/blob/master/tests/fixture.ts

@kettanaito
Copy link
Member Author

@christoph-fricke, makes sense. We are also exploring setupRemoteServer for native server-side network interception in MSW (see #1617).

@kettanaito kettanaito merged commit 450e7bc into main Feb 12, 2024
11 checks passed
@kettanaito kettanaito deleted the feat/concurrent-runs branch February 12, 2024 09:55
@kettanaito
Copy link
Member Author

Released: v2.2.0 🎉

This has been released in v2.2.0!

Make sure to always update to the latest version (npm i msw@latest) to get the newest features and bug fixes.


Predictable release automation by @ossjs/release.

@HristoKolev
Copy link

Just FYI this fixes a bug where server.resetHandlers would act as server.use and not reset handlers. I recently updated msw (node) from 2.0.11 to 2.2.13 and that caused a bunch of tests to fail because resetHandlers started to work correctly. I tracked it down to 2.2.0 and probably this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support concurrent runs in Node.js
4 participants