unc-csxl / csxl.unc.edu

CS Experience Labs' web application.
https://csxl.unc.edu
MIT License
10 stars 11 forks source link

100% Test Coverage for Room Reservations #434

Closed yuvrajjain2003 closed 4 months ago

yuvrajjain2003 commented 5 months ago

This PR addresses the longstanding issue of incomplete documentation and test coverage present in the backend for the room reservation feature.

In general, I made sure that each method that we implemented in the backend has a corresponding test which covers the various cases that could arise while running a method.

Also, I worked on changing the docstrings wherever I saw rooms for improvement to better document the code that we wrote.

yuvrajjain2003 commented 5 months ago

Nice start!

Few outstanding issues:

  • The reservation coverage is at 94% - check the coverage doc and add extra tests as needed.
  • The test_get_map_reserved_times_by_date is failing

Once that is fixed, the PR can be approved!

Ah I see! Okay! I'll take a look at the coverage doc. As far as the test_get_map_reserved_times_by_date method is concerned, the reason it is failing is because of office hours. Since we feed in the office hours manually, the test becomes almost time dependent. That is, I ran it yesterday and it passed because no office hours on Sunday, but then today since we have office hours, the table changes.

I'm not seeing an easy way to test this by removing this "time" aspect. Any suggestions? Otherwise I might have to add in a bunch of if-else conditions to check for each day of the week.

ajaygandecha commented 5 months ago

Yeah @yuvrajjain2003 after reviewing the main branch, it seems that the test was failing before as well. We can loop @KrisJordan in for this.

yuvrajjain2003 commented 5 months ago

Yeah @yuvrajjain2003 after reviewing the main branch, it seems that the test was failing before as well. We can loop @KrisJordan in for this.

Alright! So I actually changed the test slightly to now check for rooms SN139 and SN141, i.e., the pair programming rooms because I know that we don't have office hours in those rooms. So this should make the tests time "independent" now, while also ensuring that we are actually testing the functionality of the method.

As an aside, I was able to reach 99% test coverage with only 5 lines of code missing test coverage! Will keep working on it!

yuvrajjain2003 commented 5 months ago
Screenshot 2024-04-30 at 2 37 54 AM

Should have 100% test coverage for reservation.py now. Also, none of the tests are time dependent, so all of these tests should pass regardless of when the test is run.

yuvrajjain2003 commented 5 months ago

Looking great @yuvrajjain2003, almost there!

The final thing here is that the policy test is only at 90% coverage! The blame log indicates that this file was changed in your group's feature merge, so we want to make sure coverage gets to 100% here too 😄

Yes! Although I am not exactly sure how to write the test for this since this is just the file where I listed the office hour schedule. The function that is not tested is actually the office_hours() function, which is just taking a number and returning a corresponding dictionary.

I could hard code the test for what we have currently, but I don't necessarily think that is a good or permanent solution given the dynamic nature of office hours.

What do you think? @ajaygandecha @KrisJordan

ajaygandecha commented 5 months ago

Ah, good point @yuvrajjain2003! I wonder if, to get around this for now, we can move the policies for each day into a dictionary field, then return something like oh_schedule[day] - then, we can have one test that ensures that all weekdays return a value.

yuvrajjain2003 commented 5 months ago

Alright! Hopefully all good now! Just added tests for the policy service to reach 100% coverage. Thanks for catching all these for me! Please let me know if I need to make any other changes. Happy to do so!