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

test(sample-01): added service unit tests #10623

Merged
merged 3 commits into from
Nov 20, 2024

Conversation

Yansb
Copy link
Contributor

@Yansb Yansb commented Dec 1, 2022

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • [ x] Other... Please describe: Adding unitary tests to sample 01 service

What is the current behavior?

The sample 01 already have e2e tests and unitary tests on the controller, but it's missing tests on the service. It's also missing tests on the create cat function

Issue Number: #1539

What is the new behavior?

I've added a new test file providing an example on how to test only the service, and modified the controller test to add the create cat test.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

I've tried to create an e2e test to the POST method, however the Role guard is blocking the request, I've noticed that in the src/common/guards/roles.guard.ts the guard is expecting an property from the request called user but I did not found a way to pass this property in my request, without using the header or body.

Copy link
Member

@micalevisk micalevisk left a comment

Choose a reason for hiding this comment

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

thank you for your suggestion.

There are couple things to change, tho.

@coveralls
Copy link

coveralls commented Dec 1, 2022

Pull Request Test Coverage Report for Build 961a2e7c-ad7e-401a-bff2-7cecf5aa4419

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 93.404%

Totals Coverage Status
Change from base Build a5022226-a52a-4785-87b3-bf2bbe181377: 0.0%
Covered Lines: 6202
Relevant Lines: 6640

💛 - Coveralls

@Yansb
Copy link
Contributor Author

Yansb commented Dec 1, 2022

Hey @micalevisk I've implemented the code review suggestions, but could you help me with the Role guard so i can write the e2e test to the create endpoint?
image
is this line correct? if so, how do i pass the user metadata in the supertest library?

@micalevisk
Copy link
Member

@Yansb we would have to mock the request.user object but I didn't find any non-hacky way to do that 🤔 Also, looks like the app itself didn't have any way to populate the request.user field

@Yansb
Copy link
Contributor Author

Yansb commented Dec 1, 2022

@micalevisk should i change this implementation to search for the role on the body or header?

@micalevisk
Copy link
Member

you could propose that change in another PR after merging this one.

@kamilmysliwiec kamilmysliwiec merged commit f3ec5f2 into nestjs:master Nov 20, 2024
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.

4 participants