Conversation
…show the same results
| { | ||
| displayName: 'node', | ||
| testEnvironment: 'node', | ||
| testMatch: ['**/__tests__/**/*.spec.ts'], |
There was a problem hiding this comment.
i wish we were doing the tests alongside our code like we do in other repos (mono repo, authkit-js, etc).
(the auth.ts / auth.spects.ts pattern)
There was a problem hiding this comment.
Any specific reason why? I find that it clutters up the source file somewhat by having the tests in there in parallel.
There was a problem hiding this comment.
ok my two reasons why:
- (more compelling): having the tests live next to the code is consistent with the monorepo and other authkit repos
- personal preference: i think it's easier to get to the tests (and keep them in sync with the code) when they live side-by-side (can't say my opinion is better than yours though).
There was a problem hiding this comment.
My big issue is that the src folder is already pretty cluttered since we dump all files in there without any subdivision. This will be fixed in #147 however where I've at least separated the source by components.
I'm going to merge this PR as is for now, but once the mentioned PR has merged I'll move the tests to be alongside the rest of the code since it would make it more consistent with other authkit repos as you mentioned.
|
|
||
| coverageThreshold: { | ||
| global: { | ||
| branches: 100, |
There was a problem hiding this comment.
is this enforcing 100% coverage? i think maybe having a little bit of wiggle room would be better (90%?)
There was a problem hiding this comment.
I landed on going for 100% coverage for now, but with the expectation of lowering it later if it proves to be too annoying.
| const headersStore = new Map(); | ||
|
|
||
| return { | ||
| headers: async () => ({ |
There was a problem hiding this comment.
it is wild to me that nextjs doesn't provide something out of the box for testing headers/cookies/etc
| const handler = handleAuth(); | ||
| const response = await handler(request); | ||
|
|
||
| expect(response.status).toBe(500); |
There was a problem hiding this comment.
not relevant to your tests, but i'm surprised we return a 500 in all of these wrong/missing code scenarios. seems wrong? (this is the classic nextjs-can't-render 400 errors though i suppose?). i wonder if we should redirect back to the login page?
| const originalLocation = window.location; | ||
|
|
||
| // @ts-expect-error - we're deleting the property to test the mock | ||
| delete window.location; |
There was a problem hiding this comment.
could we maybe do something like jest.spyOn(window.location, 'reload') instead of replacing munging the window object directly? (then your location will be properly restored even if an exception is thrown/etc).
There was a problem hiding this comment.
Unfortunately not because window.location.reload is a read only property. Spying on it with jest still attempts to modify the function giving you a TypeError: Cannot assign to read only property 'reload' of object '[object Location]' error.
| request, | ||
| true, | ||
| { | ||
| enabled: false, |
There was a problem hiding this comment.
probably a separate PR but i think this redirect should NOT occur when the middeware auth is not enabled.
There was a problem hiding this comment.
Yeah I agree, I plan to change that behaviour after this PR lands.
| }); | ||
|
|
||
| it('should use Response if NextResponse.redirect is not available', async () => { | ||
| const originalRedirect = NextResponse.redirect; |
There was a problem hiding this comment.
another place we could probably use spyOn ?
There was a problem hiding this comment.
In this case we actually do want redirect to be undefined since we're simulating the conditions of <= Next.js 13.
2 participants
Adds tests for all of authkit-nextjs.
There's a large diff here but it's mostly
package-lock.json.To run the tests locally,
npm run test.Also used
actto test the github workflow.