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: testing helpers #4539

Merged
merged 38 commits into from
Dec 2, 2022
Merged

feat: testing helpers #4539

merged 38 commits into from
Dec 2, 2022

Conversation

mcansh
Copy link
Collaborator

@mcansh mcansh commented Nov 7, 2022

shout out to @jrestall for the initial plumbing, we were both working on this concurrently and came up with similar implementations for todays Remix on React Router 6.3 - I'm currently working on switching this over to use Remix on RR@6.4

Closes: #4488

  • Docs
  • Tests

Testing Strategy:

@changeset-bot
Copy link

changeset-bot bot commented Nov 7, 2022

🦋 Changeset detected

Latest commit: 65bda9d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 17 packages
Name Type
remix Patch
@remix-run/react Patch
@remix-run/serve Patch
@remix-run/server-runtime Patch
@remix-run/testing Patch
@remix-run/dev Patch
@remix-run/cloudflare Patch
@remix-run/deno Patch
@remix-run/node Patch
create-remix Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/architect Patch
@remix-run/express Patch
@remix-run/netlify Patch
@remix-run/vercel Patch
@remix-run/eslint-config Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@mcansh mcansh changed the base branch from dev to release-experimental November 8, 2022 18:28
@mcansh mcansh changed the base branch from release-experimental to document-request-experimental November 10, 2022 17:44
@ryanflorence ryanflorence mentioned this pull request Nov 14, 2022
4 tasks
@mcansh mcansh linked an issue Nov 14, 2022 that may be closed by this pull request
4 tasks
Co-authored-by: James Restall <james.restall@gmail.com>

Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
@mcansh mcansh changed the base branch from document-request-experimental to dev November 16, 2022 00:40
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
@mcansh mcansh marked this pull request as ready for review November 22, 2022 19:28
@ryanflorence
Copy link
Member

This is awesome! I made an experimental release to play with it, especially to see if the types and stuff are working well, etc.

A couple of things to look into before we merge:

Types

RouteObject is requiring element but that shouldn't be required, like a resource routes. Pretty much every key on a route object should be optional.

{
  path: "/users",
  loader: () => {
    return []
  },
},

^ That should be fine but TS yells at you for not having an element. I don't know if this is related to this work, or something in @remix-run/router that needs to lighten up.

FormData problems

When testing in vitest with happydom or jsdom we have problems. I'm not completely sure what's going on but this doesn't work:

      <fetcher.Form action="/users" method="get">
        <input
          type="text"
          name="search"
          onChange={(event) => {
            // this works in browsers but not happydom/jsdom
            fetcher.submit(event.target.form);
          }}
        />
      </fetcher.Form>

Both happydom and jsdom fail right here in our fork of web-std-io

  • happydom fails our isFormElement check because it returns [object Object] instead of [object HTMLFormElement]. We should open an issue with them.
  • jsdom fails because we we don't support passing in a form element to new FormData(form).

Rather than try to support this in our FormData implementation, I think we just need to figure out why the testing environment is even using our backend polyfill for FormData in the first place? It should be using the FormData from happydom/jsdom!

I use a stock Indie Stack, so we can look there to see what's going on.

Here's a repo showing the problem: https://github.com/ryanflorence/tmp-form-data-error

git clone
npm i
npm test
💣

While I know this problem isn't necessarily part of the work in this PR, I'd like to figure this out and fix the test harness in our stacks to make it work before we release so we have the solution from the start.

Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
@mcansh
Copy link
Collaborator Author

mcansh commented Nov 29, 2022

RouteObject is requiring element but that shouldn't be required, like a resource routes. Pretty much every key on a route object should be optional.

that's the types in the stub, AgnosticRoutes don't have element, but once RR@6.4 is in here we can re-use that type. for now it's fixed here 2201c47

happydom fails our isFormElement check because it returns [object Object] instead of [object HTMLFormElement].

strange https://github.com/capricorn86/happy-dom/blob/9c0f9b7a7b1363297cf3f4cb2199a430e2a74d1a/packages/happy-dom/test/nodes/html-form-element/HTMLFormElement.test.ts#L16-L20, but yeah looking into why it's not using jsdom's FormData (happydom doesn't have a complete one)

Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
@mcansh
Copy link
Collaborator Author

mcansh commented Nov 29, 2022

we're running installGlobals in ./test/setup-test-env.ts

using this works, but i dont think we should need to...?

import { installGlobals } from "@remix-run/node";
import "@testing-library/jest-dom";
import { JSDOM } from "jsdom";

installGlobals()

const jsdom = new JSDOM(`<!doctype html>`);
globalThis.FormData = jsdom.window.FormData;

@mcansh
Copy link
Collaborator Author

mcansh commented Nov 29, 2022

removing the installGlobals causes a Request is not defined error, perhaps in @remix-run/testing we have our own installGlobals?

import { installGlobals as installNodeGlobals } from "@remix-run/node";

export async function installGlobals(framework: 'jsdom' | 'happy-dom' = 'jsdom') {
  installNodeGlobals();

  if (framework === 'happy-dom') {
    throw new Error(
      `happy-dom is not currently supported as it doesn't have a FormData implementation`
    );
  }

  let JSDOM = await import("jsdom").catch(() => {
    throw new Error(
      `Could not locate jsdom. Please verify you have it installed.`
    );
  });

  let jsdom = new JSDOM.JSDOM(`<!doctype html>`);
  globalThis.FormData = jsdom.window.FormData;
}

Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
@ryanflorence
Copy link
Member

@mcansh yeah let's do that, and if we think of anything better in the meantime, we'll change it when we remove the unstable prefix.

import { unstable_installGlobals } from "@remix-run/testing";
unstable_installGlobals("jsdom");

@mcansh
Copy link
Collaborator Author

mcansh commented Nov 30, 2022

sounds good! thats the exact public api i went with for it haha

@ryanflorence
Copy link
Member

Wait, what if you're on node 18/19 with jsdom, do you even need to install any globals?

@jrestall
Copy link
Contributor

I've found that when using the jsdom environment on vitest as of version 0.25.3 the FormData workaround is no longer required due to this (vitest-dev/vitest#2290) recent fix. Ryan your repro branch is using the older 0.24.5.

This is great as jsdom setup is quite slow and importing/creating JSDOM again in each test setup after vitest already did it here would likely slow test setup. I haven't tested jest's jsdom environment though.

Wait, what if you're on node 18/19 with jsdom, do you even need to install any globals?

I've found calling installGlobals in a jsdom environment breaks the dom tests due to overwriting FormData. So I have to run the node and jsdom tests separately which isn't ideal. I'd prefer if vitest passed the current environment to the setup files then I could install globals only when running in a node env for action/loader tests.

// maybe one day...
if (globals.environment === "node") installGlobals();

@ryanflorence ryanflorence merged commit ee8d4ac into dev Dec 2, 2022
@ryanflorence ryanflorence deleted the logan/testing-helpers branch December 2, 2022 21:29
@github-actions github-actions bot added the awaiting release This issue has been fixed and will be released soon label Dec 2, 2022
pcattori added a commit that referenced this pull request Dec 16, 2022
* fix(remix-dev): convert `config.appDirectory` to relative unix path (#4709)

* fix(remix-dev): convert appDirectory to unix style for fast-glob

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* chore: relative path

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* chore: add test

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* fix test

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* fix: typo

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* chore: update test

Signed-off-by: Logan McAnsh <logan@mcan.sh>

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* chore: add changeset for #4709 (#4718)

* ci: add typechecking for deno (#4715)

* ci: add typechecking for deno

* ci: install deno for integration tests

* chore: format

* chore: format

* fix: Firefox LiveReload (#4725)

Firefox infinitely reloads the page as long as `<LiveReload>` is rendering.

Closes #4692

* fix(remix-dev): allow defining multiple routes for the same route module file (#3970)

* Allow multiple routes for same route module

* Update packages/remix-dev/config/routes.ts

Co-authored-by: Andrew Leedham <AndrewLeedham@outlook.com>

* Update routes.ts

- Better name for automated ID variable;
- Small adjust in `id` option comment;

* - Removing redundant IF

* Update routes.ts

Revert complex custom ID in routes

* Non unique custom routes ID error and test

* Update assets.ts

Trying to solve a conflict

* Revert "Update assets.ts"

This reverts commit 2064c57.

* Error on collisions with non-custom routeIds

* Create big-spoons-grab.md

Co-authored-by: Andrew Leedham <AndrewLeedham@outlook.com>
Co-authored-by: Matt Brophy <matt@brophy.org>

* feat: Allow pass-through script props in `ScrollRestoration` (#2879)

* ci(nightly): add deno for typechecking deno files (#4738)

* ci: fix race condition writing globals.d.ts shim (#4717)

Co-authored-by: Chance Strickland <hi@chance.dev>

* chore: bump @playwright/test to latest (#4749)

Signed-off-by: Logan McAnsh <logan@mcan.sh>

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* Fix 4199: TypedResponse allows incompatible types (#4734)

* Fixes #4199: Do not allow assignment of incompatible TypedResponses

* Add myself to contributors.yml

* Create light-sheep-give.md

* slight changeset tweak

* additional changeset tweaks

Co-authored-by: Pedro Cattori <pcattori@gmail.com>

* chore: format

* test: add transition integration tests (#4739)

test: useTransition to wait for states

This approach could probably be applied across other flakey tests and could also be documented as a good approach of if there is user-feedback for a specific action when running integration tests.

* feat: testing helpers (#4539)

Co-authored-by: James Restall <james.restall@gmail.com>
Signed-off-by: Logan McAnsh <logan@mcan.sh>

* chore: format

* chore(remix-testing): update dependencies (#4756)

* fix(remix-testing): fix deps (#4757)

* Fix deps for new remix-testing package

* fix lint

* Remove ENABLE_REMIX_ROUTER flags (#4732)

* chore: add `@remix-run/testing` to changesets (#4781)

* chore: add `@remix-run/testing` to changesets

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* chore: update `ADDING_A_PACKAGE.md`

Signed-off-by: Logan McAnsh <logan@mcan.sh>

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* refactor(remix-react): upgrade Remix to `react-router-dom@6.4` (non-data-router) and drop `history` (#4731)

* Bump remix to react-router-dom@6.4.4 (#4668)

Co-authored-by: Mehdi Achour <machour@gmail.com>

* Bump remix to RR 6.4.4 and drop history dependency (#4702)

Co-authored-by: Mehdi Achour <machour@gmail.com>

* ci(nightly): move git operations after build (#4797)

* ci(nightly): move git operations after build

when adding deno typechecking (#4715), nightly now refers to the version we just calculated which hasnt been published when trying to build and typecheck

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* ci: update tmp branch name

Signed-off-by: Logan McAnsh <logan@mcan.sh>

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* perf(remix-dev): Optimize `parentRouteId` lookup in `defineConventionalRoutes` (#4538)

* Use object for parentRouteId lookup

* Sign CLA

* Move parentRouteId logic to a separate function

* Update test fixture path

* chore: format

* chore(dev): add changeset for PR #4538 (#4800)

* chore: format

* refactor(remix-react): use `react-router-dom` import instead of `react-router` (#3325)

* chore: manually bump remix-testing version due to not being on main just yet

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* fixup! chore: manually bump remix-testing version due to not being on main just yet

* fixup! chore: manually bump remix-testing version due to not being on main just yet

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* chore: unify error usage (#4696)

* chore: format

* Add fetcher state/type tests (#4803)

* chore(deps): bump esbuild to latest (#4754)

* chore: add invariant instead of using `!`

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* chore(deps): bump esbuild to latest

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* Create fresh-shrimps-join.md

Signed-off-by: Logan McAnsh <logan@mcan.sh>
Co-authored-by: Pedro Cattori <pcattori@gmail.com>

* test(integration): close server synchronously  (#4785)

* chore: normalize afterAll

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* test: close server synchronously

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* chore: appFixture.close isnt async anymore

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* Update integration/helpers/create-fixture.ts

Co-authored-by: Pedro Cattori <pcattori@gmail.com>

Signed-off-by: Logan McAnsh <logan@mcan.sh>
Co-authored-by: Pedro Cattori <pcattori@gmail.com>

* feat: remix optional segments (#4706)

* feat: transform optional routes from remix to react router

* Add to contributors

* Add changeset

* fix(optional-segments): fix escaping of parenthesis

* small function fix

* Update packages/remix-dev/__tests__/routesConvention-test.ts

Co-authored-by: Pedro Cattori <pcattori@gmail.com>

Co-authored-by: Pedro Cattori <pcattori@gmail.com>

* chore: format

* chore: edit the optional segments changeset (#4815)

to make it clear that Remix won't support optional segments until integrated with React Router 6.5

* chore: format

* docs: rearrange docs, give everything its own page (#4821)

* chore: format

* fix: wrong parentheses in the optional segments changeset (#4825)

* fix: wrong parentheses in the optional segments changeset

* add: akamfoad to the contributers

* ci(nightly): add workflow_call for testing purposes

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* Revert "ci(nightly): add workflow_call for testing purposes"

This reverts commit f3fa77e.

* chore(scripts): Use relaxed peer dependencies to avoid triggering major version bumps (#4736)

- Add script to bump peer deps with changesets version

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Add integration tests for request structures (#4829)

* fix(remix-dev): resolve asset entry full path to support mono-repo import of styles (#4855)

* chore: have eslint report unused eslint comments (#4863)

* chore: have eslint report unused eslint comments

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* chore: remove additional comment

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* perf(remix-architect,remix-netlify): improve performance of `isBinaryType` (#4761)

Co-authored-by: Logan McAnsh <logan@mcan.sh>
Co-authored-by: Pannatier Guillaume <Guillaume.Pannatier@hopitalvs.ch>

* chore: format

* chore(remix-testing): remove internal installGlobals (#4755)

* chore: remove internal installGlobals

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* chore: add README

Signed-off-by: Logan McAnsh <logan@mcan.sh>

* Create quick-cats-fix.md

* Update .changeset/quick-cats-fix.md

Co-authored-by: Michaël De Boey <info@michaeldeboey.be>

* chore(deps): remove jsdom and happydom from devDependencies

Signed-off-by: Logan McAnsh <logan@mcan.sh>

Signed-off-by: Logan McAnsh <logan@mcan.sh>
Co-authored-by: Michaël De Boey <info@michaeldeboey.be>

* fix(dev): build js modules for ts->js conversion

The TS->JS migration was removed from the CLI codemod options, but still
used for TS->JS conversion when creating a new Remix project from the
CLI. The TS modules responsible for the TS->JS conversion were
incorrectly removed from the Rollup build, resulting in the
corresponding built JS modules being absent. That caused the CLI to
error when trying to perform TS->JS conversion. This changes
reintroduces the wiring to build the modules responsible for the TS->JS
conversion.

Fixes #4854

Signed-off-by: Logan McAnsh <logan@mcan.sh>
Co-authored-by: Logan McAnsh <logan@mcan.sh>
Co-authored-by: Matt Kane <m@mk.gg>
Co-authored-by: Remix Run Bot <hello@remix.run>
Co-authored-by: Chance Strickland <hi@chance.dev>
Co-authored-by: Ryan Florence <rpflorence@gmail.com>
Co-authored-by: Lucas Ferreira <panchorf@gmail.com>
Co-authored-by: Andrew Leedham <AndrewLeedham@outlook.com>
Co-authored-by: Matt Brophy <matt@brophy.org>
Co-authored-by: dabdine <1955040+dabdine@users.noreply.github.com>
Co-authored-by: Jacob Ebey <jacob.ebey@live.com>
Co-authored-by: James Restall <james.restall@gmail.com>
Co-authored-by: Michaël De Boey <info@michaeldeboey.be>
Co-authored-by: Mehdi Achour <machour@gmail.com>
Co-authored-by: Dylan Markow <dylan@dylanmarkow.com>
Co-authored-by: Daniel Rios <ieldanr@gmail.com>
Co-authored-by: Akam Foad <41629832+akamfoad@users.noreply.github.com>
Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
Co-authored-by: Guillaume Pannatier <guillaume.pannatier@gmail.com>
Co-authored-by: Pannatier Guillaume <Guillaume.Pannatier@hopitalvs.ch>
@mcansh mcansh mentioned this pull request Dec 21, 2022
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting release This issue has been fixed and will be released soon CLA Signed package:testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🗺 Testing Helpers
4 participants