From e46ce7f7d0f15c9d81287823fc29f12e9d7a17dc Mon Sep 17 00:00:00 2001 From: jenniw Date: Fri, 27 Oct 2023 16:41:51 -0400 Subject: [PATCH] Course API test updates (#1962) * Added tests, fixture and utility functions * oops, put nplus 1 back & reparameterize the fixture * reformat * updated per Nathan's feedback * Example of catalog data fixture * reformat --------- Co-authored-by: Nathan Levesque --- courses/conftest.py | 113 +++++++++++++++++++ courses/views/v1/__init__.py | 2 +- courses/views_test.py | 212 +++++++++++++++++++++++++---------- fixtures/common.py | 10 ++ main/test_utils.py | 29 ++++- poetry.lock | 34 +++++- pyproject.toml | 1 + 7 files changed, 335 insertions(+), 66 deletions(-) create mode 100644 courses/conftest.py diff --git a/courses/conftest.py b/courses/conftest.py new file mode 100644 index 0000000000..590f9806ee --- /dev/null +++ b/courses/conftest.py @@ -0,0 +1,113 @@ +"""Shared pytest configuration for courses application""" +import random + +import pytest + +from courses.factories import ( + CourseFactory, + CourseRunFactory, + ProgramFactory, + ProgramRequirementFactory, +) +from courses.models import ( + ProgramRequirementNodeType, + ProgramRequirement, +) + + +@pytest.fixture() +def programs(): + """Fixture for a set of Programs in the database""" + return ProgramFactory.create_batch(3) + + +@pytest.fixture() +def courses(): + """Fixture for a set of Courses in the database""" + return CourseFactory.create_batch(3) + + +@pytest.fixture() +def course_runs(): + """Fixture for a set of CourseRuns in the database""" + return CourseRunFactory.create_batch(3) + + +@pytest.fixture() +def course_catalog_program_count(request): + return getattr(request, "param", 5) + + +@pytest.fixture() +def course_catalog_course_count(request): + return getattr(request, "param", 10) + + +@pytest.fixture() +def course_catalog_data(course_catalog_program_count, course_catalog_course_count): + """ + Current production data is around 85 courses and 150 course runs. I opted to create 3 of each to allow + the best course run logic to play out as well as to push the endpoint a little harder in testing. + + There are currently 3 programs in production, so I went with 15, again, to get ready for more data. To allow + things to be somewhat random, I grab courses for them at random (one required and one elective) which also allows + for courses to be in more than one program. If we need/want more specific test cases, we can add them, but this + is a more robust data set than production is presently. + + Returns 3 separate lists to simulate what the tests received prior. + + Args: + num_courses(int): number of courses to generate. + num_programs(int): number of programs to generate. + """ + programs = [] + courses = [] + course_runs = [] + for n in range(course_catalog_course_count): + course, course_runs_for_course = _create_course(n) + courses.append(course) + course_runs.append(course_runs_for_course) + for n in range(course_catalog_program_count): + program = _create_program(courses) + programs.append(program) + return courses, programs, course_runs + + +def _create_course(n): + test_course = CourseFactory.create(title=f"Test Course {n}") + cr1 = CourseRunFactory.create(course=test_course, past_start=True) + cr2 = CourseRunFactory.create(course=test_course, in_progress=True) + cr3 = CourseRunFactory.create(course=test_course, in_future=True) + return test_course, [cr1, cr2, cr3] + + +def _create_program(courses): + program = ProgramFactory.create() + ProgramRequirementFactory.add_root(program) + root_node = program.requirements_root + required_courses_node = root_node.add_child( + node_type=ProgramRequirementNodeType.OPERATOR, + operator=ProgramRequirement.Operator.ALL_OF, + title="Required Courses", + ) + elective_courses_node = root_node.add_child( + node_type=ProgramRequirementNodeType.OPERATOR, + operator=ProgramRequirement.Operator.MIN_NUMBER_OF, + operator_value=2, + title="Elective Courses", + elective_flag=True, + ) + if len(courses) > 3: + for c in random.sample(courses, 3): + required_courses_node.add_child( + node_type=ProgramRequirementNodeType.COURSE, course=c + ) + for c in random.sample(courses, 3): + elective_courses_node.add_child( + node_type=ProgramRequirementNodeType.COURSE, course=c + ) + else: + required_courses_node.add_child( + node_type=ProgramRequirementNodeType.COURSE, course=courses[0] + ) + return program diff --git a/courses/views/v1/__init__.py b/courses/views/v1/__init__.py index 3eca23f615..0342ea5a5f 100644 --- a/courses/views/v1/__init__.py +++ b/courses/views/v1/__init__.py @@ -1,4 +1,4 @@ -"""Course views verson 1""" +"""Course views version 1""" import logging from typing import Optional, Tuple, Union diff --git a/courses/views_test.py b/courses/views_test.py index c16a01d65d..9fcdb88da1 100644 --- a/courses/views_test.py +++ b/courses/views_test.py @@ -19,10 +19,11 @@ CourseFactory, CourseRunEnrollmentFactory, CourseRunFactory, - ProgramFactory, - program_with_empty_requirements, ) -from courses.models import CourseRun, ProgramEnrollment +from courses.models import ( + CourseRun, + ProgramEnrollment, +) from courses.serializers import ( CourseRunEnrollmentSerializer, CourseRunSerializer, @@ -33,6 +34,7 @@ from courses.views.v1 import UserEnrollmentsApiViewSet from ecommerce.factories import LineFactory, OrderFactory, ProductFactory from ecommerce.models import Order +from fixtures.common import raise_nplusone from main import features from main.constants import ( USER_MSG_COOKIE_NAME, @@ -40,32 +42,44 @@ USER_MSG_TYPE_ENROLL_FAILED, USER_MSG_TYPE_ENROLLED, ) -from main.test_utils import assert_drf_json_equal +from main.test_utils import assert_drf_json_equal, duplicate_queries_check from main.utils import encode_json_cookie_value from openedx.exceptions import NoEdxApiAuthError -pytestmark = [pytest.mark.django_db] - - -EXAMPLE_URL = "http://example.com" - -@pytest.fixture() -def programs(): - """Fixture for a set of Programs in the database""" - return ProgramFactory.create_batch(3) +pytestmark = [pytest.mark.django_db, pytest.mark.usefixtures("raise_nplusone")] -@pytest.fixture() -def courses(): - """Fixture for a set of Courses in the database""" - return CourseFactory.create_batch(3) +EXAMPLE_URL = "http://example.com" -@pytest.fixture() -def course_runs(): - """Fixture for a set of CourseRuns in the database""" - return CourseRunFactory.create_batch(3) +def _num_queries_from_course(course): + """ + Generates approximately the number of queries we should expect to see, in a worst case scenario. This is + difficult to predict without weighing down the test more as it traverses a bunch of wagtail and other related models. + New endpoints should solve this, but the v1 endpoints will not change until/unless they are modified. + + programs see about 9 hits right now: + - 4 are duplicated grabbing related courses + - 3 grab flexible pricing data + - 1 grabs content types related to it + - 1 grabs the content of that content type + + course sees about 22 - this number varies on flexible pricing, wagtail data, and some relations with other objects + - 12 are grabbing related objects both course objects and wagtail objects + - 6 are grabbing flexible pricing + - 4 are grabbing wagtail objects (page, image, etc) + + course runs grab about 6 (this varies if there's a relation to pricing) + - ~4 are wagtail related - this is where things get hazy + - 2 are checking relations + + Args: + course (object): course object + """ + num_programs = len(course.programs) + num_course_runs = course.courseruns.count() + return (9 * num_programs) + (num_course_runs * 6) + 22 def test_get_programs(user_drf_client, programs): @@ -115,31 +129,68 @@ def test_delete_program(user_drf_client, programs): assert resp.status_code == status.HTTP_405_METHOD_NOT_ALLOWED -def test_get_courses(user_drf_client, courses, mock_context): +@pytest.mark.parametrize("course_catalog_course_count", [100], indirect=True) +@pytest.mark.parametrize("course_catalog_program_count", [15], indirect=True) +def test_get_courses( + user_drf_client, mock_context, django_assert_max_num_queries, course_catalog_data +): """Test the view that handles requests for all Courses""" - resp = user_drf_client.get(reverse("courses_api-list")) - courses_data = resp.json() - assert len(courses_data) == len(courses) + courses, _, _ = course_catalog_data + courses_from_fixture = [] + num_queries = 0 for course in courses: - assert ( + courses_from_fixture.append( CourseWithCourseRunsSerializer(instance=course, context=mock_context).data - in courses_data ) + num_queries += _num_queries_from_course(course) + with django_assert_max_num_queries(num_queries) as context: + resp = user_drf_client.get(reverse("courses_api-list")) + # This will become an assert rather than a warning in the future, for now this function is informational + duplicate_queries_check(context) + courses_data = resp.json() + assert len(courses_data) == len(courses_from_fixture) + """ + Due to the number of relations in our current course endpoint, and the potential for re-ordering of those nested + objects, deepdiff has an ignore_order flag which I've added with an optional boolean argument to the assert_drf_json + function. + """ + assert_drf_json_equal(courses_data, courses_from_fixture, ignore_order=True) -def test_get_course(user_drf_client, courses, mock_context): +@pytest.mark.parametrize("course_catalog_course_count", [1], indirect=True) +@pytest.mark.parametrize("course_catalog_program_count", [1], indirect=True) +def test_get_course( + user_drf_client, + course_catalog_data, + mock_context, + django_assert_max_num_queries, +): """Test the view that handles a request for single Course""" + courses, _, _ = course_catalog_data course = courses[0] - resp = user_drf_client.get(reverse("courses_api-detail", kwargs={"pk": course.id})) + num_queries = _num_queries_from_course(course) + with django_assert_max_num_queries(num_queries) as context: + resp = user_drf_client.get( + reverse("courses_api-detail", kwargs={"pk": course.id}) + ) + duplicate_queries_check(context) course_data = resp.json() - assert ( - course_data - == CourseWithCourseRunsSerializer(instance=course, context=mock_context).data + course_from_fixture = dict( + CourseWithCourseRunsSerializer(instance=course, context=mock_context).data ) + assert_drf_json_equal(course_data, course_from_fixture, ignore_order=True) -def test_create_course(user_drf_client, courses, mock_context): +@pytest.mark.parametrize("course_catalog_course_count", [1], indirect=True) +@pytest.mark.parametrize("course_catalog_program_count", [1], indirect=True) +def test_create_course( + user_drf_client, + course_catalog_data, + mock_context, + django_assert_max_num_queries, +): """Test the view that handles a request to create a Course""" + courses, _, _ = course_catalog_data course = courses[0] course_data = CourseWithCourseRunsSerializer( instance=course, context=mock_context @@ -147,30 +198,48 @@ def test_create_course(user_drf_client, courses, mock_context): del course_data["id"] course_data["title"] = "New Course Title" request_url = reverse("courses_api-list") - resp = user_drf_client.post(request_url, course_data) + with django_assert_max_num_queries(1) as context: + resp = user_drf_client.post(request_url, course_data) + duplicate_queries_check(context) assert resp.status_code == status.HTTP_405_METHOD_NOT_ALLOWED -def test_patch_course(user_drf_client, courses): +@pytest.mark.parametrize("course_catalog_course_count", [1], indirect=True) +@pytest.mark.parametrize("course_catalog_program_count", [1], indirect=True) +def test_patch_course( + user_drf_client, course_catalog_data, django_assert_max_num_queries +): """Test the view that handles a request to patch a Course""" + courses, _, _ = course_catalog_data course = courses[0] request_url = reverse("courses_api-detail", kwargs={"pk": course.id}) - resp = user_drf_client.patch(request_url, {"title": "New Course Title"}) + with django_assert_max_num_queries(1) as context: + resp = user_drf_client.patch(request_url, {"title": "New Course Title"}) + duplicate_queries_check(context) assert resp.status_code == status.HTTP_405_METHOD_NOT_ALLOWED -def test_delete_course(user_drf_client, courses): +@pytest.mark.parametrize("course_catalog_course_count", [1], indirect=True) +@pytest.mark.parametrize("course_catalog_program_count", [1], indirect=True) +def test_delete_course( + user_drf_client, course_catalog_data, django_assert_max_num_queries +): """Test the view that handles a request to delete a Course""" + courses, _, _ = course_catalog_data course = courses[0] - resp = user_drf_client.delete( - reverse("courses_api-detail", kwargs={"pk": course.id}) - ) + with django_assert_max_num_queries(1) as context: + resp = user_drf_client.delete( + reverse("courses_api-detail", kwargs={"pk": course.id}) + ) + duplicate_queries_check(context) assert resp.status_code == status.HTTP_405_METHOD_NOT_ALLOWED -def test_get_course_runs(user_drf_client, course_runs): +def test_get_course_runs(user_drf_client, course_runs, django_assert_max_num_queries): """Test the view that handles requests for all CourseRuns""" - resp = user_drf_client.get(reverse("course_runs_api-list")) + with django_assert_max_num_queries(38) as context: + resp = user_drf_client.get(reverse("course_runs_api-list")) + duplicate_queries_check(context) course_runs_data = resp.json() assert len(course_runs_data) == len(course_runs) # Force sorting by run id since this test has been flaky @@ -181,7 +250,12 @@ def test_get_course_runs(user_drf_client, course_runs): @pytest.mark.parametrize("is_enrolled", [True, False]) def test_get_course_runs_relevant( - mocker, user_drf_client, course_runs, user, is_enrolled + mocker, + user_drf_client, + course_runs, + user, + is_enrolled, + django_assert_max_num_queries, ): """A GET request for course runs with a `relevant_to` parameter should return user-relevant course runs""" course_run = course_runs[0] @@ -203,35 +277,43 @@ def test_get_course_runs_relevant( if is_enrolled: CourseRunEnrollmentFactory.create(user=user, run=course_run, edx_enrolled=True) - resp = user_drf_client.get( - f"{reverse('course_runs_api-list')}?relevant_to={course_run.course.readable_id}" - ) + with django_assert_max_num_queries(20) as context: + resp = user_drf_client.get( + f"{reverse('course_runs_api-list')}?relevant_to={course_run.course.readable_id}" + ) + duplicate_queries_check(context) patched_run_qset.assert_called_once_with(course_run.course, user) course_run_data = resp.json()[0] assert course_run_data["is_enrolled"] == is_enrolled -def test_get_course_runs_relevant_missing(user_drf_client): +def test_get_course_runs_relevant_missing( + user_drf_client, django_assert_max_num_queries +): """A GET request for course runs with an invalid `relevant_to` query parameter should return empty results""" - resp = user_drf_client.get( - f"{reverse('course_runs_api-list')}?relevant_to=invalid+course+id" - ) + with django_assert_max_num_queries(3) as context: + resp = user_drf_client.get( + f"{reverse('course_runs_api-list')}?relevant_to=invalid+course+id" + ) + duplicate_queries_check(context) course_runs_data = resp.json() assert course_runs_data == [] -def test_get_course_run(user_drf_client, course_runs): +def test_get_course_run(user_drf_client, course_runs, django_assert_max_num_queries): """Test the view that handles a request for single CourseRun""" course_run = course_runs[0] - resp = user_drf_client.get( - reverse("course_runs_api-detail", kwargs={"pk": course_run.id}) - ) + with django_assert_max_num_queries(18) as context: + resp = user_drf_client.get( + reverse("course_runs_api-detail", kwargs={"pk": course_run.id}) + ) + duplicate_queries_check(context) course_run_data = resp.json() assert course_run_data == CourseRunWithCourseSerializer(course_run).data -def test_create_course_run(user_drf_client, course_runs): +def test_create_course_run(user_drf_client, course_runs, django_assert_max_num_queries): """Test the view that handles a request to create a CourseRun""" course_run = course_runs[0] course_run_data = CourseRunSerializer(course_run).data @@ -244,24 +326,30 @@ def test_create_course_run(user_drf_client, course_runs): } ) request_url = reverse("course_runs_api-list") - resp = user_drf_client.post(request_url, course_run_data) + with django_assert_max_num_queries(1) as context: + resp = user_drf_client.post(request_url, course_run_data) + duplicate_queries_check(context) assert resp.status_code == status.HTTP_405_METHOD_NOT_ALLOWED -def test_patch_course_run(user_drf_client, course_runs): +def test_patch_course_run(user_drf_client, course_runs, django_assert_max_num_queries): """Test the view that handles a request to patch a CourseRun""" course_run = course_runs[0] request_url = reverse("course_runs_api-detail", kwargs={"pk": course_run.id}) - resp = user_drf_client.patch(request_url, {"title": "New CourseRun Title"}) + with django_assert_max_num_queries(1) as context: + resp = user_drf_client.patch(request_url, {"title": "New CourseRun Title"}) + duplicate_queries_check(context) assert resp.status_code == status.HTTP_405_METHOD_NOT_ALLOWED -def test_delete_course_run(user_drf_client, course_runs): +def test_delete_course_run(user_drf_client, course_runs, django_assert_max_num_queries): """Test the view does not handle a request to delete a CourseRun""" course_run = course_runs[0] - resp = user_drf_client.delete( - reverse("course_runs_api-detail", kwargs={"pk": course_run.id}) - ) + with django_assert_max_num_queries(1) as context: + resp = user_drf_client.delete( + reverse("course_runs_api-detail", kwargs={"pk": course_run.id}) + ) + duplicate_queries_check(context) assert resp.status_code == status.HTTP_405_METHOD_NOT_ALLOWED diff --git a/fixtures/common.py b/fixtures/common.py index 564e21cde4..8721980984 100644 --- a/fixtures/common.py +++ b/fixtures/common.py @@ -5,6 +5,7 @@ import pytest import responses from django.test.client import Client +from nplusone.core import profiler from rest_framework.test import APIClient from courses.factories import CourseFactory, ProgramFactory, ProgramRequirementFactory @@ -147,3 +148,12 @@ def webpack_stats(settings): loader_config["STATS_FILE"] = os.path.join( settings.BASE_DIR, directory, "default.json" ) + + +@pytest.fixture() +def raise_nplusone(request): + if request.node.get_closest_marker("skip_nplusone"): + yield + else: + with profiler.Profiler(): + yield diff --git a/main/test_utils.py b/main/test_utils.py index ba0d9b0386..876a0f082b 100644 --- a/main/test_utils.py +++ b/main/test_utils.py @@ -2,12 +2,15 @@ import abc import csv import json +import logging import tempfile import traceback +from collections import Counter from contextlib import contextmanager from unittest.mock import Mock import pytest +from deepdiff import DeepDiff from django.contrib.sessions.middleware import SessionMiddleware from django.core.files.uploadedfile import SimpleUploadedFile from requests.exceptions import HTTPError @@ -25,7 +28,7 @@ def assert_not_raises(): pytest.fail(f"An exception was not raised: {traceback.format_exc()}") -def assert_drf_json_equal(obj1, obj2): +def assert_drf_json_equal(obj1, obj2, ignore_order=False): """ Asserts that two objects are equal after a round trip through JSON serialization/deserialization. Particularly helpful when testing DRF serializers where you may get back OrderedDict and other such objects. @@ -33,11 +36,15 @@ def assert_drf_json_equal(obj1, obj2): Args: obj1 (object): the first object obj2 (object): the second object + ignore_order (bool): Boolean to ignore the order in the result """ json_renderer = JSONRenderer() converted1 = json.loads(json_renderer.render(obj1)) converted2 = json.loads(json_renderer.render(obj2)) - assert converted1 == converted2 + if ignore_order: + assert DeepDiff(converted1, converted2, ignore_order=ignore_order) == {} + else: + assert converted1 == converted2 class MockResponse: @@ -167,3 +174,21 @@ def update_namespace(tuple_to_update, **updates): **updates, } ) + + +def duplicate_queries_check(context): + """ + For now this is informational until we fix the queries, then we will swap this over + to an assertion that captured_queries_list == list(set(captured_queries_list)) + """ + captured_queries_list = [ + query_dict["sql"] for query_dict in context.captured_queries + ] + total_queries = len(captured_queries_list) + count_of_requests = Counter(captured_queries_list) + if max(count_of_requests.values()) > 1: + logger = logging.getLogger() + dupes = [value for query, value in count_of_requests.items() if value > 1] + logger.info( + f"{len(dupes)} out of {total_queries} queries duplicated", stacklevel=2 + ) diff --git a/poetry.lock b/poetry.lock index 64b29d78f4..3ac596bfde 100644 --- a/poetry.lock +++ b/poetry.lock @@ -980,6 +980,24 @@ files = [ {file = "decorator-5.1.1.tar.gz", hash = "sha256:637996211036b6385ef91435e4fae22989472f9d571faba8927ba8253acbc330"}, ] +[[package]] +name = "deepdiff" +version = "6.6.1" +description = "Deep Difference and Search of any Python object/data. Recreate objects by adding adding deltas to each other." +optional = false +python-versions = ">=3.7" +files = [ + {file = "deepdiff-6.6.1-py3-none-any.whl", hash = "sha256:891b3cb12837e5d376ac0b58f4c8a2764e3a8bbceabb7108ff82235f1f2c4460"}, + {file = "deepdiff-6.6.1.tar.gz", hash = "sha256:75c75b1511f0e48edef2b70d785a9c32b2631666b465fa8c32270a77a7b950b5"}, +] + +[package.dependencies] +ordered-set = ">=4.0.2,<4.2.0" + +[package.extras] +cli = ["click (==8.1.3)", "pyyaml (==6.0.1)"] +optimize = ["orjson"] + [[package]] name = "defusedxml" version = "0.7.1" @@ -2672,6 +2690,20 @@ files = [ [package.dependencies] et-xmlfile = "*" +[[package]] +name = "ordered-set" +version = "4.1.0" +description = "An OrderedSet is a custom MutableSet that remembers its order, so that every" +optional = false +python-versions = ">=3.7" +files = [ + {file = "ordered-set-4.1.0.tar.gz", hash = "sha256:694a8e44c87657c59292ede72891eb91d34131f6531463aab3009191c77364a8"}, + {file = "ordered_set-4.1.0-py3-none-any.whl", hash = "sha256:046e1132c71fcf3330438a539928932caf51ddbc582496833e23de611de14562"}, +] + +[package.extras] +dev = ["black", "mypy", "pytest"] + [[package]] name = "packaging" version = "23.1" @@ -4362,4 +4394,4 @@ testing = ["coverage (>=5.0.3)", "zope.event", "zope.testing"] [metadata] lock-version = "2.0" python-versions = "3.9.6" -content-hash = "8050bda472ea2797ae4e44274bad36d0587534a965f39ac2bf10e682d7367561" +content-hash = "8d185553cf53a8f6a75643af7ed7f2374af31ed3040de7e72452bfcd9910ec03" diff --git a/pyproject.toml b/pyproject.toml index 74d2669ae9..beccecc4d7 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -84,6 +84,7 @@ wagtail = "^5.0" hypothesis = "4.23.9" posthog = "^3.0.1" uwsgitop = "^0.11" +deepdiff = "^6.6.1" [tool.poetry.group.dev.dependencies]