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 🧪 #1937

Closed
ubbe-xyz opened this issue May 5, 2021 · 12 comments · Fixed by #1992
Closed

Add tests 🧪 #1937

ubbe-xyz opened this issue May 5, 2021 · 12 comments · Fixed by #1992
Assignees
Labels
enhancement New feature or request help-needed The maintainer needs help due to time constraint/missing knowledge stale Did not receive any activity for 60 days test Related to testing

Comments

@ubbe-xyz
Copy link
Collaborator

ubbe-xyz commented May 5, 2021

Summary 💭

Currently, we don't have any test coverage. We rely on manual testing, Typescript, and inline code comments to ensure the library works as expected.

Description 📓

By adding unit/integration tests to the different modules exposed by the library: next-auth/client, next-auth/providers, etc... we will gain confidence during refactors and releases and better documentation than inline code comments. 💭

The idea is to start testing our client module: next-auth/client as a POC to agree on the tooling and the workflows we want. Once we're happy with the setup, then go ahead and test the rest of the modules. 🏋🏽

@balazsorban44
Copy link
Member

So some feedback for those who are following this thread. @lluia and started working on an initial setup at #1992.

We are going to use jest + @testing-library/react + msw for testing the client code, and later on we will move on to the core source code. I also would like us to create some uniform tests for our adapters over at https://github.com/nextauthjs/adapters

Currently, the best-tested adapter is the new Prisma one: https://github.com/nextauthjs/adapters/blob/canary/packages/prisma/tests/index.test.ts

Even that could be improved though in my opinion.

When all these are in place, we will have to start to think about how to write some tests that would connect everything together. Open for suggestions!

@balazsorban44 balazsorban44 changed the title Add tests for client module 🤖 Add tests May 13, 2021
@balazsorban44 balazsorban44 changed the title Add tests Add tests 🧪 May 13, 2021
@balazsorban44 balazsorban44 pinned this issue May 13, 2021
@ndom91
Copy link
Member

ndom91 commented May 13, 2021

So I just checked out the prisma tests, and I'm a bit confused as to how jest expect() is wired up to prisma / the db behind it.

i.e. how does this on test#L53

test("createUser", async () => {
    const adapter = await prismaAdapter.getAdapter(appOptions)

    user = await adapter.createUser({
      email: "[email protected]",
      name: "test",
      image: "https://",
    } as any)

    expect(user.email).toMatchInlineSnapshot(`"[email protected]"`)
    expect(user.name).toMatchInlineSnapshot(`"test"`)
    expect(user.image).toMatchInlineSnapshot(`"https://"`)
  })

Know if Prisma actually created those? Snapshots are usually just text files of literal snapshots of the dom, right?

If jest / jest snapshots can be wired up to database queries / database snapshots that could make the rest of the adapter testing much easier too!

@balazsorban44
Copy link
Member

not sure why they didn't use toEqual(expect.ObjectContaining(user) there

https://jestjs.io/docs/expect#expectobjectcontainingobject

@balazsorban44
Copy link
Member

I started working on Adapter tests, have a look: https://github.com/nextauthjs/adapters/pull/90/files

This basically introduces a helper function that runs the most basic tests for any adapter, in a few lines of code.

@ndom91
Copy link
Member

ndom91 commented May 15, 2021

@balazsorban44 this is great! To wire up the other adapters, i.e. typeorm for example, we just need to spin up docker containers for each type of db again, seed them, and then run a similar test file against it, obviously requiring typeorm instead of the prisma adapter, for example. Right?

@balazsorban44
Copy link
Member

yes, that are my thoughts as well!

@balazsorban44
Copy link
Member

balazsorban44 commented May 15, 2021

Nice progress today, I managed to run tests for the prisma, prisma-legacy and fauna adapters! I will look into dynamodb soon, and then it is only typeorm-legacy that is left.

Although I still have to implement some basic tests, currently I have these:

image

@UmarFKhawaja
Copy link

Are there any test scripts or acceptance criteria that custom adapter developers can use to verify their implementation?

@balazsorban44
Copy link
Member

balazsorban44 commented May 22, 2021

@khawajaumarfarooq Funny that you ask, @ndom91 and I merged a PR (nextauthjs/adapters#90) a few minutes ago, that introduces a way to run basic tests that all adapters should be able to pass!

We should maybe expose it as a package, so people could use it much easier for their custom adapters that are not in the official repo. For now, have a look at: https://github.com/nextauthjs/adapters/blob/canary/basic-tests.ts
And an example on usage: https://github.com/nextauthjs/adapters/blob/canary/packages/prisma-legacy/tests/index.test.ts

Note, that it does not test account linking, because I first have to learn more about it how we use it in the source code in the first place.

@mtt87
Copy link

mtt87 commented Jun 21, 2021

I have an example of how to use playwright for e2e tests, it includes oauth2-mock-server so there is no need to use a real provider. I've created global-setup.ts to handle setup/teardown of the mock server and added the code to [...nextauth].js to handle the refresh token.

mtt87@63a8a9b

At the moment it's just a basic test, open the page, check the user is not logged in, click sign in and login the user, check that the user is now logged in.
I can add more stuff tomorrow or the coming days. Ideally I wanted to add tests for the refresh token scenarios.

@balazsorban44 balazsorban44 unpinned this issue Aug 17, 2021
@stale stale bot added the stale Did not receive any activity for 60 days label Aug 21, 2021
@nextauthjs nextauthjs deleted a comment from stale bot Aug 21, 2021
@stale stale bot removed the stale Did not receive any activity for 60 days label Aug 21, 2021
@stale stale bot added the stale Did not receive any activity for 60 days label Oct 20, 2021
@stale stale bot closed this as completed Oct 27, 2021
@nextauthjs nextauthjs deleted a comment from stale bot Oct 27, 2021
@nextauthjs nextauthjs deleted a comment from stale bot Oct 27, 2021
@balazsorban44 balazsorban44 reopened this Oct 27, 2021
@stale stale bot removed the stale Did not receive any activity for 60 days label Oct 27, 2021
@balazsorban44 balazsorban44 added the help-needed The maintainer needs help due to time constraint/missing knowledge label Oct 27, 2021
@stale
Copy link

stale bot commented Dec 26, 2021

Hi there! It looks like this issue hasn't had any activity for a while. It will be closed if no further activity occurs. If you think your issue is still relevant, feel free to comment on it to keep it open. (Read more at #912) Thanks!

@stale stale bot added the stale Did not receive any activity for 60 days label Dec 26, 2021
@stale
Copy link

stale bot commented Jan 3, 2022

Hi there! It looks like this issue hasn't had any activity for a while. To keep things tidy, I am going to close this issue for now. If you think your issue is still relevant, just leave a comment and I will reopen it. (Read more at #912) Thanks!

@stale stale bot closed this as completed Jan 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help-needed The maintainer needs help due to time constraint/missing knowledge stale Did not receive any activity for 60 days test Related to testing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants