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 unit tests for API user service #2223

Merged

Conversation

horvathmarton
Copy link
Contributor

@horvathmarton horvathmarton commented Jul 9, 2023

Checks

  • Ran yarn test-build
  • Updated relevant documentations
  • Updated matching config options in altair-static

Changes proposed in this pull request:

Added unit tests for all public method of altair-api/UserService that is not a proxy call.

Along with that, I added a couple of mocks and custom matches to make the test cases more readable.

Copy link
Collaborator

@imolorhe imolorhe left a comment

Choose a reason for hiding this comment

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

Thanks for creating this PR! Left a few comments. I only added each comment once but the same comment should be considered in other places where they apply.

it('should return a user object', async () => {
// GIVEN
const userMock = mockUser();
jest.spyOn(service, 'getUser').mockResolvedValueOnce(userMock);
Copy link
Collaborator

Choose a reason for hiding this comment

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

So instead of mocking the method on the service being tested, let's mock the underlying prismaService method instead.

const user = await service.mustGetUser(userMock.id);

// THEN
(expect(user) as any).toBeUser();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like that you have implemented custom matchers. Can we add typings for the custom matchers also, instead of using any? It would also help with discovery so other authors know it exists?

https://redd.one/blog/practical-guide-to-custom-jest-matchers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@imolorhe can you give me some guidance here, please?

I defined the type extension (jest.d.ts) under the altair-api folder. But I'm having issues with which tsconfig.json to include.

This is what I have tried already:

  1. The package's (packages/altair-api/tsconfig.json)
  2. The root (tsconfig.json)
  3. Create a spec tsconfig under the package (packages/altair-api/tsconfig.spec.json) and point the jest config to it.

The test suite keeps failing on the missing type:

src/auth/user/user.service.spec.ts:60:20 - error TS2339: Property 'toBeUser' does not exist on type 'JestMatchers<User>'.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think what you'll want is to create it under @types/jest/index.d.ts (similar to the one for express)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried placing it under the @types folder both as @types/jest/index.d.ts and @types/jest.d.ts, but in that case jest global keywords (it, describe etc.) are lost for me.

I could make it work by defining the declaration file in the altair-api root. Is it okay?

@imolorhe imolorhe added this pull request to the merge queue Jul 15, 2023
@imolorhe
Copy link
Collaborator

Thanks for the changes! @horvathmarton

Merged via the queue into altair-graphql:master with commit 2c87d42 Jul 15, 2023
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants