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 cross-process interception via setupRemoteServer #1617

Draft
wants to merge 218 commits into
base: main
Choose a base branch
from

Conversation

kettanaito
Copy link
Member

@kettanaito kettanaito commented May 12, 2023

This is an experimental feature. It's unlikely to ship before 2.0.

Intention

Introduce an API that allows one process to modify the traffic of another process. The most apparent application for this is testing server-side behaviors of a JavaScript application:

// app.js
forwardNetworkToRemote()

export const loader = async () => {
  const res = await fetch('https://example.com/resource')
}
// app.test.js
it('fetches the user server-side', async () => {
  let a = listenToRemoteNetwork(targetProcess)
  modifyNetwork(a)
  // ...render
  // ...assert
})

This is an example API. For the exact proposed API, keep reading.

This API is designed exclusively for use cases when the request-issuing process and the request-resolving process (i.e. where you run MSW) are two different processes.

Proposed API

With consideration to the existing MSW user experience, I suggest we add a setupRemoteServer() API that implements the SetupApi interface and has a similar API to setupServer. The main user-facing distinction here is that setupRemoteServer is affecting a remote process, as indicated by the name.

import { http } from 'msw'
import { setupRemoteServer } from 'msw/node'

const remote = setupRemoteServer(...initialHandlers)

// Notice: async!
beforeAll(async () => await remote.listen())
afterEach(() => remote.resetHandlers())
afterAll(async () => await remote.close())

The .listen() and .close() methods of the remote server become async since they now establish and terminate an internal server instance respectively.

Similar to the setupServer integration, it would be recommended to call setupRemoteServer once as a part of your global testing setup. Closing the WebSocket server after each test suite will have performance implications since each next test suite would wait while remote.listen() spawns that server again.

You can then operate with the remote server as you would with a regular setupServer, keeping in mind that it doesn't affect the current process (your test) but instead, any remote process that runs setupServer (your app).

it('handles user errors', () => {
  // Appending and removing request handlers is sync
  // because they are stored in the current (test) process.
  remote.use(
    http.get('/user', () => {
      return new Response(null, { status: 500 })
    })
  )

  // ...interact and assert your app.
})

By fully extending the SetupApi, the setupRemoteServer API provides the user with full network-managing capabilities. This includes defining initial and runtime request handlers, as well as observing the outgoing traffic of a remote process using the Life-cycle API (remote.events.on(event, listener)). I think this is a nice familiarity that also provides the user with more power when it comes to controlling the network.

Implementation

I've considered multiple ways of implementing this feature. Listing them below.

(Chosen) WebSocket server

The setupRemoteServer API can establish an internal WebSocket server that can route the outgoing traffic from any server-side MSW instance anywhere and deliver it to the remote server to potentially resolve.

Technically, the WebSocket server acts as a resolution point (i.e. your handlers) while the remote MSW process acts as a request supplier (similar to how the Service Worker acts in the browser).

Very roughly, this implies that the regular setupServer instances now have a fixed request handler that tries to check if any outgoing request is potentially handled by an existing remote WebSocket server:

// setupServer.js
await handleRequest(
  request,
  requestId,
  [
    // A very basic idea on how a "remote" request handler works.
    http.all('*', async ({ request }) => {
      wsServer.emit('request', serializeRequest(request))
      await wsServer.on('response', (serializedResponse) => {
        return deserializeResponse(serializedResponse)
      })
    }),
    ...this.currentHandlers,
  ]
)

Unlike request handler (i.e. function) serialization, it is perfectly fine to serialize Request and Response instances and transfer them over any message channel, like a WebSocket transport.

If no WebSocket server was found or establishing a connection with it fails within a sensible timeout period (~500ms), the setupServer instance of the app continues to operate as normal.

Alternatively, we can skip the WebSocket server lookup altogether and make it opt-in via some remote: true option on the app's side.

IPC

The test process and the app process can utilize IPC (interprocess communication) to implement a messaging protocol. Using that protocol, the app can signal back any outgoing requests and the test can try resolving them against the request handlers you defined immediately in the test.

This approach is similar to the WebSocket approach above with the exception that it relies on IPC instead of a standalone running server. With that, it also gains its biggest disadvantage: the app process must be a child process of the test process. This is not easy to guarantee. Depending on the framework's internal implementation, the user may not achieve this parent/child relationship, and the IPC implementation will not work.

Given such a demanding requirement, I've decided not to use this implementation.

Limitations

  • useRemoteServer() affects the network resolution for the entire app. This means that you cannot have multiple tests that override request handlers for the same app at the same time. I think this is more than reasonable since you know you're running 1 app instance that can only behave in a single way at a single point in time. Still, I expect users to be confused when they parallelize their E2E tests and suddenly see some network behaviors leaking across the test cases.

Concerns

  • Can we rely on a fixed local port to always be available?
  • Is it safe to introduce a WebSocket server that will be, effectively, routing HTTP messages over the local network (during tests only)?
    • Yes. If someone can intercept that WebSocket communication, they are already in your machine and can do things far worse than that.
  • Is it clear that setupRemoteServer only affects the server-side network behavior of any running application process with the server-side MSW integration? To affect the client-side network behavior from a test you have to 1) have setupWorker integration in the app; 2) set a global window.worker instance; 3) use window.worker.use() to add runtime request handlers. This stays as it is right now, no changes here.

The API is TBD and is subjected to change.

Roadmap

  • Ensure the sync server connection is awaited before the first request handler runs.
  • Introduce serialize/deserialize utilities for requests and responses (used both in the worker and in the WS sync layer now).
  • Fix listeners' memory leaks on hot updates (clean up listeners).
  • Make the WS events map type-safe
  • Rely on the internal request header when bypassing Socket IO connection requests in the rest.all() handler.
  • Handle socket timeout and errors when awaiting for the response in setupServer.
  • Support ReadableStream from the remote request handler (may consider transferring ReadableStream over the WS messages instead of ArrayBuffer, if that's allowed).
    • This may not be needed, in the end, but if we can pull off ReadableStream transfer over WebSockets that would be great.
  • Support all Life-cycle events.
  • Support setting a custom WebSocket server port number through environment variables.
  • Make the remotePort and port an implementation detail of setupRemoteServer and setupServer({ remote: true }). The developer mustn't care about those.
  • Do not spread the list of user-defined request handlers to prepend the fixed remote server handler (spreading of large lists may have performance implications).
    • Not an issue until proven otherwise; have no wish to optimize prematurely.
  • Solve the test/app catch 22 by attaching a self-replicating one-time handlers only for the first-time requests (those fetched when the testing framework pings your app).
  • Fix: failing use() test (may have something to do with the handlers management refactoring as a part of the server.boundary()).
  • Support differentiating between requests done in different Playwright workers (see this).
  • Add more tests, specifically for different response body types.
  • Consider adding setupWorker support (see feat: support cross-process interception via setupRemoteServer #1617 (comment)).

Blockers

@Phoenixmatrix
Copy link

Just saw this PR. Since we're doing playwright integration testing in Remix and want to mock our actual backend (not the Remix backend for frontend), we're kind of reimplementing a poor man's version. Looking forward to when its picked back up!

@chrisb2244
Copy link

@Phoenixmatrix I'm unsure if you're willing to try unpublished versions, but if you wanted to try out my PR at #2041 I'd be grateful for any advance feedback (@kettanaito is I think busy with various other PRs and activities currently, but if you can point out any problems or confirm that it works as you'd expect, I can make changes if needed to try and smooth the merge).

@ar4hc
Copy link

ar4hc commented Jun 13, 2024

FYI:
There is a possible workaround for this i found during my search for this problem:
Test your SvelteKit app end-to-end with Playwright and MSW

@spuxx1701
Copy link

FYI: There is a possible workaround for this i found during my search for this problem: Test your SvelteKit app end-to-end with Playwright and MSW

Glad that my post was of some help to you! 😄

@kettanaito kettanaito changed the title feat: "setupRemoteServer" API feat: add setupRemoteServer API Sep 5, 2024
@kettanaito kettanaito changed the title feat: add setupRemoteServer API feat: support cross-process interception via setupRemoteServer Sep 5, 2024
@kettanaito
Copy link
Member Author

kettanaito commented Sep 5, 2024

Updates

I've had some time to work on this. Sharing the updates below.

  • Simplified and fixed the client sync connection in setupServer. Introduced the beforeRequest async hook that allows setupServer to fire a side effect (sync server connection) before the first intercepted request is being handled.
  • RemoteRequestHandler now correctly support unhandled requests. This is done by moving the response await logic to the parse phase, and then deciding whether the remote handler is matching in predicate based on the received mocked response from the remote. This way, the remote request handler is no longer all-matching, but instead the remote process decides whether the remote handler will match at all.
  • Unified how x-msw-intention header works. Made the header name a constant, made its possible values an enum. Updates the usage code.
  • Polished the tests.

On test/app relationship conundrum

I believe I found a way to fix the test/app catch 22 problem (app depending on test to handle initial GET /, test won't run until the app responds to GET / during the testing framework pinging the app to know if it's running).

We can circumvent this problem by attaching a permissive request handler that will resolve all requests as 200 OK. This is relevant only for the ping request made by the testing framework. The framework doesn't care about the validity of your homepage, only that the app responded (i.e. is running).

We can try utilizing one-time request handlers for this. Basically, the first time any resource is loaded, it gets 200 OK as a mocked response. The subsequent loads (those happening then you visit your app in tests) are not affected.

We don't have the means to create such handlers as of now, since a one-time handler will self-terminate on the first matching resource. We'd probably need to create something like

const handlers = [
  http.all('*', ({ request }) => {
    handlers.unshift(http[request.method](request.url, () => new Response(), { once: true }))
  }
]

So a permissive handler that will prepend one-time handlers specifically for intercepted requests. Then, this permissive handler would have to self-terminate as well somehow. This will be done in the SetupServer context, so we can use this.handlerController.prepend() to prepend such handlers.

@kettanaito
Copy link
Member Author

kettanaito commented Sep 5, 2024

Before I am to ship a questionable workaround to that catch 22 problem, I will try to solve it on the ecosystem level first. I've already raised this question in the Playwright repo (microsoft/playwright#28893), that making a HEAD/GET request to ping the app server is undesirable.

I've also opened jeffbski/wait-on#163 to wait-on, which is a dependency in start-server-and-test widely used in Cypress E2E testing, to consider the same thing (switching from a HEAD/GET request to net.connect() ping). Let's see where this leads us.

@Phoenixmatrix
Copy link

I discussed it in the other PR, but do you have thoughts on how this will handle multiple Playwright workers? There's going to be multiple socket servers (running within the test workers), but only one "app server", where msw/node is running, which then needs to know which worker is making the request to handle to connect to the appropriate web socket server.

@kettanaito
Copy link
Member Author

kettanaito commented Sep 5, 2024

@Phoenixmatrix, I must've missed your comment. That's a great concern!

At the present state, there's no support for that. We need to design one.

Initial guess: I think it can be solved by using process.pid as an identifier for individual workers that Playwright spawns. The workers won't be making any requests as that would be your app, but instead there has to be some mapping between these:

// my.test.ts

test('...', async ({ page }) => {
  // 2. ...should be handled by these handlers.
  remote.use(http.get('/resource', resolver))

  // 1. Make MSW understand that any server-side requests
  // issued as a part of this below...
  await page.goto('/')
})

This will be tricky because the app is just one, while there may be multiple tests running in parallel in different workers.

Basically, we need some sort of ID to be the same between the test (the worker) and the app's runtime (a browser tab, ideally). The biggest challenge here is to achieve this while not shipping any Playwright-specific logic!

Need to think and explore this in more detail before arriving at any conclusion. If you have any thoughts, please share.

@Phoenixmatrix
Copy link

Phoenixmatrix commented Sep 5, 2024

Thanks! What we did here is having a userland implementation (since I didn't want to fork MSW), and I added ecosystem specific adapters. One to decorate the Remix context (we use Remix), one as a Wretch plugin (Wretch being the http client we use), and a Playwright helper in charge of setting a header. Then the header has the port of the socket server, playwright ensures all requests have it, Remix forwards it via context, and the http client built from context adds the header. Then my socket client looks for the header to connect to the right server.

What I was thinking was to leave those implementations details to framework authors or as an implementation detail/recipe. Then MSW only needs to do 2 things:

A) expose the createServer function (already done)
B) provide users of MSW with a predicate in the form (r: Request) => ClientConfig, where ClientConfig would be, in its simplest form, an object of the form { port: number}. Then people can create a server wherever they want, get its port, forward it along their environment following a community provided recipe, and then do something like (r) => { port: Number(r.headers["my-cool-header-with-port")) }

Then community packages could be provided to streamline this even more (I made a "msw-remix-remote-server" in our internal monorepo with all the pieces).

Is it elegant? No, not one bit.
But it works for every use case and environment I can think of, without polluting MSW with implementation specific stuff (it also solves your catch 22 with Playwright above, since you simply don't handle a request if the predicate returns a falsy value or an empty config object)

If you can figure out something better, then definitely go a different route, but the above work well in a medium sized production app right now, so I guess its the "worse case scenario if we can't figure out any better". The pid strategy does look interesting.

@kettanaito
Copy link
Member Author

kettanaito commented Sep 5, 2024

What is interesting about this issue is that it's not Playwright-specific. It's a fundamental functionality we should provide as a part of the feature itself. And yeah, it has to be framework-agnostic. Your tests are likely running against the same instance of the app so, naturally, you want some sort of isolation on a per-test basis. There are a few things we can use as an ID of the page, but the problem is that there's no easy way to associate that particular page with a test that opens it.

Even a single test suite can have different behaviors for the same request:

test('one', async () => {
  await page.goto('/')
})

test('two', async () => {
  remote.use(http.get('/resource', resolver))
  await page.goto('/')
})

test('three', async () => {
  remote.use(http.get('/resource', differentResolver))
  await page.goto('/')
})

And since the app doesn't share anything with the test's scope, we cannot use things like server.boundary() here to provide the said isolation. Tricky, tricky.

@Phoenixmatrix
Copy link

Phoenixmatrix commented Sep 5, 2024

Yup! Not only is it not Playwright specific, but even in Playwright, there might be other things (in our case, Remix and our http client) that happen in between and that may not be able to be monkeypatched. That's why I think it has to be up to the environment to forward along the required information with the request, and let the user of MSW teach it how to fetch that information back.

So for Playwright, we used:

// in beforeEach
context.setExtraHTTPHeaders({
  "x-msw-server-port": String(address.port) //address.port comes from the remoteServer instance
});

And then tweak the environment to forward that header along across all the hops (on our case the above takes care of the Remix useFetcher, but we had to add code for our http client running on the server). If its a microservice app, there could be multiple hops in multiple programming languages before hitting the Node server we're trying to mock!

What IS cool, is that the handler itself runs in the Playwright worker, so the handlers are automatically isolated from each other. So if I have state used across handlers in a single spec files, and want to share them across these, but not across other handlers in another file, it is now trivial. My handlers are isolated in their processes thanks to the remote server architecture.

It dramatically simplifies our tests.

@kettanaito
Copy link
Member Author

Your suggestion is interesting, it just flips the request flow upside-down. You cannot access a request reference in server.listen(). We can do whatever we want in the handlers though, particularly in the internal RemoteRequestHandler. I think I will take the multiple Playwright tests above as the base scenario. If we can solve that, we can likely solve any concurrency issues with this feature.

@Phoenixmatrix
Copy link

Phoenixmatrix commented Sep 5, 2024

You cannot access a request reference in server.listen()

You cannot, that's why I was suggesting a callback. Pass a callback in listen() as an option, and send THAT down to the internals to be used by the handler, to be evaluated on each request.

Alternatively, the way I handled it in our code base, is I just created a special "remoteRequestHandler" factory similar to the one in this PR and takes the callback as argument, but its up to the user of MSW to add it to their handlers array in the Node server. Then it can be configured however they want, and added wherever they want.

For us, we needed it a the beginning of our handlers because we want it to override our old static handlers when a handler is defined in Playwright. Some other people might want the opposite.

Less magical, but powerful.

@kettanaito
Copy link
Member Author

kettanaito commented Sep 5, 2024

I meant conceptually, not just implementation-wise. MSW follows a list of strict rules of what can be done where, and I'd like to keep following those rules. Sure, they make my life way harder. But as a result, everyone gets a consistent usage experience. One of such rules is that the only way to affect the network is through a request handler. I'm not in favor of userland packages violating those rules either, as I wouldn't want your MSW experience to suddenly become different if you are using a userland package.

There are fundamentally two issues here:

  1. Prevent individual tests from using the remote instance as a shared state. This can be solved with server.boundary() since Playwright tests are running in Node.js. Won't work for Cypress tests that run in the browser (no AsyncLocalStorage support there just yet).
  2. Associate certain page runtimes with certain network behaviors.

Edit: Yeah, my head spins from all of this. You do page.goto() but that hits the server first before any page is created.

@kettanaito
Copy link
Member Author

Here's one idea:

// my.test.ts
test('...', async ({ page }) => {
  // `server.use()` generates a unique ID and wraps all its runtime handlers with it.
  // You get that ID back.
  const contextId = remote.use(override)

  // Next, you can decorate any request with that ID.
  // The remaining part is that the `setupServer()` counterpart would read that ID
  // and forward it back to the `remote` to use in predicate.
  await page.goto('/', { referer: contextId })
})

// app.js
// The problem with the app, is that "GET /" that triggers the server
// won't share any of its data, like headers, with any resources the server
// has to fetch to handle the "GET /" request.

@Phoenixmatrix
Copy link

Phoenixmatrix commented Sep 5, 2024

I like that api. Though how does the counterpart knows the port of the socket servers that are available to connect to? With just the contextIds they still wouldn't know which socket servers are available, right?

Unless you went all in and have a socket -server- in the app on a known port and the remotes running in playwright would use that when you call remote.use to announce themselves (I also considered doing this via local files or named pipes in environments that support them)

PS: the hardest thing when discussing this is the terminology, haha. In my code, having a socket client running in a server handler and a socket server running in a client test makes my head hurts, and code reviewers cry.

@kettanaito
Copy link
Member Author

Though how does the counterpart knows the port of the socket servers that are available to connect to?

The socket server is the same, there's only one being spawned. The request differentiation based on the context ID is done in the response resolvers. If a resolver receives a request decorated with a context ID other than the one associated with this resolver, it ignores it. All handlers still run for all requests, but only the same-context handlers take effect, providing that context collocation.

@kettanaito
Copy link
Member Author

kettanaito commented Sep 17, 2024

Support in setupWorker

One of the community members have raised a great point: it would be nice to be able to apply handlers to a browser process as well. There is no way to pass runtime handlers from a Playwright test to the browser unless you expose both worker and http/graphql to create the handlers. You cannot serialize functions in page.evaluate().

I should consider extending setupWorker to also support the remote: true option. If provided, it will behave similarly to setupServer's handling of that option. Any intercepted client-side requests (after they arrive from the worker) will be sent over WebSocket to the remote sync server to handle first.

// your/app.ts
const worker = setupWorker({ remote: true })
// playwright.test.ts
import { http } from 'msw'

const remote = setupRemoteServer()
// ...start and stop it.

test('', async () => {
  remote.use(
    http.get('/resource', override)
  )
})

This will finally solve the DX gap in modifying client-side network behavior in E2E tests!

@niccholaspage
Copy link
Sponsor

niccholaspage commented Sep 18, 2024

I’ve went ahead and made a sample repo with Remix, Playwright, and MSW that does both server and client side data fetching from the same URL, which can be used as a test bed for this functionality: https://github.com/niccholaspage/remix-msw-playwright-sandbox

I will try to use this PR over the next couple days with it and see how it works out, and try to think through how we could achieve isolation of handler overrides per test like we’ve been chatting about.

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.