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 remainder auth tests #2233

Merged

Conversation

horvathmarton
Copy link
Contributor

Fixes

#1717

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 methods of:

  • AuthController
  • PasswordService
  • UserController
  • UserService

Add dynamic mocks for Express request and response.

Add custom matches for user controller responses.

@imolorhe
Copy link
Collaborator

@horvathmarton do you need to rebase? Looks like this still shows changes from the previous PR

@horvathmarton
Copy link
Contributor Author

@imolorhe true, rebase is done.

@@ -0,0 +1,9 @@
import { Request, Response } from 'express';

export function mockRequest(props?: object): Request {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
export function mockRequest(props?: object): Request {
export function mockRequest(props?: Partial<Request>): Request {

the props should be valid fields in request right? Same treatment for response below.

it(`should return true if the password matches the provided hash`, async () => {
// GIVEN
jest
.spyOn(bcrypt, 'compare')
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of mocking bcrypt, let's generate a valid hash and hardcode it in the test. You can create a map of password-to-hash that you can use in the tests, if you need multiple.

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 making the changes!

@imolorhe imolorhe added this pull request to the merge queue Jul 22, 2023
Merged via the queue into altair-graphql:master with commit f433ee1 Jul 22, 2023
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.

2 participants