Skip to content

Conversation

@jng34
Copy link
Member

@jng34 jng34 commented May 2, 2025

Fixes #1894

What changes did you make and why did you make them ?

  • Wrote unit tests for recurringEvents.router
  • Wrote tests for fail cases
  • Fixed minor bug in catch block of the "/internal" route in recurringEvents.router

Screenshots of Passed Tests

image

@JackHaeg JackHaeg requested a review from dannyprikaz May 15, 2025 22:45
Copy link
Member

@dannyprikaz dannyprikaz left a comment

Choose a reason for hiding this comment

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

Hey @jng34

You're killing it with these PRs. As I was going through some of these, I realized that I was being lazy with my own tests of routes that were passing the request to a controller, and I ultimately found a solution that I think you should apply to these tests as well.

Prime example is line 137 of backend/routers/recurringEvents.router.test.js. Right now we're just expecting that the create method is being called, but we really want to make sure it's being called with the correct input, namely newEvent. The problem that I was having (and that I'm sure you ran into), is that the controller method is getting passed a ton more than just newEvent. I found a solution, that I put in the comments here and that I incorporated into my own PR. Please update your code to use this expectation where necessary.

Thanks!

Copy link
Member Author

@jng34 jng34 left a comment

Choose a reason for hiding this comment

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

@dannyprikaz
Thank you again for your review and pointers.
It is making a lot more sense now with the specifications of the tests.
I have made the updates. Please review once more when you have time.

@jng34 jng34 requested a review from dannyprikaz May 27, 2025 17:06
@jng34
Copy link
Member Author

jng34 commented Jun 17, 2025

@dannyprikaz please hold off on reviewing this PR. I am currently working on writing tests for error handling as we discussed last week. I will drop a comment to let you know when I have added those tests and the PR is ready for review again.

@JackHaeg
Copy link
Member

Hi @jng34 FYI - I added the waiting to merge label to this PR since we are holding off on reviewing / merging while you work on writing tests for error handling. Once this PR is ready for review, please tag Danny in a comment and remove this label from the PR :)

@jng34
Copy link
Member Author

jng34 commented Jun 19, 2025

@dannyprikaz
The PR has been updated and ready for review.

Copy link
Member

@dannyprikaz dannyprikaz left a comment

Choose a reason for hiding this comment

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

Looks good!

@dannyprikaz dannyprikaz merged commit 57c794b into hackforla:development Jun 24, 2025
3 of 6 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.

Create New Unit Tests for ./backend/routers/recurringEvents.router.js

3 participants