remix-run / remix

Build Better Websites. Create modern, resilient user experiences with web fundamentals.
https://remix.run
MIT License
29k stars 2.45k forks source link

Built-in testing? #372

Closed kentcdodds closed 2 years ago

kentcdodds commented 2 years ago

Might be helpful. My idea is basically a @remix-run/test package with all the utilities.

sergiodxa commented 2 years ago

Aside of a testing setup, it would be helpful to have Jest assertions for Responses so you could test loaders and responses easily

andrewgadziksonos commented 2 years ago

Would it be possible to spin up a checklist of items that are needed to implement this? It seems like the community is eager to get this in and with a checklist we could help out!

so you could test losers and responses easily

I giggled at this @sergiodxa πŸ˜‚ (assuming you mean loaders here)

kentcdodds commented 2 years ago

spin up a checklist of items that are needed

This is probably something I'll need to work on myself to get the package off the ground I think. But I did put a couple high-level ideas. I'm sure the package will evolve.

wladpaiva commented 2 years ago

I wonder if this render could also be used for the storybook rendering system. It would be really nice tweek things like transition and see things changing.

Also, since remix ins't going to be react only (as far I have seen), we'll probably have to split up the package

kentcdodds commented 2 years ago

That's a good point. It's possible that we'll want multiple packages. I've had to put this work on hold for a little bit while I work on higher-priority things, but I'll be thinking more on this. I do want to be the one who does this work though so thank you all for your patience.

sergiodxa commented 2 years ago

I think at least two libraries for now will make sense, one for server-side testing helpers (like the Jest assertions for response objects or helpers to test loaders/actions that throw responses), and another one with the React helpers (the render function).

That way in a future it could be more packages for more the UI part.

wladpaiva commented 2 years ago

I don't know how MSW have been doing for you guys but for me it has made my tests freaking slow. Any request to the server is taking ~1.2 seconds... On those cases where I need to POST a form to a /reaally/long/route/like/this any of subsequents GETs take more than 2s where normally it takes 20ms.

We'll probably need a way to narrow down the route where the MSW we'll be listening.

kentcdodds commented 2 years ago

@wladiston that is unusual. My MSW handlers don't take much time at all. I would suggest trying to make a minimal reproduction (https://kcd.im/repro) and (if you can) reporting that to the MSW project.

wladpaiva commented 2 years ago

Just an FYI, while I was recreating the minimal reproduction env I stumbled in the real problem. The bottleneck was that I was compiling the msw typescript files via ts-node during runtime. Using esbuild-register made it faster but not faster enough. For now, I've just changed all mock files to JS.

luanlucho commented 2 years ago

Any progress on the testing lib?

kentcdodds commented 2 years ago

I've been on break. I'll update this issue when there are updates. No need to ask 😁

kettanaito commented 2 years ago

This sounds like a great plan. Let me know if I can be of any help.

luistak commented 2 years ago

I was thinking of an API suggestion for this test package in something like:

import type { LoaderFunction } from  'remix';
import { json } from 'remix';
import { render as customRender } from '@remix-run/tests';

import SomeRoute from '../routes/some-route';

it('should render <SomeRoute /> Accordingly', async () => {
  type MockedData = any;
  const customTestLoader: LoaderFunction = async ({ request }) => {
    // you could do something with the request

    const mockedData = faker.something();

    return json<MockedData>(mockedData);
  };

  const { rerender, remixUtils } = customRender(<SomeRoute />, { loader: customTestLoader });

  // Changing a loader value
  rerender(<SomeRoute />, { loader: otherCustomLoader })
  // Or
  remixUtils.reload(otherCustomLoader)
});
kentcdodds commented 2 years ago

Thanks for sharing @luistak! I'll give that consideration when I work on this more in the future (sorry, this was a high priority until I got into that accident πŸ₯΄)

sergiodxa commented 2 years ago

I was thinking of an API suggestion for this test package in something like:

import type { LoaderFunction } from  'remix';
import { json } from 'remix';
import { render as customRender } from '@remix-run/tests';

import SomeRoute from '../routes/some-route';

it('should render <SomeRoute /> Accordingly', async () => {
  type MockedData = any;
  const customTestLoader: LoaderFunction = async ({ request }) => {
    // you could do something with the request

    const mockedData = faker.something();

    return json<MockedData>(mockedData);
  };

  const { rerender, remixUtils } = customRender(<SomeRoute />, { loader: customTestLoader });

  // Changing a loader value
  rerender(<SomeRoute />, { loader: otherCustomLoader })
  // Or
  remixUtils.reload(otherCustomLoader)
});

This looks good, also would be cool to pass an action function so you can test when useActionData returns something.

kentcdodds commented 2 years ago

WIP PR: https://github.com/remix-run/remix/pull/1939

bjufre commented 2 years ago

Nice to see some progress with this! I think it will be an amazing addition to the framework and it will help build robust and ship with more confidence.

I was thinking about something simple with some similar API like this (pseudo-code):

// setup.ts
import { createTestRequestHandler } from "@remix-run/testing";
import handleRequest from '~/entry.server'; // Or whatever is needed

// Import to extend the matchers API for interacting with the server's response.
import "@remix-run/testing/jest";

import "@testing-library/jest-dom";

// This will spin up a fake HTTP server that will use `light-my-request` to
// test the `handleRequest` function as if it were being actually called.
const { get, post } = createServer(handleRequest);

// OR
import serverBuild from '../build';
// This could initialize an `express` server in `inactive mode` (without `listen`ing)
import { createServer } from '@remix-run/testing';

// Returns some util methods wrappers around the `inject` api from `light-my-request`
const { get, post } = createTestRequestHandler(serverBuild);

export { get, post }

// index.spec.tsx
import { get, post } from './setup';
import { act, waitFor } from "@testing-library/react";
import { describe, it, expect } from "vitest";

describe("index", () => {
  it("returns successfully", async() => {
    // Returns the actual response from the server (with the headers and everything).
    // As well as a `screen` wrapper around the document (React DOM) in order to run
    // `@testing-library/react` assertions against the rendered HTML.
    // To opt-out of the `screen` wrapper one could `{ createScreen: false }` as the second
    // argument; which could be useful for resource routes!
    const { response, screen } = await get("/").end();

    // Test status
    expect(response).toHaveStatus(200);
    // Test headers
    expect(response).toHaveHeader("Content-Type", "text/html");
    expect(response.toHaveHeaders([
      { "Content-Type": "text/html" }
    ]));
    // Test cookies
    expect(response).toHaveCookie("foo", "bar"); // Individual cookie
    expect(response).toHaveCookies([             // Multiple cookies
      ["foo", "bar"]
    ]);

    // Of course this could be simple for testing against the rendered HTML,
    // what I have no idea is how we could manage to get an interactive wrapper for the React route.
    // `@testing-library/react` assertions
    expect(screen.findByText("Hello, world!")).toBeInTheDocument();
    // Interact with the DOM
    await act(() => {
      screen.getByText("Hello, world!").click();
    })
    await waitFor(() => {
      expect(screen.getByText("New, world!")).toBeInTheDocument();
    });
  });
kettanaito commented 2 years ago

It'd be great to be able to import the entire page and provide it to a testing setup function that'd take care of establishing a proper environment for that page to run:

// routes/index.jsx
export const loader = () => {}
export const action = () => {}

export default function UI() {}
// tests/index.test.jsx
import { renderPage } from '@remix-run/testing'
import * as Homepage from '../routes/index'

test('renders some state', async () => {
  await renderPage(Homepage)
})

test('displays requested user', async () => {
  await renderPage(Homepage, {
    // We can even infer things like path params 
    // from "Homepage.loader" types with TypeScript.
    params: { userId: 'abc-123' }
  })
})
// node_modules/@remix-run/testing/renderPage.js
export async function renderPage(page) {
  // Execute the "loader" first.
  const loaderParams = {
    request: new Request(...), // allow overrides on test level
    // Reuse the same logic Remix uses when parsing incoming requests.
    params: { ... }
  }
  const loaderData = await page.loader(...loaderParams)
  injectLoaderDataIntoAppContext(loaderData)

  const { default: Component } = page
  render(
    <RemixTestProvider appData={{ loaderData }}>
      <Component />
    </RemixTestProvider>
  )

  // Also establish the "action" to run whenever
  // the Component submits itself onto this route.
}

This is a very rough idea but I think it'd work nicely and will allow per-page testing. Integration tests will run in Node.js anyway so Remix can execute both loader and action without worrying about the environment. We can utilize that and perhaps circumvent having to dive into build server routes etc. for testing purposes. Each page is, effectively, a complicated function, and we can construct a proper test environment to run that function without unnecessary dependencies.

Would appreciate any feedback on this one.

kentcdodds commented 2 years ago

I like what you've proposed there @kettanaito :)

The trick is that the test provider needs to come up with values for all these __remix-prefixed variables:

image

I still need to investigate how much of that can be reasonably faked. If all of it can then I think we're good. Otherwise we'd need to somehow run a build before running the tests or something.

luistak commented 2 years ago

Nice guys, I think the biggest problem is the Route rendering along with every child route re-creating all of these states and behaviors from Remix in a test environment, if you want any help with it please let me know 😊

kentcdodds commented 2 years ago

The more I think about this more I think you won't want to test route modules in isolation. Those should be covered by e2e tests which I've already got a great solution for (and MSW has a lot to do with it πŸ˜‰).

So the isolated component-level tests that I want to support are for things like isolated complex components that use useFetcher. Once I have a good solution for a component that uses useFetcher then I think we'll be set. It's possible you may want to test a useTransition usage as well, which will be pretty similar. The thing is, lots of those usages end up redirecting you somewhere else and without having the whole app mounted that doesn't really make much sense (which is why those are probably better tested via E2E).

So I'm going to pursue what will probably be mocking Remix exports rather than faking out remix context related stuff. E2E testing with Cypress is pretty dang good, so it's hard to justify faking out the world for what's maybe a marginally better testing strategy in some situations πŸ€·β€β™‚οΈ

kettanaito commented 2 years ago

@kentcdodds, I think it'd be great to design testing infrastructure so it doesn't depend on the build step. Yeah, I understand that the context includes build-related things, as dev build is still a build. I'd say that mocking anything module-related should be safe (things like routeModules, entry, etc.)β€”anything developers don't normally interact with while writing routes in Remix. Everything else can be hard-coded with the ability to override on a per-test level (like the action function).

I think we also need to keep in mind the nested layouts feature. If we omit this and assume a page is tested in complete isolation, we may be testing a page that won't behave the same when run in a browser (i.e. a parental loader will run before this page's loader is run). But, since nesting is an implementation detail of Remix, maybe we do want to test each page in complete isolation and recommend proper E2E tests for "complete" page testing. Perhaps when isolation is a given in integration tests, it can make the decisions about mocking internals somewhat easier?

As a developer, my main goal when I want to test a Remix page is to have an environment that'd run loader and expose its value via useLoaderData(), as well as guarantee that page submits behave the same as in the browser (call the action function). I know I'm scratching the surface of all the complex scenarios people may find themselves in, but I think supporting just these two things is a great starting point.

kentcdodds commented 2 years ago

Your stated goal sounds best served by an E2E test I think.

That's why I'm mostly targeting non-route component testing for this.

kentcdodds commented 2 years ago

Alright, I've made some great progress here. I was playing around in one of the examples that uses useFetcher: https://github.com/remix-run/remix/pull/2133

It's working ok, but I basically need two things:

  1. The ability to generate the manifest at runtime in the tests... Not sure how feasible this will be...
  2. An "adapter" of sorts for Web Fetch -> MSW -> Web Fetch

The adapter bit I think would be really helpful. We want to have Remix do it's regular thing with network requests, but we want to intercept those with MSW and send them to the loader/action that's supposed to handle them. We're not going to handle navigation/transitions (if you need to test those, then use E2E testing with Cypress), but for non-navigation stuff having MSW in place to help ferry Remix's requests to the proper loader and back again would be quite helpful.

@kettanaito, what do you think about this? How challenging would it be to do this? Specifically, I'm thinking about this bit: https://github.com/remix-run/remix/pull/2133/files#diff-cb9a6c896d66e289c9e09651c45c24f83f4b17feb5b36150529feffd5185dfb3R15-R17

I'd really appreciate some feedback here. What I have so far does not require any changes in Remix, so you can use what's in that diff for your own tests in your own apps. It's just missing some stuff for handling network requests as the user interacts with your stuff. So, here's a simple version of what I have that works for the initial render:

import * as React from "react";
import {
  render as rtlRender,
  RenderOptions,
  screen,
} from "@testing-library/react";
import { RemixBrowser, useFetcher } from "../";

function LangCombobox() {
  const langs = useFetcher();

  return (
    <div>
      <input
        id="showSearch"
        name="lang"
        onChange={(e) => {
          // When the input changes, load the languages
          langs.load(`/lang-search?q=${e.target.value}`);
        }}
      />
      {langs.state === "loading" && "loading..."}
      {langs.data && langs.data.length > 0 && (
        <ul>
          {langs.data.map((lang, index) => (
            <li key={index} value={lang.alpha2}>
              {lang.name} ({lang.alpha2})
            </li>
          ))}
        </ul>
      )}
    </div>
  );
}

function render(ui: React.ReactElement, options?: RenderOptions) {
  function RootComponent() {
    return ui;
  }

  window.__remixManifest = {
    routes: {
      root: {
        hasAction: false,
        hasCatchBoundary: false,
        hasErrorBoundary: false,
        hasLoader: false,
        id: "root",
        imports: [],
        module: "",
        path: "",
      },
    },
    entry: { imports: [], module: "" },
    url: "",
    version: "",
  };
  window.__remixRouteModules = { root: { default: RootComponent } };
  window.__remixContext = {
    matches: [],
    manifest: window.__remixManifest,
    routeModules: window.__remixRouteModules,
    routeData: {},
    appState: {
      catchBoundaryRouteId: null,
      loaderBoundaryRouteId: null,
      renderBoundaryRouteId: "root",
      trackBoundaries: false,
      trackCatchBoundaries: false,
    },
  };

  function Wrapper({ children }: { children: React.ReactNode }) {
    return <RemixBrowser>{children}</RemixBrowser>;
  }
  return rtlRender(ui, {
    wrapper: Wrapper,
  });
}

test("example test", () => {
  render(<LangCombobox />);
  screen.debug();
});

You can build upon that. Totally works. Now we just gotta make it "right" πŸ˜…. Feedback welcome.

cmd-johnson commented 2 years ago

I just tried you render function and it appears to be working fine for the few tests I've written, with one notable exception: rerender doesn't work properly. It still returns the original render result for me, not the updated one. I'm now overriding its implementation as well, and so far it seems to be working fine:

function render(/* ... */) {
  /* ... the above render implementation */

  const { rerender, ...rest } = reactRender(ui, { wrapper: Wrapper, ...options })

  return {
    rerender: (ui: ReactElement) => {
      const RootComponent = () => ui
      window.__remixRouteModules.root.default = RootComponent
      rerender(ui)
    },
    ...rest,
  }
}
kentcdodds commented 2 years ago

That's interesting! I'm guessing that's happening due to this optimization: https://kentcdodds.com/blog/optimize-react-re-renders

Thanks for sharing!

BenoitAverty commented 2 years ago

Hey :wave:

Do you plan to make the testing utilities for Remix independent of jest (or any testing framework) ? I love jest, but it would be nice to be able to use the next great test framework when it comes out.

kentcdodds commented 2 years ago

Our stacks actually use vitest (which is very similar to Jest, just faster) and depending on how things shake out it may be framework specific and it may not be. The utilities above are framework-agnostic, but we're thinking about going the mocking route and you'll just need to adapt what we provide to whatever testing framework you use.

filipemir commented 2 years ago

Hey @kentcdodds. Curious about your plans for E2E tests here. I've had no issues setting Cypress up with MSW except that I have yet to find a reasonable way to override a MSW handler (which run server-side) for specific Cypress tests. Is such an override part of what you're thinking about for the "helpers for E2E tests" you mentioned above?

Context: I have been trying unsuccessfully to override handlers using cy.task(). So far I have had no luck actually accessing the mock server instance that intercepts the requests sent to the Remix backend. I may punt on this entirely if that's something you expect to ship.

kentcdodds commented 2 years ago

Yeah, I need to explore that space a bit. Suggestions welcome!

One thought is to actually just use cy.intercept() for the one-off mocking (you normally only need the one-offs for the error case anyway).

filipemir commented 2 years ago

Yeah, that's what I was thinking too. The challenge is that cy.intercept() is only able to intercept requests made client-side, so any requests made server-side for the first render wouldn't be able to be intercepted that way. Otherwise, that solution should work fine.

I'll let you know if I figure out a way to do this with cy.task(). I may reach out to the msw and cypress folks as well to see if they have any thoughts. Thanks for the response.

kettanaito commented 2 years ago

Hey, @kentcdodds. Sorry for the late reply.

Did I get it correctly that your intention is to use MSW to intercept requests to loaders/actions that a test makes?

I'm afraid I don't know Remix internals much to judge how feasible this approach is. But from the look of the code you've shared, the setup seems to be fine.

Do you have some concrete obstacles for me to look into? Do you think we can help you somehow to lift off some implementation weight on the MSW's side?

wladpaiva commented 2 years ago

Honestly, I was using cypress and doing the whole MSW assertion thing similar to what @kentcdodds had implemented on his website but turnout the Cypress was too slow for me so I've ended up migrating to Playwright which feels so much better than Cypress. Now, because my tests are run in parallel, the strategy to save the request data in a json file doesn't really work. I think the whole problem with the Cypress/Playwright and MSW integration is that we initialize the MSW through the node -r param, making it part of a separate process. Cypress or Playwright don’t have any idea on what is going on with MSW.

I was thinking on building a redis-msw kinda of thing to be able to assert the requests from the testing tool but it didn't feel the right thing to do so I've just stoped doing e2e for now.

Jest have a nice way to create this kind of interception because jest is the one that actually startup the MSW. https://mswjs.io/docs/api/setup-server/use

I was thinking on building some kind of @remix-run/playwright package that initiates the remix server with the MSW. This way I could intercept the one-off requests and also assert them without the need to save it somewhere else. I'm not sure if such thing could be achieved on Cypress though.

kentcdodds commented 2 years ago

Closing this issue in favor of this discussion/proposal: https://github.com/remix-run/remix/discussions/2481

kettanaito commented 2 years ago

I think the whole problem with the Cypress/Playwright and MSW integration is that we initialize the MSW through the node -r param, making it part of a separate process.

Agree. MSW will mock requests in the process where it's initialized. That's why you don't use setupWorker in Cypress configuration but in the test/browser directly because you don't want to intercept requests in the Cypress process, you want that in the browser process.

quantuminformation commented 7 months ago

This remix run testing is so much easier to test than mocking thunks and stuff, thanks team!