chore(site): update and refactor all custom hook tests that rely on React Router #12219

Parkreiner · 2024-02-19T23:50:33Z

Changes made

Revamps our renderHookWithAuth test helper function
- Makes sure that manually calling renderHook's rerender function does not cause state to get lost or incorrectly refreshed
- Revamps the API for the function to ensure that have to grab "snapshots" of router state, rather than having a live, volatile link
- Removes the router from the function's return value (React Router has flagged all of their router properties as being internal-only, so we don't want a test helper that relies on APIs that could break outside of major versions)
- Splits off the function into a separate file so that the renderHelpers file doesn't have a bunch of noise
Beefs up and generalizes useTab into useSearchParamsKey
- Adds support for clearing out the current key, and also exposes more options via its inputs
- Adds test cases for all major functionality
Moved usePaginatedQuery to new test helper, and verified tests
Moved useWorkspaceDuplication to the new test helper and verified tests

Notes

Figuring out how to build renderHookWithAuth took a lot of trial and error. I refactored the code several times to make it as clear as I could, but if there's anything that still isn't clear, please flag that, so I can update the code (or at least add a comment)
- The function has to do some wacky stuff with React rendering behavior to get createMemoryRouter and renderHook wired up and overcome their incompatible APIs
- That said, the function does everything through React Router's public APIs (no reading internals that they don't document anywhere), so this code should be pretty stable
- I tried as hard as I could to make the interface for the function as nice as possible, so you can ignore the chaotic implementation details

Parkreiner · 2024-02-28T19:26:53Z

Bumping this to prevent it from getting stale – had to put it off to get the clipboard fixes/tests done faster, but I will be getting back to this

Parkreiner · 2024-03-03T23:56:35Z

Nearly got this thing done. The tests should be stable now, but one thing I do want to do is remove the renderHookWithAuth's reliance on internal React Router implementation details

(The React Router router objects have a ton of properties that you can freely access, but they're all marked as 'for internal use only'. Not sure if those comments got added after I made the initial version of the helper, or if I missed them)

aslilac

The 20 line comment about how cursed this has to be because we're using the undocumented RouterProvider component has me really concerned about this. I know we were already using that component, but I don't understand why we're using an undocumented component over a component like MemoryRouter 😅

aslilac · 2024-03-06T17:38:35Z

site/src/pages/CreateWorkspacePage/useWorkspaceDuplication.test.tsx

@@ -45,28 +49,23 @@ describe(`${useWorkspaceDuplication.name}`, () => {
    expect(result.current.isDuplicationReady).toBe(false);

    for (let i = 0; i < 10; i++) {
-      rerender({ workspace: undefined });
+      void rerender({ workspace: undefined });


this really isn't supposed to be an await?

Typo I forgot to fix – good catch

aslilac · 2024-03-06T17:41:42Z

site/src/pages/CreateWorkspacePage/useWorkspaceDuplication.test.tsx

-
-    const parsedParams = new URLSearchParams(router.state.location.search);
-    const extraMetadataEntries = [
+    const extraMetadataEntries: readonly [string, string][] = [


why the switch from as const?

I felt like it would surface any accidental errors in a better way, and that it does a better job of communicating intent

With the previous code, I could do something like this:

const extraMetadataEntries = [ ["mode", "duplicate"], ["name", `${MockWorkspace.name}-copy`], ["version", MockWorkspace.template_active_version_id], function obviouslyWrong() {}, null, ["forgotToAddTheSecondTupleElement"] ] as const;

And this would be valid still – the underlying type would get broadened. So while the type mismatch would get caught down the line, redefining the type to be an array of string tuples tells you what types should go in, and is more defensive against typos

aslilac · 2024-03-06T17:44:53Z

site/src/testHelpers/hooks.tsx

+   * we're not relying on internal React Router implementation details that
+   * could break at a moment's notice
+   */
+  let escapedLocation!: Location; // Definite assignment via !


well that's a bit of typescript I've never seen before. I'm wary of comments that just say "this line of code means x" tho.

It's less a "this line of code means x", and more a "here's a bigger warning sign for something that silences the compiler and is only a single, easy-to-miss character", but I get what you mean

Wish there were a more obvious syntax, but because of grammar rules, you can't even insert type assertions

aslilac · 2024-03-06T17:59:47Z

our testing code seems to differ substantially from the sort of stuff https://v5.reactrouter.com/web/guides/testing recommends

Parkreiner · 2024-03-08T16:59:45Z

@aslilac So there's a couple of things this is trying to get around. Sorry for the walls of text, and I want to be very clear: this isn't directed at you, but I'm also trying to get this put into writing before I forget anything:

MemoryRouter just isn't nearly as ergonomic as createMemoryRouter for testing setup
- While MemoryRouter still works, the main issue for it is that it's incredibly hard to generalize into a single test helper. MemoryRouter needs you to supply an individual Route component for every single path segment you want to test for, and make sure that each Route has some kind of component associated with it. This is because under the hood, React Router is processing and stripping the children JSX it receives into a JSON object of what routes to set up. You can't just go "I want to setup a component for /a/b/c, and I'm okay if going anywhere else causes an error"
- createMemoryRouter doesn't have this limitation – you can go "make me a route for /a/b/c and ignore everything else", and it'll fill in the missing pieces behind the scenes. But the tradeoff is that the component that receives the router no longer accepts children
- It would be possible to make a helper to help interface our tests with the original MemoryRouter, but it's going to require recursive components and a bunch of dummy components. I considered it for a little bit, but I stopped myself because that felt like it'd be even more complex
The test examples you linked are for React Router v5 – and v6 is notorious for its breaking changes from v5 (as in, genuinely made a lot of people mad)
- The very first example they show in the v5 docs is no longer valid (and that's on them). Trying to have Sidebar be a direct child of MemoryRouter will immediately break
- Though the real issue is that despite v6 being 2.5 years old, they don't have any modern testing documentation at this point
Honestly, I feel like the React Router people haven't really been concerned about ergonomics outside of Remix that much
- They don't stabilize the function references for any of their custom hooks
- They just expect you to ignore the ESLint rules around dependency arrays all over the place, and even codify that assumption into their docs
The problem of testing React Router custom hooks has been a problem for ages, and the Remix team just doesn't seem that interested in supporting the use case
- I scoured StackOverflow and GitHub for ages, and the very few solutions I found were also hacks, but they didn't address every possible thing you would want from a custom hook test
- This issue just got updated because other people are still struggling with this:
```
# Formatting link like this to avoid pinging them
https://github.com/remix-run/react-router/issues/7759
```
Also, just making sure: were you looking at the docs for v5? The page for RouterProvider is right here

Now all that said, I can go back to the drawing board on this. What are your main concerns, either from an interface, or an implementation standpoint?

aslilac · 2024-03-08T19:05:27Z

gotta love that the top google search result was the docs for a 3 year old version of the library 🙃 sorry about that. let me go read the right docs and rereview with that in mind.

aslilac

I gotta admit, I'm still very unhappy about this code, but I at least sort of get why it has to be this nasty. 😅

aslilac · 2024-03-08T19:54:59Z

site/src/pages/CreateWorkspacePage/useWorkspaceDuplication.test.tsx

 ) {
  await waitFor(() => expect(result.current.isDuplicationReady).toBe(true));
-  result.current.duplicateWorkspace();
+  void act(() => result.current.duplicateWorkspace());


another void/await?

Yup – will fix

Parkreiner · 2024-03-08T21:39:42Z

@aslilac Yeah, believe me – I would throw this into a fire if I could

Parkreiner added 3 commits February 19, 2024 23:45

chore: rename useTab to useSearchParamsKey and add test

0b186bf

chore: mark old renderHookWithAuth as deprecated (temp)

a7a0944

fix: update imports for useResourcesNav

ec3ba4f

Parkreiner self-assigned this Feb 19, 2024

Parkreiner changed the title ~~chore: revamp all custom hook tests using React Router~~ chore: revamp all custom hook tests that rely on React Router Feb 19, 2024

Parkreiner changed the title ~~chore: revamp all custom hook tests that rely on React Router~~ chore: update and refactor all custom hook tests that rely on React Router Feb 19, 2024

Parkreiner changed the title ~~chore: update and refactor all custom hook tests that rely on React Router~~ chore(site): update and refactor all custom hook tests that rely on React Router Feb 19, 2024

github-actions bot added the stale This issue is like stale bread. label Feb 27, 2024

Parkreiner added the site Area: frontend dashboard label Feb 28, 2024

github-actions bot removed the stale This issue is like stale bread. label Feb 29, 2024

Parkreiner added 10 commits March 3, 2024 15:14

Merge branch 'main' into mes/hook-test-revamps-4

6d7ef9f

refactor: change API for useSearchParamsKey

afffb26

chore: let user pass in their own URLSearchParams value

590f02b

refactor: clean up comments for clarity

9e00ea6

fix: update import

d7110c5

wip: commit progress on useWorkspaceDuplication revamp

2cc63d7

chore: migrate duplication test to new helper

26089e3

refactor: update code for clarity

8057397

refactor: reorder test cases for clarity

f8f87a3

refactor: split off hook helper into separate file

dcc89dc

Parkreiner added 3 commits March 4, 2024 00:32

refactor: remove reliance on internal React Router state property

1ab6f03

refactor: move variables around for more clarity

b32603f

refactor: more updates for clarity

b0fabde

Parkreiner requested review from a team and aslilac and removed request for a team March 4, 2024 16:56

Parkreiner marked this pull request as ready for review March 4, 2024 16:56

refactor: reorganize test cases for clarity

b94decc

aslilac reviewed Mar 6, 2024

View reviewed changes

aslilac approved these changes Mar 8, 2024

View reviewed changes

Parkreiner added 3 commits March 8, 2024 23:08

refactor: clean up test cases for useWorkspaceDupe

3e41877

refactor: clean up test cases for useWorkspaceDupe

1fcf9e2

Merge branch 'main' into mes/hook-test-revamps-4

13f5e53

Parkreiner merged commit 4d42c07 into main Mar 8, 2024

Parkreiner deleted the mes/hook-test-revamps-4 branch March 8, 2024 23:31

github-actions bot locked and limited conversation to collaborators Mar 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(site): update and refactor all custom hook tests that rely on React Router #12219

chore(site): update and refactor all custom hook tests that rely on React Router #12219

Parkreiner commented Feb 19, 2024 •

edited

Loading

Parkreiner commented Feb 28, 2024

Parkreiner commented Mar 3, 2024

aslilac left a comment

aslilac Mar 6, 2024

Parkreiner Mar 8, 2024

aslilac Mar 6, 2024

Parkreiner Mar 8, 2024 •

edited

Loading

aslilac Mar 6, 2024

Parkreiner Mar 8, 2024 •

edited

Loading

aslilac commented Mar 6, 2024

Parkreiner commented Mar 8, 2024 •

edited

Loading

aslilac commented Mar 8, 2024

aslilac left a comment

aslilac Mar 8, 2024

Parkreiner Mar 8, 2024

Parkreiner commented Mar 8, 2024

chore(site): update and refactor all custom hook tests that rely on React Router #12219

chore(site): update and refactor all custom hook tests that rely on React Router #12219

Conversation

Parkreiner commented Feb 19, 2024 • edited Loading

Changes made

Notes

Parkreiner commented Feb 28, 2024

Parkreiner commented Mar 3, 2024

aslilac left a comment

Choose a reason for hiding this comment

aslilac Mar 6, 2024

Choose a reason for hiding this comment

Parkreiner Mar 8, 2024

Choose a reason for hiding this comment

aslilac Mar 6, 2024

Choose a reason for hiding this comment

Parkreiner Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

aslilac Mar 6, 2024

Choose a reason for hiding this comment

Parkreiner Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

aslilac commented Mar 6, 2024

Parkreiner commented Mar 8, 2024 • edited Loading

aslilac commented Mar 8, 2024

aslilac left a comment

Choose a reason for hiding this comment

aslilac Mar 8, 2024

Choose a reason for hiding this comment

Parkreiner Mar 8, 2024

Choose a reason for hiding this comment

Parkreiner commented Mar 8, 2024

Parkreiner commented Feb 19, 2024 •

edited

Loading

Parkreiner Mar 8, 2024 •

edited

Loading

Parkreiner Mar 8, 2024 •

edited

Loading

Parkreiner commented Mar 8, 2024 •

edited

Loading