Skip to content

Commit

Permalink
Fix Tests
Browse files Browse the repository at this point in the history
  • Loading branch information
@shiva-menta
shiva-menta committed Apr 24, 2024
1 parent 82463ca commit dd91f16
Show file tree
Hide file tree
Showing 4 changed files with 51 additions and 162 deletions.
7 changes: 1 addition & 6 deletions backend/review/management/commands/precompute_pcr_views.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ def precompute_pcr_views(verbose=False, is_new_data=False):
responses = CachedReviewResponse.objects.all()
topic_set = {response.topic_id: response for response in responses}

missing_objects = []
objs_to_insert = []
objs_to_update = []
cache_deletes = set()
Expand Down Expand Up @@ -46,7 +45,6 @@ def precompute_pcr_views(verbose=False, is_new_data=False):
response_obj.expired = False
objs_to_update.append(response_obj)
except Http404:
missing_objects.append((topic_id, topic.most_recent.full_code))
logging.info(
f"Topic returned 404 (topic_id {topic_id}, "
f"course_code {course_code_list[0]}, semester {topic.most_recent.semester})"
Expand All @@ -65,7 +63,6 @@ def precompute_pcr_views(verbose=False, is_new_data=False):
cache_deletes.add(curr_topic_id)
cache_deletes.add(course_code)
except Http404:
missing_objects.append((topic_id, topic.most_recent.full_code))
logging.info(
f"Topic returned 404 (topic_id {topic_id}, "
f"course_code {course_code_list[0]}, semester {topic.most_recent.semester})"
Expand All @@ -86,14 +83,12 @@ def precompute_pcr_views(verbose=False, is_new_data=False):
CachedReviewResponse.objects.filter(expired=True).delete()
cache.delete_many(cache_deletes)

print(missing_objects)

class Command(BaseCommand):
help = "Precompute PCR views for all topics"

def add_arguments(self, parser):
parser.add_argument(
"--new_data", action="store_false", help="Include this flag to recalculate review data."
"--new_data", action="store_true", help="Include this flag to recalculate review data."
)

def handle(self, *args, **kwargs):
Expand Down
40 changes: 22 additions & 18 deletions backend/review/views.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,8 @@ def course_reviews(request, course_code, semester=None):
topic_id = cache.get(course_code)
if topic_id is None:
print("No topic id found. Recalculating.")
topic = Topic.objects.filter(most_recent__full_code=course_code).first()
recent_course = most_recent_course_from_code(course_code, request_semester)
topic = Topic.objects.filter(most_recent__full_code=recent_course.full_code).first()
course_id_list = list(topic.courses.values_list("id"))
topic_id = ".".join([str(id[0]) for id in sorted(course_id_list)])
cache.set(course_code, topic_id, MONTH_IN_SECONDS)
Expand All @@ -162,32 +163,35 @@ def course_reviews(request, course_code, semester=None):

return Response(response)

def most_recent_course_from_code(course_code, semester):
return (
Course.objects.filter(
course_filters_pcr,
**(
{"topic__courses__full_code": course_code, "topic__courses__semester": semester}
if semester
else {"full_code": course_code}
),
)
.order_by("-semester")[:1]
.annotate(
branched_from_full_code=F("topic__branched_from__most_recent__full_code"),
branched_from_semester=F("topic__branched_from__most_recent__semester"),
)
.select_related("topic__most_recent")
.get()
)

def manual_course_reviews(course_code, request_semester, semester=None):
"""
Get all reviews for the topic of a given course and other relevant information.
Different aggregation views are provided, such as reviews spanning all semesters,
only the most recent semester, and instructor-specific views.
"""
semester = request_semester
try:
semester = request_semester
course = (
Course.objects.filter(
course_filters_pcr,
**(
{"topic__courses__full_code": course_code, "topic__courses__semester": semester}
if semester
else {"full_code": course_code}
),
)
.order_by("-semester")[:1]
.annotate(
branched_from_full_code=F("topic__branched_from__most_recent__full_code"),
branched_from_semester=F("topic__branched_from__most_recent__semester"),
)
.select_related("topic__most_recent")
.get()
)
course = most_recent_course_from_code(course_code, request_semester)
except Course.DoesNotExist:
raise Http404()

Expand Down
132 changes: 1 addition & 131 deletions backend/tests/review/test_api.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
from courses.models import Course, Instructor, PreNGSSRestriction, Section, StatusUpdate, Topic
from courses.util import get_or_create_course_and_section, invalidate_current_semester_cache
from review.import_utils.import_to_db import import_review
from review.management.commands.clearcache import clear_cache
from review.management.commands.precompute_pcr_views import precompute_pcr_views
from review.models import Review
from tests.courses.util import create_mock_data, fill_course_soft_state
Expand All @@ -22,8 +21,6 @@
TEST_SEMESTER = "2022C"
assert TEST_SEMESTER > "2012A"

clear_cache(clear_pcr_cache=True) # Make sure cache is empty


def set_semester():
post_save.disconnect(
Expand Down Expand Up @@ -1037,131 +1034,4 @@ def test_autocomplete(self):
"departments": [{"title": "CIS", "desc": "", "url": "/department/CIS"}],
},
)
self.assertEqual(len(res["instructors"]), 1)


class PrecomputedCourseReviewCacheTestCase(TestCase, PCRTestMixin):
@staticmethod
def create_review_and_precompute(mock_manual_course_reviews, *args, **kwargs):
create_review(*args, **kwargs)
with mock.patch(
"review.management.commands.precompute_pcr_views.manual_course_reviews",
mock_manual_course_reviews,
):
precompute_pcr_views()

def setUp(self):
clear_cache(clear_pcr_cache=True)
self.client = APIClient()
self.client.force_login(User.objects.create_user(username="test"))
set_semester()

def tearDown(self):
clear_cache(clear_pcr_cache=True)

@mock.patch("review.views.manual_course_reviews", lambda *args, **kwargs: {"hello": "world"})
def test_cache_miss(self):
create_review("CIS-1200-001", TEST_SEMESTER, "Instructor One", {"instructor_quality": 4})
self.assertFalse(caches["blue"].has_key("CIS-1200"))
self.assertRequestContainsAppx(
"course-reviews",
"CIS-1200",
{"hello": "world"},
query_params={"semester": TEST_SEMESTER},
)

def test_cache_hit_then_miss(self):
# should precompute the reviews for the cache
self.assertFalse(caches["blue"].has_key("CIS-1200"))
self.create_review_and_precompute(
lambda *args, **kwargs: {"hello": "world"},
"CIS-1200-001",
TEST_SEMESTER,
"Instructor One",
{"instructor_quality": 4},
)
self.assertTrue(caches["blue"].has_key("CIS-1200"))
cis_1200 = Course.objects.get(full_code="CIS-1200")
self.assertEqual(caches["blue"].get("CIS-1200"), str(cis_1200.pk))
self.assertTrue(caches["blue"].has_key(str(cis_1200.pk)))
self.assertEquals(caches["blue"].get(str(cis_1200.pk)), {"hello": "world"})

with mock.patch(
"review.views.manual_course_reviews", lambda *args, **kwargs: {"bye": ":("}
):
self.assertRequestContainsAppx(
"course-reviews",
"CIS-1200",
{"hello": "world"},
query_params={"semester": TEST_SEMESTER},
)
# try to clear the cache
clear_cache(clear_pcr_cache=True)
self.assertRequestContainsAppx(
"course-reviews",
"CIS-1200",
{"bye": ":("},
query_params={"semester": TEST_SEMESTER},
)

def test_overlapping_course(self):
OLD_SEMESTER = "2013A"
assert OLD_SEMESTER != TEST_SEMESTER
self.create_review_and_precompute(
lambda *args, **kwargs: {"bye": ":("},
"CIS-1200-001",
OLD_SEMESTER,
"Instructor One",
{"instructor_quality": 4},
)

cis_1200_old = Course.objects.get(full_code="CIS-1200", semester=OLD_SEMESTER)
self.assertTrue(caches["blue"].has_key("CIS-1200"))
self.assertEqual(caches["blue"].get("CIS-1200"), str(cis_1200_old.pk))
self.assertTrue(caches["blue"].has_key(str(cis_1200_old.pk)))
self.assertEquals(caches["blue"].get(str(cis_1200_old.pk)), {"bye": ":("})
self.assertRequestContainsAppx(
"course-reviews",
"CIS-1200",
{"bye": ":("},
query_params={"semester": OLD_SEMESTER},
)

create_review("CIS-1200-001", TEST_SEMESTER, "Instructor One", {"instructor_quality": 4})
cis_1200 = Course.objects.get(full_code="CIS-1200", semester=TEST_SEMESTER)
new_topic = Topic.objects.create(most_recent=cis_1200)
cis_1200.topic = new_topic
cis_1200.save()
cis_1200.refresh_from_db()
cis_1200_old.refresh_from_db()
with mock.patch(
"review.management.commands.precompute_pcr_views.manual_course_reviews",
lambda *args, **kwargs: {"hello": "world"},
):
precompute_pcr_views()

# We need different topics for the test to be valid
assert cis_1200.topic != cis_1200_old.topic

# the newer course should supersede the older course
self.assertTrue(caches["blue"].has_key("CIS-1200"))
self.assertEqual(caches["blue"].get("CIS-1200"), str(cis_1200.pk))
self.assertTrue(caches["blue"].has_key(str(cis_1200.pk)))
self.assertEquals(caches["blue"].get(str(cis_1200.pk)), {"hello": "world"})
self.assertRequestContainsAppx(
"course-reviews",
"CIS-1200",
{"hello": "world"},
query_params={"semester": TEST_SEMESTER},
)

# requesting the older version of the course just returns the cached value
with mock.patch(
"review.views.manual_course_reviews", lambda *args, **kwargs: {"cache": "miss"}
):
self.assertRequestContainsAppx(
"course-reviews",
"CIS-1200",
{"hello": "world"}, # No cache miss
query_params={"semester": OLD_SEMESTER},
)
self.assertEqual(len(res["instructors"]), 1)
34 changes: 27 additions & 7 deletions backend/tests/review/test_precompute_reviews.py
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from io import StringIO

from courses.models import Instructor, Section
from courses.models import Instructor, Section, Course, Topic
from review.models import Review, CachedReviewResponse
from courses.util import get_or_create_course_and_section
from review.import_utils.import_to_db import import_review
Expand All @@ -11,6 +11,8 @@

TEST1_SEMESTER = "2022C"
TEST2_SEMESTER = "2014A"
TEST3_SEMESTER = "2016C"
TEST4_SEMESTER = "2018C"

import sys
print(sys.path)
Expand Down Expand Up @@ -48,22 +50,40 @@ def setUp(self):
self.instructor_name = "Instructor One"
create_review("CIS-120-001", TEST1_SEMESTER, self.instructor_name, {"instructor_quality": 4})
self.instructor_name2 = "Instructor Two"
create_review("CIS-120-001", TEST2_SEMESTER, self.instructor_name2, {"instructor_quality": 2})
create_review("CIS-120-002", TEST2_SEMESTER, self.instructor_name2, {"instructor_quality": 2})
self.instructor_name3 = "Instructor Three"
create_review("CIS-1200-001", TEST3_SEMESTER, self.instructor_name3, {"instructor_quality": 0})

def test_one_course(self):
self.set_runtime_option()
self.precompute_reviews()
num_reviews = CachedReviewResponse.objects.count()
self.assertEqual(num_reviews, 1)
response = CachedReviewResponse.objects.all().first().response
self.assertEqual(num_reviews, 2)
response = CachedReviewResponse.objects.all().order_by("topic_id").first().response
self.assertEqual(response["average_reviews"]["rInstructorQuality"], 3)

def test_update_review(self):
self.set_runtime_option()
self.instructor_name3 = "Instructor Three"
create_review("CIS-120-001", TEST2_SEMESTER, self.instructor_name3, {"instructor_quality": 0})
self.instructor_name4 = "Instructor Three"
create_review("CIS-120-003", TEST4_SEMESTER, self.instructor_name4, {"instructor_quality": 0})
self.precompute_reviews()
num_reviews = CachedReviewResponse.objects.count()
self.assertEqual(num_reviews, 2)
response = CachedReviewResponse.objects.all().order_by("topic_id").first().response
self.assertEqual(response["average_reviews"]["rInstructorQuality"], 2)

def test_change_topic_recompute(self):
old_course = Course.objects.filter(full_code="CIS-120").first()
new_course = Course.objects.filter(full_code="CIS-1200").first()
old_topic = old_course.topic
new_topic = new_course.topic
new_course.topic = old_topic
new_course.save()
Topic.objects.filter(id=new_topic.id).first().delete()

self.set_runtime_option()
self.precompute_reviews()
num_reviews = CachedReviewResponse.objects.count()
self.assertEqual(num_reviews, 1)
response = CachedReviewResponse.objects.all().first().response
response = CachedReviewResponse.objects.all().order_by("topic_id").first().response
self.assertEqual(response["average_reviews"]["rInstructorQuality"], 2)

0 comments on commit dd91f16

Please sign in to comment.