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

Add tests #145

Merged
merged 15 commits into from
Dec 27, 2024
Merged

Add tests #145

merged 15 commits into from
Dec 27, 2024

Conversation

PaulAsjes
Copy link
Collaborator

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.

@PaulAsjes PaulAsjes mentioned this pull request Dec 3, 2024
@PaulAsjes PaulAsjes requested a review from cmatheson December 4, 2024 15:51
__tests__/auth.spec.ts Show resolved Hide resolved
{
displayName: 'node',
testEnvironment: 'node',
testMatch: ['**/__tests__/**/*.spec.ts'],
Copy link
Collaborator

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)

Copy link
Collaborator Author

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.

Copy link
Collaborator

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).

Copy link
Collaborator Author

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,
Copy link
Collaborator

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%?)

Copy link
Collaborator Author

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 () => ({
Copy link
Collaborator

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know right?

package.json Outdated Show resolved Hide resolved
__tests__/get-authorization-url.spec.ts Show resolved Hide resolved
const handler = handleAuth();
const response = await handler(request);

expect(response.status).toBe(500);
Copy link
Collaborator

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;
Copy link
Collaborator

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).

Copy link
Collaborator Author

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,
Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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;
Copy link
Collaborator

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 ?

Copy link
Collaborator Author

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.

@PaulAsjes PaulAsjes requested a review from cmatheson December 16, 2024 09:17
@PaulAsjes PaulAsjes merged commit 2d5204e into main Dec 27, 2024
3 checks passed
@PaulAsjes PaulAsjes deleted the pma/add-tests branch December 27, 2024 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants