From 660bc8f447d03bdb562dce388d178e9f144961b9 Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Fri, 9 Sep 2016 04:32:04 -0400 Subject: [PATCH 1/4] Enable Persistent Grades in unit tests --- lms/djangoapps/grades/config/models.py | 3 + .../grades/config/tests/test_models.py | 19 +++--- lms/djangoapps/grades/tests/test_new.py | 59 +++++++++---------- lms/djangoapps/grades/tests/test_signals.py | 2 + lms/envs/test.py | 3 + 5 files changed, 45 insertions(+), 41 deletions(-) diff --git a/lms/djangoapps/grades/config/models.py b/lms/djangoapps/grades/config/models.py index 05f852d08a..33f98d6dbd 100644 --- a/lms/djangoapps/grades/config/models.py +++ b/lms/djangoapps/grades/config/models.py @@ -3,6 +3,7 @@ Models for configuration of the feature flags controlling persistent grades. """ from config_models.models import ConfigurationModel +from django.conf import settings from django.db.models import BooleanField from xmodule_django.models import CourseKeyField @@ -29,6 +30,8 @@ class PersistentGradesEnabledFlag(ConfigurationModel): If the flag is enabled and no course ID is given, we return True since the global setting is enabled. """ + if settings.FEATURES.get('PERSISTENT_GRADES_ENABLED_FOR_ALL_TESTS'): + return True if not PersistentGradesEnabledFlag.is_enabled(): return False elif not PersistentGradesEnabledFlag.current().enabled_for_all_courses and course_id: diff --git a/lms/djangoapps/grades/config/tests/test_models.py b/lms/djangoapps/grades/config/tests/test_models.py index 2e4d5614c6..7ea3fe7b35 100644 --- a/lms/djangoapps/grades/config/tests/test_models.py +++ b/lms/djangoapps/grades/config/tests/test_models.py @@ -3,6 +3,9 @@ Tests for the models that control the persistent grading feature. """ import ddt +from django.conf import settings +import itertools +from mock import patch from django.test import TestCase from opaque_keys.edx.locator import CourseLocator @@ -10,6 +13,7 @@ from lms.djangoapps.grades.config.models import PersistentGradesEnabledFlag from lms.djangoapps.grades.config.tests.utils import persistent_grades_feature_flags +@patch.dict(settings.FEATURES, {'PERSISTENT_GRADES_ENABLED_FOR_ALL_TESTS': False}) @ddt.ddt class PersistentGradesFeatureFlagTests(TestCase): """ @@ -21,16 +25,11 @@ class PersistentGradesFeatureFlagTests(TestCase): self.course_id_1 = CourseLocator(org="edx", course="course", run="run") self.course_id_2 = CourseLocator(org="edx", course="course2", run="run") - @ddt.data( - (True, True, True), - (True, True, False), - (True, False, True), - (True, False, False), - (False, True, True), - (False, False, True), - (False, True, False), - (False, False, False), - ) + @ddt.data(*itertools.product( + (True, False), + (True, False), + (True, False), + )) @ddt.unpack def test_persistent_grades_feature_flags(self, global_flag, enabled_for_all_courses, enabled_for_course_1): with persistent_grades_feature_flags( diff --git a/lms/djangoapps/grades/tests/test_new.py b/lms/djangoapps/grades/tests/test_new.py index 0ddde1fe92..746f14959a 100644 --- a/lms/djangoapps/grades/tests/test_new.py +++ b/lms/djangoapps/grades/tests/test_new.py @@ -3,6 +3,7 @@ Test saved subsection grade functionality. """ import ddt +from django.conf import settings from mock import patch from capa.tests.response_xml_factory import MultipleChoiceResponseXMLFactory @@ -71,6 +72,7 @@ class TestCourseGradeFactory(GradeTestBase): Test that CourseGrades are calculated properly """ + @patch.dict(settings.FEATURES, {'PERSISTENT_GRADES_ENABLED_FOR_ALL_TESTS': False}) @ddt.data( (True, True), (True, False), @@ -107,44 +109,39 @@ class SubsectionGradeFactoryTest(GradeTestBase): """ Tests to ensure that a persistent subsection grade is created, saved, then fetched on re-request. """ - with persistent_grades_feature_flags( - global_flag=True, - enabled_for_all_courses=False, - course_id=self.course.id, - enabled_for_course=True - ): + with patch( + 'lms.djangoapps.grades.new.subsection_grade.SubsectionGradeFactory._save_grade', + wraps=self.subsection_grade_factory._save_grade # pylint: disable=protected-access + ) as mock_save_grades: with patch( - 'lms.djangoapps.grades.new.subsection_grade.SubsectionGradeFactory._save_grade', - wraps=self.subsection_grade_factory._save_grade # pylint: disable=protected-access - ) as mock_save_grades: - with patch( - 'lms.djangoapps.grades.new.subsection_grade.SubsectionGradeFactory._get_saved_grade', - wraps=self.subsection_grade_factory._get_saved_grade # pylint: disable=protected-access - ) as mock_get_saved_grade: - with self.assertNumQueries(22): - grade_a = self.subsection_grade_factory.create( - self.sequence, - self.course_structure, - self.course - ) - self.assertTrue(mock_get_saved_grade.called) - self.assertTrue(mock_save_grades.called) + 'lms.djangoapps.grades.new.subsection_grade.SubsectionGradeFactory._get_saved_grade', + wraps=self.subsection_grade_factory._get_saved_grade # pylint: disable=protected-access + ) as mock_get_saved_grade: + with self.assertNumQueries(19): + grade_a = self.subsection_grade_factory.create( + self.sequence, + self.course_structure, + self.course + ) + self.assertTrue(mock_get_saved_grade.called) + self.assertTrue(mock_save_grades.called) - mock_get_saved_grade.reset_mock() - mock_save_grades.reset_mock() + mock_get_saved_grade.reset_mock() + mock_save_grades.reset_mock() - with self.assertNumQueries(4): - grade_b = self.subsection_grade_factory.create( - self.sequence, - self.course_structure, - self.course - ) - self.assertTrue(mock_get_saved_grade.called) - self.assertFalse(mock_save_grades.called) + with self.assertNumQueries(3): + grade_b = self.subsection_grade_factory.create( + self.sequence, + self.course_structure, + self.course + ) + self.assertTrue(mock_get_saved_grade.called) + self.assertFalse(mock_save_grades.called) self.assertEqual(grade_a.url_name, grade_b.url_name) self.assertEqual(grade_a.all_total, grade_b.all_total) + @patch.dict(settings.FEATURES, {'PERSISTENT_GRADES_ENABLED_FOR_ALL_TESTS': False}) @ddt.data( (True, True), (True, False), diff --git a/lms/djangoapps/grades/tests/test_signals.py b/lms/djangoapps/grades/tests/test_signals.py index 2125944e47..7485b98cc8 100644 --- a/lms/djangoapps/grades/tests/test_signals.py +++ b/lms/djangoapps/grades/tests/test_signals.py @@ -3,6 +3,7 @@ Tests for the score change signals defined in the courseware models module. """ import ddt +from django.conf import settings from django.test import TestCase from mock import patch, MagicMock from unittest import skip @@ -169,6 +170,7 @@ class SubmissionSignalRelayTest(TestCase): self.signal_mock.assert_not_called() +@patch.dict(settings.FEATURES, {'PERSISTENT_GRADES_ENABLED_FOR_ALL_TESTS': False}) @ddt.ddt class ScoreChangedUpdatesSubsectionGradeTest(ModuleStoreTestCase): """ diff --git a/lms/envs/test.py b/lms/envs/test.py index 4983e6787e..0afd3d6780 100644 --- a/lms/envs/test.py +++ b/lms/envs/test.py @@ -298,6 +298,9 @@ OIDC_COURSE_HANDLER_CACHE_TIMEOUT = 0 FEATURES['ENABLE_MOBILE_REST_API'] = True FEATURES['ENABLE_VIDEO_ABSTRACTION_LAYER_API'] = True +########################### Grades ################################# +FEATURES['PERSISTENT_GRADES_ENABLED_FOR_ALL_TESTS'] = True + ###################### Payment ##############################3 # Enable fake payment processing page FEATURES['ENABLE_PAYMENT_FAKE'] = True From d244715e87c313bd61276243d22ec7dcc542613f Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Sat, 10 Sep 2016 11:27:47 -0400 Subject: [PATCH 2/4] Don't sort blocks to retain order. --- lms/djangoapps/grades/models.py | 43 ++++++++++--------- lms/djangoapps/grades/tests/test_models.py | 50 +++++++++++++--------- 2 files changed, 53 insertions(+), 40 deletions(-) diff --git a/lms/djangoapps/grades/models.py b/lms/djangoapps/grades/models.py index 837445a91c..e82568ff11 100644 --- a/lms/djangoapps/grades/models.py +++ b/lms/djangoapps/grades/models.py @@ -29,13 +29,16 @@ log = logging.getLogger(__name__) BlockRecord = namedtuple('BlockRecord', ['locator', 'weight', 'max_score']) -class BlockRecordSet(frozenset): +class BlockRecordList(tuple): """ - An immutable ordered collection of BlockRecord objects. + An immutable ordered list of BlockRecord objects. """ - def __init__(self, *args, **kwargs): - super(BlockRecordSet, self).__init__(*args, **kwargs) + def __new__(cls, blocks): + return super(BlockRecordList, cls).__new__(cls, tuple(blocks)) + + def __init__(self, blocks): + super(BlockRecordList, self).__init__(blocks) self._json = None self._hash = None @@ -56,8 +59,7 @@ class BlockRecordSet(frozenset): stable ordering. """ if self._json is None: - sorted_blocks = sorted(self, key=attrgetter('locator')) - list_of_block_dicts = [block._asdict() for block in sorted_blocks] + list_of_block_dicts = [block._asdict() for block in self] course_key_string = self._get_course_key_string() # all blocks are from the same course for block_dict in list_of_block_dicts: @@ -77,7 +79,7 @@ class BlockRecordSet(frozenset): @classmethod def from_json(cls, blockrecord_json): """ - Return a BlockRecordSet from a json list. + Return a BlockRecordList from a previously serialized json. """ data = json.loads(blockrecord_json) course_key = data['course_key'] @@ -97,6 +99,13 @@ class BlockRecordSet(frozenset): ) return cls(record_generator) + @classmethod + def from_list(cls, blocks): + """ + Return a BlockRecordList from a list. + """ + return cls(tuple(blocks)) + def to_hash(self): """ Return a hashed version of the list of block records. @@ -120,24 +129,18 @@ class VisibleBlocksQuerySet(models.QuerySet): """ Creates a new VisibleBlocks model object. - Argument 'blocks' should be a BlockRecordSet. + Argument 'blocks' should be a BlockRecordList. """ - - if not isinstance(blocks, BlockRecordSet): - blocks = BlockRecordSet(blocks) - model, _ = self.get_or_create(hashed=blocks.to_hash(), defaults={'blocks_json': blocks.to_json()}) return model def hash_from_blockrecords(self, blocks): """ - Return the hash for a given BlockRecordSet, serializing the records if + Return the hash for a given list of blocks, saving the records if possible, but returning the hash even if an IntegrityError occurs. + + Argument 'blocks' should be a BlockRecordList. """ - - if not isinstance(blocks, BlockRecordSet): - blocks = BlockRecordSet(blocks) - try: with transaction.atomic(): model = self.create_from_blockrecords(blocks) @@ -176,7 +179,7 @@ class VisibleBlocks(models.Model): Returns the blocks_json data stored on this model as a list of BlockRecords in the order they were provided. """ - return BlockRecordSet.from_json(self.blocks_json) + return BlockRecordList.from_json(self.blocks_json) class PersistentSubsectionGradeQuerySet(models.QuerySet): @@ -204,7 +207,7 @@ class PersistentSubsectionGradeQuerySet(models.QuerySet): if not kwargs.get('course_id', None): kwargs['course_id'] = kwargs['usage_key'].course_key - visible_blocks_hash = VisibleBlocks.objects.hash_from_blockrecords(blocks=visible_blocks) + visible_blocks_hash = VisibleBlocks.objects.hash_from_blockrecords(BlockRecordList.from_list(visible_blocks)) return super(PersistentSubsectionGradeQuerySet, self).create( visible_blocks_id=visible_blocks_hash, **kwargs @@ -358,7 +361,7 @@ class PersistentSubsectionGrade(TimeStampedModel): Modify an existing PersistentSubsectionGrade object, saving the new version. """ - visible_blocks_hash = VisibleBlocks.objects.hash_from_blockrecords(blocks=visible_blocks) + visible_blocks_hash = VisibleBlocks.objects.hash_from_blockrecords(BlockRecordList.from_list(visible_blocks)) self.course_version = course_version or "" self.subtree_edited_timestamp = subtree_edited_timestamp diff --git a/lms/djangoapps/grades/tests/test_models.py b/lms/djangoapps/grades/tests/test_models.py index d1d9b5e683..0428dd4bf8 100644 --- a/lms/djangoapps/grades/tests/test_models.py +++ b/lms/djangoapps/grades/tests/test_models.py @@ -14,27 +14,27 @@ from opaque_keys.edx.locator import CourseLocator, BlockUsageLocator from lms.djangoapps.grades.models import ( BlockRecord, - BlockRecordSet, + BlockRecordList, PersistentSubsectionGrade, VisibleBlocks ) -class BlockRecordSetTestCase(TestCase): +class BlockRecordListTestCase(TestCase): """ - Verify the behavior of BlockRecordSets, particularly around edge cases + Verify the behavior of BlockRecordList, particularly around edge cases """ empty_json = '{"blocks":[],"course_key":null}' def test_empty_block_record_set(self): - brs = BlockRecordSet() + brs = BlockRecordList(()) self.assertFalse(brs) self.assertEqual( brs.to_json(), self.empty_json ) self.assertEqual( - BlockRecordSet.from_json(self.empty_json), + BlockRecordList.from_json(self.empty_json), brs ) @@ -108,11 +108,17 @@ class VisibleBlocksTest(GradesModelTestCase): """ Test the VisibleBlocks model. """ + def _create_block_record_list(self, blocks): + """ + Creates and returns a BlockRecordList for the given blocks. + """ + return VisibleBlocks.objects.create_from_blockrecords(BlockRecordList.from_list(blocks)) + def test_creation(self): """ Happy path test to ensure basic create functionality works as expected. """ - vblocks = VisibleBlocks.objects.create_from_blockrecords([self.record_a]) + vblocks = self._create_block_record_list([self.record_a]) list_of_block_dicts = [self.record_a._asdict()] for block_dict in list_of_block_dicts: block_dict['locator'] = unicode(block_dict['locator']) # BlockUsageLocator is not json-serializable @@ -128,30 +134,34 @@ class VisibleBlocksTest(GradesModelTestCase): self.assertEqual(expected_json, vblocks.blocks_json) self.assertEqual(expected_hash, vblocks.hashed) - def test_ordering_does_not_matter(self): + def test_ordering_matters(self): """ - When creating new vblocks, a different ordering of blocks produces the - same record in the database. + When creating new vblocks, different ordering of blocks produces + different records in the database. """ - stored_vblocks = VisibleBlocks.objects.create_from_blockrecords([self.record_a, self.record_b]) - repeat_vblocks = VisibleBlocks.objects.create_from_blockrecords([self.record_b, self.record_a]) - new_vblocks = VisibleBlocks.objects.create_from_blockrecords([self.record_b]) + stored_vblocks = self._create_block_record_list([self.record_a, self.record_b]) + repeat_vblocks = self._create_block_record_list([self.record_b, self.record_a]) + same_order_vblocks = self._create_block_record_list([self.record_a, self.record_b]) + new_vblocks = self._create_block_record_list([self.record_b]) - self.assertEqual(stored_vblocks.pk, repeat_vblocks.pk) - self.assertEqual(stored_vblocks.hashed, repeat_vblocks.hashed) + self.assertNotEqual(stored_vblocks.pk, repeat_vblocks.pk) + self.assertNotEqual(stored_vblocks.hashed, repeat_vblocks.hashed) + + self.assertEquals(stored_vblocks.pk, same_order_vblocks.pk) + self.assertEquals(stored_vblocks.hashed, same_order_vblocks.hashed) self.assertNotEqual(stored_vblocks.pk, new_vblocks.pk) self.assertNotEqual(stored_vblocks.hashed, new_vblocks.hashed) def test_blocks_property(self): """ - Ensures that, given an array of BlockRecord, creating visible_blocks and accessing - visible_blocks.blocks yields a copy of the initial array. Also, trying to set the blocks property should raise - an exception. + Ensures that, given an array of BlockRecord, creating visible_blocks + and accessing visible_blocks.blocks yields a copy of the initial array. + Also, trying to set the blocks property should raise an exception. """ - expected_blocks = [self.record_a, self.record_b] - visible_blocks = VisibleBlocks.objects.create_from_blockrecords(expected_blocks) - self.assertEqual(BlockRecordSet(expected_blocks), visible_blocks.blocks) + expected_blocks = (self.record_a, self.record_b) + visible_blocks = self._create_block_record_list(expected_blocks) + self.assertEqual(expected_blocks, visible_blocks.blocks) with self.assertRaises(AttributeError): visible_blocks.blocks = expected_blocks From c1d4d730e0a2298ad62aa4dce390efe65955dd80 Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Sat, 10 Sep 2016 13:28:19 -0400 Subject: [PATCH 3/4] Fix submissions API test - now that calculated grades are saved --- .../tests/test_submitting_problems.py | 42 ++++++++++++++----- lms/djangoapps/grades/signals/handlers.py | 4 -- 2 files changed, 31 insertions(+), 15 deletions(-) diff --git a/lms/djangoapps/courseware/tests/test_submitting_problems.py b/lms/djangoapps/courseware/tests/test_submitting_problems.py index 78296d0312..9262e1b6d4 100644 --- a/lms/djangoapps/courseware/tests/test_submitting_problems.py +++ b/lms/djangoapps/courseware/tests/test_submitting_problems.py @@ -25,6 +25,7 @@ from courseware.models import StudentModule, BaseStudentModuleHistory from courseware.tests.helpers import LoginEnrollmentTestCase from lms.djangoapps.lms_xblock.runtime import quote_slashes from student.models import anonymous_id_for_user, CourseEnrollment +from submissions import api as submissions_api from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from xmodule.partitions.partitions import Group, UserPartition @@ -143,9 +144,22 @@ class TestSubmittingProblems(ModuleStoreTestCase, LoginEnrollmentTestCase, Probl self.student_user = User.objects.get(email=self.student) self.factory = RequestFactory() # Disable the score change signal to prevent other components from being pulled into tests. - signal_patch = patch('courseware.module_render.SCORE_CHANGED.send') - signal_patch.start() - self.addCleanup(signal_patch.stop) + self.score_changed_signal_patch = patch('courseware.module_render.SCORE_CHANGED.send') + self.score_changed_signal_patch.start() + + def tearDown(self): + super(TestSubmittingProblems, self).tearDown() + self._stop_signal_patch() + + def _stop_signal_patch(self): + """ + Stops the signal patch for the SCORE_CHANGED event. + In case a test wants to test with the event actually + firing. + """ + if self.score_changed_signal_patch: + self.score_changed_signal_patch.stop() + self.score_changed_signal_patch = None def add_dropdown_to_section(self, section_location, name, num_inputs=2): """ @@ -540,14 +554,20 @@ class TestCourseGrader(TestSubmittingProblems): self.check_grade_percent(0.67) self.assertEqual(self.get_grade_summary()['grade'], 'B') - # But now we mock out a get_scores call, and watch as it overrides the - # score read from StudentModule and our student gets an A instead. - with patch('submissions.api.get_scores') as mock_get_scores: - mock_get_scores.return_value = { - self.problem_location('p3').to_deprecated_string(): (1, 1) - } - self.check_grade_percent(1.0) - self.assertEqual(self.get_grade_summary()['grade'], 'A') + # But now, set the score with the submissions API and watch + # as it overrides the score read from StudentModule and our + # student gets an A instead. + self._stop_signal_patch() + student_item = { + 'student_id': anonymous_id_for_user(self.student_user, self.course.id), + 'course_id': unicode(self.course.id), + 'item_id': unicode(self.problem_location('p3')), + 'item_type': 'problem' + } + submission = submissions_api.create_submission(student_item, 'any answer') + submissions_api.set_score(submission['uuid'], 1, 1) + self.check_grade_percent(1.0) + self.assertEqual(self.get_grade_summary()['grade'], 'A') def test_submissions_api_anonymous_student_id(self): """ diff --git a/lms/djangoapps/grades/signals/handlers.py b/lms/djangoapps/grades/signals/handlers.py index 64073e88ff..905d62810b 100644 --- a/lms/djangoapps/grades/signals/handlers.py +++ b/lms/djangoapps/grades/signals/handlers.py @@ -43,8 +43,6 @@ def submissions_score_set_handler(sender, **kwargs): # pylint: disable=unused-a usage_id = kwargs['item_id'] user = user_by_anonymous_id(kwargs['anonymous_user_id']) - # If any of the kwargs were missing, at least one of the following values - # will be None. SCORE_CHANGED.send( sender=None, points_possible=points_possible, @@ -73,8 +71,6 @@ def submissions_score_reset_handler(sender, **kwargs): # pylint: disable=unused usage_id = kwargs['item_id'] user = user_by_anonymous_id(kwargs['anonymous_user_id']) - # If any of the kwargs were missing, at least one of the following values - # will be None. SCORE_CHANGED.send( sender=None, points_possible=0, From 805bf28748ecae4222682cda645d205362fa5c43 Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Sun, 11 Sep 2016 00:44:48 -0400 Subject: [PATCH 4/4] Disable persistent-grades for tests failing query counts, until TNL-5458 --- lms/djangoapps/ccx/tests/test_field_override_performance.py | 1 + lms/djangoapps/courseware/tests/test_views.py | 2 ++ lms/djangoapps/instructor_task/tests/test_tasks_helper.py | 2 ++ 3 files changed, 5 insertions(+) diff --git a/lms/djangoapps/ccx/tests/test_field_override_performance.py b/lms/djangoapps/ccx/tests/test_field_override_performance.py index faf2bfa01d..0a16cf0d59 100644 --- a/lms/djangoapps/ccx/tests/test_field_override_performance.py +++ b/lms/djangoapps/ccx/tests/test_field_override_performance.py @@ -36,6 +36,7 @@ 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 diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index e5088f7441..02fcd58ab0 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -1345,6 +1345,8 @@ class ProgressPageTests(ModuleStoreTestCase): ) 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(((39, 4, True), (39, 4, False)), (True, False)) ) diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index d9907ae012..b6d6fad3c3 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -1641,6 +1641,8 @@ 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.