-
Notifications
You must be signed in to change notification settings - Fork 25
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
Add tests #145
Add tests #145
Conversation
…show the same results
{ | ||
displayName: 'node', | ||
testEnvironment: 'node', | ||
testMatch: ['**/__tests__/**/*.spec.ts'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is wild to me that nextjs doesn't provide something out of the box for testing headers/cookies/etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know right?
const handler = handleAuth(); | ||
const response = await handler(request); | ||
|
||
expect(response.status).toBe(500); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I agree, I plan to change that behaviour after this PR lands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: #155
}); | ||
|
||
it('should use Response if NextResponse.redirect is not available', async () => { | ||
const originalRedirect = NextResponse.redirect; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another place we could probably use spyOn ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case we actually do want redirect
to be undefined since we're simulating the conditions of <= Next.js 13.
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
act
to test the github workflow.