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 profile service tests & fix profile service #102

Merged
merged 16 commits into from
Oct 25, 2023

Conversation

akulsharma1
Copy link
Contributor

@akulsharma1 akulsharma1 commented Oct 19, 2023

Resolves #103

  • Added profile-router.test.ts
  • Bug fixes to profile-router.ts

@Timothy-Gonzalez Timothy-Gonzalez changed the title Finished profile tests Add profile service tests & fix profile service Oct 19, 2023
@Timothy-Gonzalez
Copy link
Member

Timothy-Gonzalez commented Oct 19, 2023

Several notes (not sure if significant, cc @AydanPirani for final review):

Coverage report for profile is as follows:

src/services/profile     |    83.8 |    73.91 |      25 |    83.8 |      
  profile-formats.ts     |   77.77 |       25 |     100 |   77.77 | 11-12,15-16,24-25
  profile-lib.ts         |   55.81 |      100 |       0 |   55.81 | 12-18,25-27,35-43
  profile-router.ts      |   88.65 |    84.21 |     100 |   88.65 | 53-77,166-167,228-229,244-246

Not covered by tests:

  • profile-router.ts
    • 55-77: GET /leaderboard/ - no idea how this works or if this should be tested yet
    • 166-167: Redirect condition when no user id, prob not easy to test but should consider checking
    • 228-229: Validation check for profile, should test this by sending bad data
    • 244-246: Check for error handling of failed create, probably not needed to test
  • profile-formats.ts
  • profile-lib.ts
    • 12-18: castLeaderboardEntries: used in GET /leaderboard/, will be covered by profile router tests
    • 25-27: isValidLimit: used in GET /leaderboard/, will be covered by profile router tests
    • 35-43: updatePoints: not used at all currently

All in all, pretty good coverage!

Some things I'd like to see tested:

  • Post as a user (unauthorized user doesn't need to be tested, probably switch this out)
    • Basically, regular users can't edit their profile
  • Can probably remove unauthorized user tests (called invalid user) as these will be tested in auth middleware

Other misc things / nitpicks:

  • Can we rename "invalid user" to something more specific
    • for example: it("delete with a user that doesn't exist") -> it("fails to delete with a non-attendee user")
    • Try to make it a sentence you can read basically

This is a great start though, great job!

@AydanPirani
Copy link
Contributor

@Timothy-Gonzalez @akulsharma1

Leaderboard

GET /leaderboard/ - no idea how this works or if this should be tested yet

12-18: castLeaderboardEntries: used in GET /leaderboard/, will be covered by profile router tests

25-27: isValidLimit: used in GET /leaderboard/, will be covered by profile router tests

I wrote this super basic endpoint, it should return the top limit users by points - @akulsharma1 do you think you can add some tests in for this as well? Basically, all you need to do is:

  • Add in like 3-4 users with different point values
  • Check that the endpoint (no limit) returns the users in ordered point values (most to least).
  • Check that the endpoint (limit 2) returns the 2 users with the highest point counts.

And maybe add in a test case to ensure that limit cannot be negative.

Format Testing

We could add tests for this, but shouldn't as we should use something like zod instead, re: #104

Yeah, dw about testing the actual "isValidformat" for now, just make sure that it throws if it gets a bad input.

Role Stuff

Can probably remove unauthorized user tests (called invalid user) as these will be tested in auth middleware

Yup, dw about testing with roles that don't have any perms - just test that each branch of hasElevatedPerms/hasStaffPerms/hasAdminPerms gets tested.

Copy link
Contributor

@AydanPirani AydanPirani left a comment

Choose a reason for hiding this comment

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

it's making me review, but please reference above comments! I'm good with this once you get through the checkboxes + if @Timothy-Gonzalez is happy. with it

Copy link
Member

@Timothy-Gonzalez Timothy-Gonzalez left a comment

Choose a reason for hiding this comment

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

Looks great! A few small nps to move duplicated code out of tests & into variables & beforeEach hooks, but otherwise great job!

src/services/profile/profile-router.test.ts Outdated Show resolved Hide resolved
src/services/profile/profile-router.test.ts Outdated Show resolved Hide resolved
src/services/profile/profile-router.test.ts Outdated Show resolved Hide resolved
src/services/profile/profile-router.test.ts Outdated Show resolved Hide resolved
src/services/profile/profile-router.test.ts Outdated Show resolved Hide resolved
src/services/profile/profile-router.test.ts Outdated Show resolved Hide resolved
src/services/profile/profile-router.test.ts Outdated Show resolved Hide resolved
src/services/profile/profile-router.test.ts Outdated Show resolved Hide resolved
src/services/profile/profile-router.test.ts Outdated Show resolved Hide resolved
src/services/profile/profile-router.test.ts Outdated Show resolved Hide resolved
Copy link
Member

@Timothy-Gonzalez Timothy-Gonzalez left a comment

Choose a reason for hiding this comment

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

few more nps that I missed

src/services/profile/profile-router.test.ts Outdated Show resolved Hide resolved
src/services/profile/profile-router.test.ts Outdated Show resolved Hide resolved
src/services/profile/profile-router.test.ts Outdated Show resolved Hide resolved
Copy link
Member

@Timothy-Gonzalez Timothy-Gonzalez left a comment

Choose a reason for hiding this comment

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

Great job! Feel free to merge!

This should be a squash merge, make sure to edit the description to only include the important things (not "testing formatting adjustments" for example)

@akulsharma1 akulsharma1 merged commit 968ff96 into main Oct 25, 2023
5 checks passed
@akulsharma1 akulsharma1 deleted the dev/akul/profiletests branch October 25, 2023 22:08
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.

Tests for profile service
3 participants