From 232d99d06da06d5534823b4775c84999efe3d891 Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Wed, 14 Sep 2016 10:34:21 -0400 Subject: [PATCH] Persisted grades optimization - update query counts in tests --- .../tests/test_field_override_performance.py | 55 +++++---- lms/djangoapps/courseware/tests/test_views.py | 112 ++++++++---------- lms/djangoapps/grades/tests/test_signals.py | 4 +- .../tests/test_tasks_helper.py | 14 ++- 4 files changed, 91 insertions(+), 94 deletions(-) diff --git a/lms/djangoapps/ccx/tests/test_field_override_performance.py b/lms/djangoapps/ccx/tests/test_field_override_performance.py index bae449cf10..d06db8fd6f 100644 --- a/lms/djangoapps/ccx/tests/test_field_override_performance.py +++ b/lms/djangoapps/ccx/tests/test_field_override_performance.py @@ -36,7 +36,6 @@ from openedx.core.djangoapps.content.block_structure.api import get_course_in_ca 'django.conf.settings.FEATURES', { 'ENABLE_XBLOCK_VIEW_ENDPOINT': True, - 'PERSISTENT_GRADES_ENABLED_FOR_ALL_TESTS': False # disable persistent grades until TNL-5458 (reduces queries) } ) @ddt.ddt @@ -230,18 +229,18 @@ class TestFieldOverrideMongoPerformance(FieldOverridePerformanceTestCase): # # of sql queries to default, # # of mongo queries, # ) - ('no_overrides', 1, True, False): (17, 6), - ('no_overrides', 2, True, False): (17, 6), - ('no_overrides', 3, True, False): (17, 6), - ('ccx', 1, True, False): (17, 6), - ('ccx', 2, True, False): (17, 6), - ('ccx', 3, True, False): (17, 6), - ('no_overrides', 1, False, False): (17, 6), - ('no_overrides', 2, False, False): (17, 6), - ('no_overrides', 3, False, False): (17, 6), - ('ccx', 1, False, False): (17, 6), - ('ccx', 2, False, False): (17, 6), - ('ccx', 3, False, False): (17, 6), + ('no_overrides', 1, True, False): (20, 6), + ('no_overrides', 2, True, False): (20, 6), + ('no_overrides', 3, True, False): (20, 6), + ('ccx', 1, True, False): (20, 6), + ('ccx', 2, True, False): (20, 6), + ('ccx', 3, True, False): (20, 6), + ('no_overrides', 1, False, False): (20, 6), + ('no_overrides', 2, False, False): (20, 6), + ('no_overrides', 3, False, False): (20, 6), + ('ccx', 1, False, False): (20, 6), + ('ccx', 2, False, False): (20, 6), + ('ccx', 3, False, False): (20, 6), } @@ -253,19 +252,19 @@ class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase): __test__ = True TEST_DATA = { - ('no_overrides', 1, True, False): (17, 3), - ('no_overrides', 2, True, False): (17, 3), - ('no_overrides', 3, True, False): (17, 3), - ('ccx', 1, True, False): (17, 3), - ('ccx', 2, True, False): (17, 3), - ('ccx', 3, True, False): (17, 3), - ('ccx', 1, True, True): (18, 3), - ('ccx', 2, True, True): (18, 3), - ('ccx', 3, True, True): (18, 3), - ('no_overrides', 1, False, False): (17, 3), - ('no_overrides', 2, False, False): (17, 3), - ('no_overrides', 3, False, False): (17, 3), - ('ccx', 1, False, False): (17, 3), - ('ccx', 2, False, False): (17, 3), - ('ccx', 3, False, False): (17, 3), + ('no_overrides', 1, True, False): (20, 3), + ('no_overrides', 2, True, False): (20, 3), + ('no_overrides', 3, True, False): (20, 3), + ('ccx', 1, True, False): (20, 3), + ('ccx', 2, True, False): (20, 3), + ('ccx', 3, True, False): (20, 3), + ('ccx', 1, True, True): (21, 3), + ('ccx', 2, True, True): (21, 3), + ('ccx', 3, True, True): (21, 3), + ('no_overrides', 1, False, False): (20, 3), + ('no_overrides', 2, False, False): (20, 3), + ('no_overrides', 3, False, False): (20, 3), + ('ccx', 1, False, False): (20, 3), + ('ccx', 2, False, False): (20, 3), + ('ccx', 3, False, False): (20, 3), } diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index 2906a4f2af..a73ae740c0 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -1136,25 +1136,38 @@ class ProgressPageTests(ModuleStoreTestCase): self.section = ItemFactory.create(category='sequential', parent_location=self.chapter.location) self.vertical = ItemFactory.create(category='vertical', parent_location=self.section.location) + def _get_progress_page(self, expected_status_code=200): + """ + Gets the progress page for the user in the course. + """ + resp = self.client.get( + reverse('progress', args=[unicode(self.course.id)]) + ) + self.assertEqual(resp.status_code, expected_status_code) + return resp + + def _get_student_progress_page(self, expected_status_code=200): + """ + Gets the progress page for the user in the course. + """ + resp = self.client.get( + reverse('student_progress', args=[unicode(self.course.id), self.user.id]) + ) + self.assertEqual(resp.status_code, expected_status_code) + return resp + @ddt.data('">', '', '') def test_progress_page_xss_prevent(self, malicious_code): """ Test that XSS attack is prevented """ - resp = self.client.get( - reverse('student_progress', args=[unicode(self.course.id), self.user.id]) - ) - self.assertEqual(resp.status_code, 200) + resp = self._get_student_progress_page() # Test that malicious code does not appear in html self.assertNotIn(malicious_code, resp.content) def test_pure_ungraded_xblock(self): ItemFactory.create(category='acid', parent_location=self.vertical.location) - - resp = self.client.get( - reverse('progress', args=[unicode(self.course.id)]) - ) - self.assertEqual(resp.status_code, 200) + self._get_progress_page() @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) def test_student_progress_with_valid_and_invalid_id(self, default_store): @@ -1180,11 +1193,8 @@ class ProgressPageTests(ModuleStoreTestCase): ) self.assertEquals(resp.status_code, 404) - resp = self.client.get( - reverse('student_progress', args=[unicode(self.course.id), self.user.id]) - ) # Assert that valid 'student_id' returns 200 status - self.assertEqual(resp.status_code, 200) + self._get_student_progress_page() @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) def test_unenrolled_student_progress_for_credit_course(self, default_store): @@ -1219,38 +1229,26 @@ class ProgressPageTests(ModuleStoreTestCase): # Add a single credit requirement (final grade) set_credit_requirements(course.id, requirements) - resp = self.client.get( - reverse('student_progress', args=[unicode(course.id), not_enrolled_user.id]) - ) - self.assertEqual(resp.status_code, 200) + self._get_student_progress_page() def test_non_ascii_grade_cutoffs(self): - resp = self.client.get( - reverse('progress', args=[unicode(self.course.id)]) - ) - self.assertEqual(resp.status_code, 200) + self._get_progress_page() def test_generate_cert_config(self): - resp = self.client.get( - reverse('progress', args=[unicode(self.course.id)]) - ) + resp = self._get_progress_page() self.assertNotContains(resp, 'Request Certificate') # Enable the feature, but do not enable it for this course CertificateGenerationConfiguration(enabled=True).save() - resp = self.client.get( - reverse('progress', args=[unicode(self.course.id)]) - ) + resp = self._get_progress_page() self.assertNotContains(resp, 'Request Certificate') # Enable certificate generation for this course certs_api.set_cert_generation_enabled(self.course.id, True) - resp = self.client.get( - reverse('progress', args=[unicode(self.course.id)]) - ) + resp = self._get_progress_page() self.assertNotContains(resp, 'Request Certificate') @patch.dict('django.conf.settings.FEATURES', {'CERTIFICATES_HTML_VIEW': True}) @@ -1295,9 +1293,7 @@ class ProgressPageTests(ModuleStoreTestCase): self.course.save() self.store.update_item(self.course, self.user.id) - resp = self.client.get( - reverse('progress', args=[unicode(self.course.id)]) - ) + resp = self._get_progress_page() self.assertContains(resp, u"View Certificate") self.assertContains(resp, u"You can keep working for a higher grade") @@ -1308,9 +1304,7 @@ class ProgressPageTests(ModuleStoreTestCase): certificates[0]['is_active'] = False self.store.update_item(self.course, self.user.id) - resp = self.client.get( - reverse('progress', args=[unicode(self.course.id)]) - ) + resp = self._get_progress_page() self.assertNotContains(resp, u"View Your Certificate") self.assertNotContains(resp, u"You can now view your certificate") self.assertContains(resp, "working on it...") @@ -1340,26 +1334,29 @@ class ProgressPageTests(ModuleStoreTestCase): # Enable certificate generation for this course certs_api.set_cert_generation_enabled(self.course.id, True) - resp = self.client.get( - reverse('progress', args=[unicode(self.course.id)]) - ) + resp = self._get_progress_page() self.assertContains(resp, u"Download Your Certificate") - # disable persistent grades until TNL-5458 (reduces query counts) - @patch.dict(settings.FEATURES, {'PERSISTENT_GRADES_ENABLED_FOR_ALL_TESTS': False}) @ddt.data( - *itertools.product(((34, 4, True), (34, 4, False)), (True, False)) + *itertools.product((True, False), (True, False)) ) @ddt.unpack - def test_query_counts(self, (sql_calls, mongo_calls, self_paced), self_paced_enabled): + def test_progress_queries_paced_courses(self, self_paced, self_paced_enabled): """Test that query counts remain the same for self-paced and instructor-paced courses.""" SelfPacedConfiguration(enabled=self_paced_enabled).save() self.setup_course(self_paced=self_paced) - with self.assertNumQueries(sql_calls), check_mongo_calls(mongo_calls): - resp = self.client.get( - reverse('progress', args=[unicode(self.course.id)]) - ) - self.assertEqual(resp.status_code, 200) + with self.assertNumQueries(37), check_mongo_calls(4): + self._get_progress_page() + + def test_progress_queries(self): + self.setup_course() + with self.assertNumQueries(37), check_mongo_calls(4): + self._get_progress_page() + + # subsequent accesses to the progress page require fewer queries. + for _ in range(2): + with self.assertNumQueries(20), check_mongo_calls(4): + self._get_progress_page() @patch( 'lms.djangoapps.grades.new.course_grade.CourseGrade.summary', @@ -1397,7 +1394,8 @@ class ProgressPageTests(ModuleStoreTestCase): self.assertEqual( cert_button_hidden, - 'Request Certificate' not in resp.content) + 'Request Certificate' not in resp.content + ) @patch.dict('django.conf.settings.FEATURES', {'CERTIFICATES_HTML_VIEW': True}) @patch( @@ -1430,9 +1428,7 @@ class ProgressPageTests(ModuleStoreTestCase): self.course.save() self.store.update_item(self.course, self.user.id) - resp = self.client.get( - reverse('progress', args=[unicode(self.course.id)]) - ) + resp = self._get_progress_page() self.assertContains(resp, u"View Certificate") self.assert_invalidate_certificate(generated_certificate) @@ -1449,9 +1445,7 @@ class ProgressPageTests(ModuleStoreTestCase): "http://www.example.com/certificate.pdf", "honor" ) - resp = self.client.get( - reverse('progress', args=[unicode(self.course.id)]) - ) + resp = self._get_progress_page() self.assertContains(resp, u'Download Your Certificate') self.assert_invalidate_certificate(generated_certificate) @@ -1466,9 +1460,7 @@ class ProgressPageTests(ModuleStoreTestCase): user = UserFactory.create() self.assertTrue(self.client.login(username=user.username, password='test')) CourseEnrollmentFactory(user=user, course_id=self.course.id, mode=CourseMode.AUDIT) - response = self.client.get( - reverse('progress', args=[unicode(self.course.id)]) - ) + response = self._get_progress_page() self.assertContains( response, @@ -1557,9 +1549,7 @@ class ProgressPageTests(ModuleStoreTestCase): ) # Invalidate user certificate certificate.invalidate() - resp = self.client.get( - reverse('progress', args=[unicode(self.course.id)]) - ) + resp = self._get_progress_page() self.assertNotContains(resp, u'Request Certificate') self.assertContains(resp, u'Your certificate has been invalidated') diff --git a/lms/djangoapps/grades/tests/test_signals.py b/lms/djangoapps/grades/tests/test_signals.py index 7485b98cc8..2054f967f5 100644 --- a/lms/djangoapps/grades/tests/test_signals.py +++ b/lms/djangoapps/grades/tests/test_signals.py @@ -216,7 +216,7 @@ class ScoreChangedUpdatesSubsectionGradeTest(ModuleStoreTestCase): with self.store.default_store(default_store): self.set_up_course() self.assertTrue(PersistentGradesEnabledFlag.feature_enabled(self.course.id)) - with check_mongo_calls(2) and self.assertNumQueries(15): + with check_mongo_calls(2) and self.assertNumQueries(11): recalculate_subsection_grade_handler(None, **self.score_changed_kwargs) def test_single_call_to_create_block_structure(self): @@ -236,7 +236,7 @@ class ScoreChangedUpdatesSubsectionGradeTest(ModuleStoreTestCase): self.assertTrue(PersistentGradesEnabledFlag.feature_enabled(self.course.id)) ItemFactory.create(parent=self.sequential, category='problem', display_name='problem2') ItemFactory.create(parent=self.sequential, category='problem', display_name='problem3') - with check_mongo_calls(2) and self.assertNumQueries(15): + with check_mongo_calls(2) and self.assertNumQueries(11): recalculate_subsection_grade_handler(None, **self.score_changed_kwargs) @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index b6d6fad3c3..21f693d246 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -1641,8 +1641,6 @@ class TestCertificateGeneration(InstructorTaskModuleTestCase): super(TestCertificateGeneration, self).setUp() self.initialize_course() - # disable persistent grades until TNL-5458 (reduces query counts) - @patch.dict(settings.FEATURES, {'PERSISTENT_GRADES_ENABLED_FOR_ALL_TESTS': False}) def test_certificate_generation_for_students(self): """ Verify that certificates generated for all eligible students enrolled in a course. @@ -1672,8 +1670,18 @@ class TestCertificateGeneration(InstructorTaskModuleTestCase): 'failed': 3, 'skipped': 2 } + with self.assertNumQueries(175): + self.assertCertificatesGenerated(task_input, expected_results) - with self.assertNumQueries(151): + expected_results = { + 'action_name': 'certificates generated', + 'total': 10, + 'attempted': 0, + 'succeeded': 0, + 'failed': 0, + 'skipped': 10 + } + with self.assertNumQueries(3): self.assertCertificatesGenerated(task_input, expected_results) @ddt.data(