Fix auth unit tests + Enable wrapping options when rendering in jest

[Tobbe]

We had a few error-printouts in our CI

And this was also affecting RW apps (so not just the framework)

Do note that these aren't test failures. The tests still pass. It just looks alarming.

This PR fixes those warnings, and also potential actual bugs where updates could happen to state in <AuthProvider> after it had been unmounted

---

Todos

• Update testing docs, with an explanation of what the render function from rwjs/testing/web does and how its different
• Update templates for jest tests to import screen and use it

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 2000771. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

---

🟥 Failed Commands
cli run test --concurrency 2 -- --colors --maxWorkers 2
cli run test --concurrency 2 -- --colors --maxWorkers 2

✅ Successfully ran 9 targets

• cli run build
• cli run build
• cli run build
• cli run build
• cli run build
• cli run build
• cli run build
• cli run build
• cli run build

---

Sent with 💌 from NxCloud.

[netlify]

✅ Deploy Preview for redwoodjs-docs canceled.

Name	Link
🔨 Latest commit	2000771
🔍 Latest deploy log	https://app.netlify.com/sites/redwoodjs-docs/deploys/62e7688a7c1ffc00094e2ef4

[Tobbe]

We actually already have a hook for this inside the router package. If we agree on these changes I can either shuffle some code around so we can use the same hook in both packages. Or I just copy it over here (I prefer copying it over)

[Tobbe]

This is a bit hacky... But it does make it so we don't have to make any updates to already existing RW Apps out there.

If we want we could wrap this in process.env.NODE_ENV === 'test' and skip the timeout in prod

It's a tradeoff if the extra code complexity the env check would bring is worth it or not

[Tobbe]

Thinking about this some more I really don't like the "wrap in env check" idea. If we had that we'd no longer be testing the same code as we run in prod.

[dac09]

Slippery slope feels slippery! I’m worried this is introducing code with side effects we don’t understand, it seems like we’re forcing the code to run on next tick. I’d be more inclined to stick to stick to standard react and testing patterns so we don’t get untraceable bugs later.

[Tobbe]

I’m worried this is introducing code with side effects we don’t understand, it seems like we’re forcing the code to run on next tick

You're right. I did want to push this execution to the next tick. This lets those most basic tests finish before any auth state changes, thus not needing the act call.

[Tobbe]

Uhh oh @dac09 #1397 😬

Taking the above PR into consideration I've come up with a new solution. I've introduced a RW-specific expectToRender method.

   it('renders Success successfully', async () => {
-    expect(() => {
-      render(<Success ${camelName}={standard().${camelName}} />)
-    }).not.toThrow()
+    await expectToRender(<Success ${camelName}={standard().${camelName}} />)
   })

So instead of doing that expect...not.toThrow thing, we now use this new expectToRender method that does a render and a generic findAllByText under the hood.

WDYT?

[Tobbe]

I just realized this whole problem will go away when/if we get this in: #5985. It no longer has the async behavior of the AuthProvider that's the root cause here.

[dac09]

I just realized this whole problem will go away when/if we get this in: #5985. It no longer has the async behavior of the AuthProvider that's the root cause here.

So... maybe we should HODL the clerk PR until we decouple auth. It's going to be a while though.

I'm also wondering.... what's the reason we needed to move to a functional component anyway? Apart from "functional components are better" - is there an actual reason why we needed to do such a big refactor for one auth client?

[dac09]

Taking the above PR into consideration I've come up with a new solution. I've introduced a RW-specific expectToRender method.

For me this is a step backwards - IMHO. Not only are we asking people to learn yet another method to test with, but we're also taking away the "implicit" learning that in a real test, not just the generated one - you should be using screen.findX and that validating things that way.

[Tobbe]

For me this is a step backwards - IMHO. Not only are we asking people to learn yet another method to test with, but we're also taking away the "implicit" learning that in a real test, not just the generated one - you should be using screen.findX and that validating things that way.

Those are all valid points. I want to involve Rob in this conversation too. Let's see if we can discuss on the next Core Team meeting

[Tobbe]

We discussed this on a meeting. We decided that the best option for now was to roll back the Clerk PR and try to find a good solution for testing auth that works both with Clerk, and with the work we're doing on trying to decouple auth.

The second best option was to start generating boilerplate testcases with render(<MyPage>, { redwoodProviders: false }) or possibly render(<MyPage>, { authProvider: false })

[Tobbe]

I'm going to close this. The change that introduce the regression I'm trying to solve here is being reverted (see #6135) And instead we're looking to revamp our auth implementation in #5985