From 8dc7f34295f399bb1ac8b72c0bf1b072d2e8dd57 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 11 Aug 2014 13:43:22 -0400 Subject: [PATCH 1/6] Improve debuggability when call-count numbers don't match up --- .../contentstore/tests/test_contentstore.py | 4 +- .../contentstore/tests/test_course_listing.py | 5 +- .../xmodule/modulestore/tests/factories.py | 116 +++++++++--------- .../tests/test_mixed_modulestore.py | 110 +++++++++-------- .../xmodule/modulestore/tests/test_publish.py | 29 +++-- .../xmodule/modulestore/xml_importer.py | 2 +- .../courseware/tests/test_module_render.py | 4 +- 7 files changed, 148 insertions(+), 122 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 946500e299..ec46484922 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -867,7 +867,7 @@ class ContentStoreToyCourseTest(ContentStoreTestCase): # so we don't need to make an extra query to compute it. # set the branch to 'publish' in order to prevent extra lookups of draft versions with mongo_store.branch_setting(ModuleStoreEnum.Branch.published_only): - with check_mongo_calls(mongo_store, 4, 0): + with check_mongo_calls(4, 0): course = mongo_store.get_course(course_id, depth=2) # make sure we pre-fetched a known sequential which should be at depth=2 @@ -879,7 +879,7 @@ class ContentStoreToyCourseTest(ContentStoreTestCase): # Now, test with the branch set to draft. No extra round trips b/c it doesn't go deep enough to get # beyond direct only categories with mongo_store.branch_setting(ModuleStoreEnum.Branch.draft_preferred): - with check_mongo_calls(mongo_store, 4, 0): + with check_mongo_calls(4, 0): mongo_store.get_course(course_id, depth=2) def test_export_course_without_content_store(self): diff --git a/cms/djangoapps/contentstore/tests/test_course_listing.py b/cms/djangoapps/contentstore/tests/test_course_listing.py index 3bca7d16a2..7622e2d54b 100644 --- a/cms/djangoapps/contentstore/tests/test_course_listing.py +++ b/cms/djangoapps/contentstore/tests/test_course_listing.py @@ -201,10 +201,11 @@ class TestCourseListing(ModuleStoreTestCase): # Now count the db queries store = modulestore()._get_modulestore_by_type(ModuleStoreEnum.Type.mongo) - with check_mongo_calls(store, USER_COURSES_COUNT): + with check_mongo_calls(USER_COURSES_COUNT): _accessible_courses_list_from_groups(self.request) - with check_mongo_calls(store, 1): + # TODO: LMS-11220: Document why this takes 6 calls + with check_mongo_calls(6): _accessible_courses_list(self.request) def test_get_course_list_with_same_course_id(self): diff --git a/common/lib/xmodule/xmodule/modulestore/tests/factories.py b/common/lib/xmodule/xmodule/modulestore/tests/factories.py index 381b0a08e8..e59b30aec8 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/factories.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/factories.py @@ -1,3 +1,6 @@ +import pprint +import pymongo.message + from factory import Factory, lazy_attribute_sequence, lazy_attribute from factory.containers import CyclicDefinitionError from uuid import uuid4 @@ -223,79 +226,74 @@ def check_exact_number_of_calls(object_with_method, method, num_calls, method_na yield -@contextmanager -def check_number_of_calls(object_with_method, method, maximum_calls, minimum_calls=1, method_name=None): +def check_number_of_calls(object_with_method, method_name, maximum_calls, minimum_calls=1): """ Instruments the given method on the given object to verify the number of calls to the method is less than or equal to the expected maximum_calls and greater than or equal to the expected minimum_calls. """ - method_wrap = Mock(wraps=method) - wrap_patch = patch.object(object_with_method, method_name or method.__name__, method_wrap) - - try: - wrap_patch.start() - yield - - finally: - wrap_patch.stop() - - # verify the counter actually worked by ensuring we have counted greater than (or equal to) the minimum calls - assert_greater_equal(method_wrap.call_count, minimum_calls) - - # now verify the number of actual calls is less than (or equal to) the expected maximum - assert_less_equal(method_wrap.call_count, maximum_calls) + return check_sum_of_calls(object_with_method, [method_name], maximum_calls, minimum_calls) @contextmanager -def check_mongo_calls(mongo_store, num_finds=0, num_sends=None): +def check_sum_of_calls(object_, methods, maximum_calls, minimum_calls=1): + """ + Instruments the given methods on the given object to verify that the total sum of calls made to the + methods falls between minumum_calls and maximum_calls. + """ + mocks = { + method: Mock(wraps=getattr(object_, method)) + for method in methods + } + + with patch.multiple(object_, **mocks): + yield + + call_count = sum(mock.call_count for mock in mocks.values()) + calls = pprint.pformat({ + method_name: mock.call_args_list + for method_name, mock in mocks.items() + }) + + # Assertion errors don't handle multi-line values, so pretty-print to std-out instead + if not minimum_calls <= call_count <= maximum_calls: + print "Expected between {} and {} calls, {} were made. Calls: {}".format( + minimum_calls, + maximum_calls, + call_count, + calls, + ) + + # verify the counter actually worked by ensuring we have counted greater than (or equal to) the minimum calls + assert_greater_equal(call_count, minimum_calls) + + # now verify the number of actual calls is less than (or equal to) the expected maximum + assert_less_equal(call_count, maximum_calls) + + +@contextmanager +def check_mongo_calls(num_finds=0, num_sends=None): """ Instruments the given store to count the number of calls to find (incl find_one) and the number of calls to send_message which is for insert, update, and remove (if you provide num_sends). At the end of the with statement, it compares the counts to the num_finds and num_sends. - :param mongo_store: the MongoModulestore or subclass to watch or a SplitMongoModuleStore :param num_finds: the exact number of find calls expected :param num_sends: If none, don't instrument the send calls. If non-none, count and compare to the given int value. """ - if mongo_store.get_modulestore_type() == ModuleStoreEnum.Type.mongo: - with check_exact_number_of_calls(mongo_store.collection, mongo_store.collection.find, num_finds): - if num_sends is not None: - with check_exact_number_of_calls( - mongo_store.database.connection, - mongo_store.database.connection._send_message, # pylint: disable=protected-access - num_sends, - ): - yield - else: + with check_sum_of_calls( + pymongo.message, + ['query', 'get_more'], + num_finds, + num_finds + ): + if num_sends is not None: + with check_sum_of_calls( + pymongo.message, + ['insert', 'update', 'delete'], + num_sends, + num_sends + ): yield - elif mongo_store.get_modulestore_type() == ModuleStoreEnum.Type.split: - collections = [ - mongo_store.db_connection.course_index, - mongo_store.db_connection.structures, - mongo_store.db_connection.definitions, - ] - # could add else clause which raises exception or just rely on the below to suss that out - try: - find_wraps = [] - wrap_patches = [] - for collection in collections: - find_wrap = Mock(wraps=collection.find) - find_wraps.append(find_wrap) - wrap_patch = patch.object(collection, 'find', find_wrap) - wrap_patches.append(wrap_patch) - wrap_patch.start() - if num_sends is not None: - connection = mongo_store.db_connection.database.connection - with check_exact_number_of_calls( - connection, - connection._send_message, # pylint: disable=protected-access - num_sends, - ): - yield - else: - yield - finally: - map(lambda wrap_patch: wrap_patch.stop(), wrap_patches) - call_count = sum([find_wrap.call_count for find_wrap in find_wraps]) - assert_equal(call_count, num_finds) + else: + yield diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py index 62aa123b72..27a16a13f0 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -270,8 +270,13 @@ class TestMixedModuleStore(unittest.TestCase): with self.assertRaises(DuplicateCourseError): self.store.create_course('org_x', 'course_y', 'run_z', self.user_id) + # Draft: + # - One lookup to locate an item that exists + # - Two lookups to determine an item doesn't exist (one to check mongo, one to check split) # split has one lookup for the course and then one for the course items - @ddt.data(('draft', 1, 0), ('split', 2, 0)) + # TODO: LMS-11220: Document why draft find count is [1, 1] + # TODO: LMS-11220: Document why split find count is [2, 2] + @ddt.data(('draft', [1, 1], 0), ('split', [2, 2], 0)) @ddt.unpack def test_has_item(self, default_ms, max_find, max_send): self.initdb(default_ms) @@ -279,15 +284,14 @@ class TestMixedModuleStore(unittest.TestCase): self.assertTrue(self.store.has_item(self.course_locations[self.XML_COURSEID1])) - mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID)) - with check_mongo_calls(mongo_store, max_find, max_send): + with check_mongo_calls(max_find.pop(0), max_send): self.assertTrue(self.store.has_item(self.problem_x1a_1)) # try negative cases self.assertFalse(self.store.has_item( self.course_locations[self.XML_COURSEID1].replace(name='not_findable', category='problem') )) - with check_mongo_calls(mongo_store, max_find, max_send): + with check_mongo_calls(max_find.pop(0), max_send): self.assertFalse(self.store.has_item(self.fake_location)) # verify that an error is raised when the revision is not valid @@ -296,7 +300,9 @@ class TestMixedModuleStore(unittest.TestCase): # draft is 2 to compute inheritance # split is 2 (would be 3 on course b/c it looks up the wiki_slug in definitions) - @ddt.data(('draft', 2, 0), ('split', 2, 0)) + # TODO: LMS-11220: Document why draft find count is [2, 2] + # TODO: LMS-11220: Document why split find count is [3, 3] + @ddt.data(('draft', [2, 2], 0), ('split', [2, 2], 0)) @ddt.unpack def test_get_item(self, default_ms, max_find, max_send): self.initdb(default_ms) @@ -304,8 +310,7 @@ class TestMixedModuleStore(unittest.TestCase): self.assertIsNotNone(self.store.get_item(self.course_locations[self.XML_COURSEID1])) - mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID)) - with check_mongo_calls(mongo_store, max_find, max_send): + with check_mongo_calls(max_find.pop(0), max_send): self.assertIsNotNone(self.store.get_item(self.problem_x1a_1)) # try negative cases @@ -313,7 +318,7 @@ class TestMixedModuleStore(unittest.TestCase): self.store.get_item( self.course_locations[self.XML_COURSEID1].replace(name='not_findable', category='problem') ) - with check_mongo_calls(mongo_store, max_find, max_send): + with check_mongo_calls(max_find.pop(0), max_send): with self.assertRaises(ItemNotFoundError): self.store.get_item(self.fake_location) @@ -322,6 +327,7 @@ class TestMixedModuleStore(unittest.TestCase): self.store.get_item(self.fake_location, revision=ModuleStoreEnum.RevisionOption.draft_preferred) # compared to get_item for the course, draft asks for both draft and published + # TODO: LMS-11220: Document why split find count is 3 @ddt.data(('draft', 8, 0), ('split', 2, 0)) @ddt.unpack def test_get_items(self, default_ms, max_find, max_send): @@ -334,9 +340,8 @@ class TestMixedModuleStore(unittest.TestCase): self.assertEqual(len(modules), 1) self.assertEqual(modules[0].location, course_locn) - mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID)) course_locn = self.course_locations[self.MONGO_COURSEID] - with check_mongo_calls(mongo_store, max_find, max_send): + with check_mongo_calls(max_find, max_send): # NOTE: use get_course if you just want the course. get_items is expensive modules = self.store.get_items(course_locn.course_key, qualifiers={'category': 'problem'}) self.assertEqual(len(modules), 6) @@ -352,7 +357,7 @@ class TestMixedModuleStore(unittest.TestCase): # split: 3 to get the course structure & the course definition (show_calculator is scope content) # before the change. 1 during change to refetch the definition. 3 afterward (b/c it calls get_item to return the "new" object). # 2 sends to update index & structure (calculator is a setting field) - @ddt.data(('draft', 7, 5), ('split', 6, 2)) + @ddt.data(('draft', 11, 5), ('split', 6, 2)) @ddt.unpack def test_update_item(self, default_ms, max_find, max_send): """ @@ -368,12 +373,11 @@ class TestMixedModuleStore(unittest.TestCase): self.store.update_item(course, self.user_id) # now do it for a r/w db - mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID)) problem = self.store.get_item(self.problem_x1a_1) # if following raised, then the test is really a noop, change it self.assertNotEqual(problem.max_attempts, 2, "Default changed making test meaningless") problem.max_attempts = 2 - with check_mongo_calls(mongo_store, max_find, max_send): + with check_mongo_calls(max_find, max_send): problem = self.store.update_item(problem, self.user_id) self.assertEqual(problem.max_attempts, 2, "Update didn't persist") @@ -434,7 +438,7 @@ class TestMixedModuleStore(unittest.TestCase): component = self.store.publish(component.location, self.user_id) self.assertFalse(self.store.has_changes(component)) - @ddt.data(('draft', 7, 2), ('split', 13, 4)) + @ddt.data(('draft', 8, 2), ('split', 13, 4)) @ddt.unpack def test_delete_item(self, default_ms, max_find, max_send): """ @@ -446,14 +450,14 @@ class TestMixedModuleStore(unittest.TestCase): with self.assertRaises(NotImplementedError): self.store.delete_item(self.xml_chapter_location, self.user_id) - mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID)) - with check_mongo_calls(mongo_store, max_find, max_send): + with check_mongo_calls(max_find, max_send): self.store.delete_item(self.writable_chapter_location, self.user_id) + # verify it's gone with self.assertRaises(ItemNotFoundError): self.store.get_item(self.writable_chapter_location) - @ddt.data(('draft', 8, 2), ('split', 13, 4)) + @ddt.data(('draft', 9, 2), ('split', 13, 4)) @ddt.unpack def test_delete_private_vertical(self, default_ms, max_find, max_send): """ @@ -484,8 +488,7 @@ class TestMixedModuleStore(unittest.TestCase): self.assertIn(vert_loc, course.children) # delete the vertical and ensure the course no longer points to it - mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID)) - with check_mongo_calls(mongo_store, max_find, max_send): + with check_mongo_calls(max_find, max_send): self.store.delete_item(vert_loc, self.user_id) course = self.store.get_course(self.course_locations[self.MONGO_COURSEID].course_key, 0) if hasattr(private_vert.location, 'version_guid'): @@ -499,7 +502,7 @@ class TestMixedModuleStore(unittest.TestCase): self.assertFalse(self.store.has_item(leaf_loc)) self.assertNotIn(vert_loc, course.children) - @ddt.data(('draft', 4, 1), ('split', 5, 2)) + @ddt.data(('draft', 5, 1), ('split', 5, 2)) @ddt.unpack def test_delete_draft_vertical(self, default_ms, max_find, max_send): """ @@ -528,24 +531,24 @@ class TestMixedModuleStore(unittest.TestCase): self.store.publish(private_vert.location, self.user_id) private_leaf.display_name = 'change me' private_leaf = self.store.update_item(private_leaf, self.user_id) - mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID)) # test succeeds if delete succeeds w/o error - with check_mongo_calls(mongo_store, max_find, max_send): + with check_mongo_calls(max_find, max_send): self.store.delete_item(private_leaf.location, self.user_id) - @ddt.data(('draft', 2, 0), ('split', 3, 0)) + # TODO: LMS-11220: Document why split find count is 5 + # TODO: LMS-11220: Document why draft find count is 4 + @ddt.data(('draft', 4, 0), ('split', 4, 0)) @ddt.unpack def test_get_courses(self, default_ms, max_find, max_send): self.initdb(default_ms) # we should have 3 total courses across all stores - mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID)) - with check_mongo_calls(mongo_store, max_find, max_send): + with check_mongo_calls(max_find, max_send): courses = self.store.get_courses() - course_ids = [course.location for course in courses] - self.assertEqual(len(courses), 3, "Not 3 courses: {}".format(course_ids)) - self.assertIn(self.course_locations[self.MONGO_COURSEID], course_ids) - self.assertIn(self.course_locations[self.XML_COURSEID1], course_ids) - self.assertIn(self.course_locations[self.XML_COURSEID2], course_ids) + course_ids = [course.location for course in courses] + self.assertEqual(len(courses), 3, "Not 3 courses: {}".format(course_ids)) + self.assertIn(self.course_locations[self.MONGO_COURSEID], course_ids) + self.assertIn(self.course_locations[self.XML_COURSEID1], course_ids) + self.assertIn(self.course_locations[self.XML_COURSEID2], course_ids) with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred): draft_courses = self.store.get_courses(remove_branch=True) @@ -588,8 +591,7 @@ class TestMixedModuleStore(unittest.TestCase): of getting an item whose scope.content fields are looked at. """ self.initdb(default_ms) - mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID)) - with check_mongo_calls(mongo_store, max_find, max_send): + with check_mongo_calls(max_find, max_send): course = self.store.get_item(self.course_locations[self.MONGO_COURSEID]) self.assertEqual(course.id, self.course_locations[self.MONGO_COURSEID].course_key) @@ -598,7 +600,8 @@ class TestMixedModuleStore(unittest.TestCase): # notice this doesn't test getting a public item via draft_preferred which draft would have 2 hits (split # still only 2) - @ddt.data(('draft', 1, 0), ('split', 2, 0)) + # TODO: LMS-11220: Document why draft find count is 2 + @ddt.data(('draft', 2, 0), ('split', 2, 0)) @ddt.unpack def test_get_parent_locations(self, default_ms, max_find, max_send): """ @@ -607,10 +610,9 @@ class TestMixedModuleStore(unittest.TestCase): self.initdb(default_ms) self._create_block_hierarchy() - mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID)) - with check_mongo_calls(mongo_store, max_find, max_send): + with check_mongo_calls(max_find, max_send): parent = self.store.get_parent_location(self.problem_x1a_1) - self.assertEqual(parent, self.vertical_x1a) + self.assertEqual(parent, self.vertical_x1a) parent = self.store.get_parent_location(self.xml_chapter_location) self.assertEqual(parent, self.course_locations[self.XML_COURSEID1]) @@ -692,7 +694,21 @@ class TestMixedModuleStore(unittest.TestCase): (child_to_delete_location, None, ModuleStoreEnum.RevisionOption.published_only), ]) - @ddt.data(('draft', [10, 3], 0), ('split', [14, 6], 0)) + # Mongo reads: + # First location: + # - count problem (1) + # - For each level of ancestors: (5) + # - Count ancestor + # - retrieve ancestor + # - compute inheritable data + # Second location: + # - load vertical + # - load inheritance data + + # TODO: LMS-11220: Document why draft send count is 5 + # TODO: LMS-11220: Document why draft find count is 18 + # TODO: LMS-11220: Document why split find count is 16 + @ddt.data(('draft', [18, 5], 0), ('split', [14, 6], 0)) @ddt.unpack def test_path_to_location(self, default_ms, num_finds, num_sends): """ @@ -713,7 +729,7 @@ class TestMixedModuleStore(unittest.TestCase): mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID)) for location, expected in should_work: - with check_mongo_calls(mongo_store, num_finds.pop(0), num_sends): + with check_mongo_calls(num_finds.pop(0), num_sends): self.assertEqual(path_to_location(self.store, location), expected) not_found = ( @@ -881,8 +897,7 @@ class TestMixedModuleStore(unittest.TestCase): block_id=location.block_id ) - mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID)) - with check_mongo_calls(mongo_store, max_find, max_send): + with check_mongo_calls(max_find, max_send): found_orphans = self.store.get_orphans(self.course_locations[self.MONGO_COURSEID].course_key) self.assertEqual(set(found_orphans), set(orphan_locations)) @@ -924,7 +939,9 @@ class TestMixedModuleStore(unittest.TestCase): self.assertEqual(self.user_id, block.subtree_edited_by) self.assertGreater(datetime.datetime.now(UTC), block.subtree_edited_on) - @ddt.data(('draft', 1, 0), ('split', 1, 0)) + # TODO: LMS-11220: Document why split find count is 2 + # TODO: LMS-11220: Document why draft find count is 2 + @ddt.data(('draft', 2, 0), ('split', 2, 0)) @ddt.unpack def test_get_courses_for_wiki(self, default_ms, max_find, max_send): """ @@ -941,8 +958,7 @@ class TestMixedModuleStore(unittest.TestCase): self.assertIn(self.course_locations[self.XML_COURSEID2].course_key, wiki_courses) # Test Mongo wiki - mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID)) - with check_mongo_calls(mongo_store, max_find, max_send): + with check_mongo_calls(max_find, max_send): wiki_courses = self.store.get_courses_for_wiki('999') self.assertEqual(len(wiki_courses), 1) self.assertIn( @@ -953,7 +969,7 @@ class TestMixedModuleStore(unittest.TestCase): self.assertEqual(len(self.store.get_courses_for_wiki('edX.simple.2012_Fall')), 0) self.assertEqual(len(self.store.get_courses_for_wiki('no_such_wiki')), 0) - @ddt.data(('draft', 2, 6), ('split', 7, 2)) + @ddt.data(('draft', 3, 6), ('split', 7, 2)) @ddt.unpack def test_unpublish(self, default_ms, max_find, max_send): """ @@ -971,8 +987,7 @@ class TestMixedModuleStore(unittest.TestCase): self.assertIsNotNone(published_xblock) # unpublish - mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID)) - with check_mongo_calls(mongo_store, max_find, max_send): + with check_mongo_calls(max_find, max_send): self.store.unpublish(self.vertical_x1a, self.user_id) with self.assertRaises(ItemNotFoundError): @@ -1000,8 +1015,7 @@ class TestMixedModuleStore(unittest.TestCase): # start off as Private item = self.store.create_child(self.user_id, self.writable_chapter_location, 'problem', 'test_compute_publish_state') item_location = item.location - mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID)) - with check_mongo_calls(mongo_store, max_find, max_send): + with check_mongo_calls(max_find, max_send): self.assertEquals(self.store.compute_publish_state(item), PublishState.private) # Private -> Public diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_publish.py b/common/lib/xmodule/xmodule/modulestore/tests/test_publish.py index e7678de926..1bd21fbb7b 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_publish.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_publish.py @@ -19,23 +19,35 @@ class TestPublish(SplitWMongoCourseBoostrapper): # There are 12 created items and 7 parent updates # create course: finds: 1 to verify uniqueness, 1 to find parents # sends: 1 to create course, 1 to create overview - with check_mongo_calls(self.draft_mongo, 5, 2): + with check_mongo_calls(5, 2): super(TestPublish, self)._create_course(split=False) # 2 inserts (course and overview) # with bulk will delay all inheritance computations which won't be added into the mongo_calls with self.draft_mongo.bulk_write_operations(self.old_course_key): # finds: 1 for parent to add child # sends: 1 for insert, 1 for parent (add child) - with check_mongo_calls(self.draft_mongo, 1, 2): + with check_mongo_calls(1, 2): self._create_item('chapter', 'Chapter1', {}, {'display_name': 'Chapter 1'}, 'course', 'runid', split=False) - with check_mongo_calls(self.draft_mongo, 2, 2): + with check_mongo_calls(2, 2): self._create_item('chapter', 'Chapter2', {}, {'display_name': 'Chapter 2'}, 'course', 'runid', split=False) - # update info propagation is 2 levels. create looks for draft and then published and then creates - with check_mongo_calls(self.draft_mongo, 8, 6): + # For each vertical (2) created: + # - load draft + # - load non-draft + # - get last error + # - load parent + # - load inheritable data + with check_mongo_calls(10, 6): self._create_item('vertical', 'Vert1', {}, {'display_name': 'Vertical 1'}, 'chapter', 'Chapter1', split=False) self._create_item('vertical', 'Vert2', {}, {'display_name': 'Vertical 2'}, 'chapter', 'Chapter1', split=False) - with check_mongo_calls(self.draft_mongo, 20, 12): + # For each (4) item created + # - load draft + # - load non-draft + # - get last error + # - load parent + # - load inheritable data + # - load parent + with check_mongo_calls(24, 12): self._create_item('html', 'Html1', "

Goodbye

", {'display_name': 'Parented Html'}, 'vertical', 'Vert1', split=False) self._create_item( 'discussion', 'Discussion1', @@ -63,7 +75,7 @@ class TestPublish(SplitWMongoCourseBoostrapper): split=False ) - with check_mongo_calls(self.draft_mongo, 0, 2): + with check_mongo_calls(0, 2): # 2 finds b/c looking for non-existent parents self._create_item('static_tab', 'staticuno', "

tab

", {'display_name': 'Tab uno'}, None, None, split=False) self._create_item('course_info', 'updates', "

  1. Sep 22

    test

", {}, None, None, split=False) @@ -76,10 +88,11 @@ class TestPublish(SplitWMongoCourseBoostrapper): vert_location = self.old_course_key.make_usage_key('vertical', block_id='Vert1') item = self.draft_mongo.get_item(vert_location, 2) # Vert1 has 3 children; so, publishes 4 nodes which may mean 4 inserts & 1 bulk remove + # TODO: LMS-11220: Document why find count is 25 # 25-June-2014 find calls are 19. Probably due to inheritance recomputation? # 02-July-2014 send calls are 7. 5 from above, plus 2 for updating subtree edit info for Chapter1 and course # find calls are 22. 19 from above, plus 3 for finding the parent of Vert1, Chapter1, and course - with check_mongo_calls(self.draft_mongo, 22, 7): + with check_mongo_calls(25, 7): self.draft_mongo.publish(item.location, self.user_id) # verify status diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index 0de12cae7c..137f7cfa58 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -607,7 +607,7 @@ def _import_course_draft( _import_module(descriptor) except Exception: - logging.exception('There while importing draft descriptor %s', descriptor) + logging.exception('while importing draft descriptor %s', descriptor) def allowed_metadata_by_category(category): diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index 8df1087a9e..bb71361017 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -326,7 +326,7 @@ class TestTOC(ModuleStoreTestCase): self.request = factory.get(chapter_url) self.request.user = UserFactory() self.modulestore = self.store._get_modulestore_for_courseid(self.course_key) - with check_mongo_calls(self.modulestore, num_finds, num_sends): + with check_mongo_calls(num_finds, num_sends): self.toy_course = self.store.get_course(self.toy_loc, depth=2) self.field_data_cache = FieldDataCache.cache_for_descriptor_descendents( self.toy_loc, self.request.user, self.toy_course, depth=2) @@ -352,7 +352,7 @@ class TestTOC(ModuleStoreTestCase): 'format': '', 'due': None, 'active': False}], 'url_name': 'secret:magic', 'display_name': 'secret:magic'}]) - with check_mongo_calls(self.modulestore, 0, 0): + with check_mongo_calls(0, 0): actual = render.toc_for_course( self.request.user, self.request, self.toy_course, self.chapter, None, self.field_data_cache ) From 3afc125ecb3ef1b061f09640de3ad0ac72e336fd Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Thu, 28 Aug 2014 14:31:46 -0400 Subject: [PATCH 2/6] Add test to verify bad query behavior of xblock_handler --- .../contentstore/tests/test_import.py | 6 ++--- cms/djangoapps/contentstore/tests/utils.py | 18 +++++++++----- cms/djangoapps/contentstore/views/item.py | 2 +- .../contentstore/views/tests/test_item.py | 24 +++++++++++++++++-- .../xmodule/modulestore/tests/django_utils.py | 3 +++ .../xmodule/modulestore/tests/factories.py | 4 ++-- 6 files changed, 43 insertions(+), 14 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_import.py b/cms/djangoapps/contentstore/tests/test_import.py index fe6b1bbbb2..c6d0a12f04 100644 --- a/cms/djangoapps/contentstore/tests/test_import.py +++ b/cms/djangoapps/contentstore/tests/test_import.py @@ -157,15 +157,15 @@ class ContentStoreImportTest(ModuleStoreTestCase): store = modulestore()._get_modulestore_by_type(ModuleStoreEnum.Type.mongo) # we try to refresh the inheritance tree for each update_item in the import - with check_exact_number_of_calls(store, store.refresh_cached_metadata_inheritance_tree, 28): + with check_exact_number_of_calls(store, "refresh_cached_metadata_inheritance_tree", 28): # _get_cached_metadata_inheritance_tree should be called only once - with check_exact_number_of_calls(store, store._get_cached_metadata_inheritance_tree, 1): + with check_exact_number_of_calls(store, "_get_cached_metadata_inheritance_tree", 1): # with bulk-edit in progress, the inheritance tree should be recomputed only at the end of the import # NOTE: On Jenkins, with memcache enabled, the number of calls here is only 1. # Locally, without memcache, the number of calls is actually 2 (once more during the publish step) - with check_number_of_calls(store, store._compute_metadata_inheritance_tree, 2): + with check_number_of_calls(store, "_compute_metadata_inheritance_tree", 2): self.load_test_import_course() def test_rewrite_reference_list(self): diff --git a/cms/djangoapps/contentstore/tests/utils.py b/cms/djangoapps/contentstore/tests/utils.py index fca7d8b2a2..359bc6fa96 100644 --- a/cms/djangoapps/contentstore/tests/utils.py +++ b/cms/djangoapps/contentstore/tests/utils.py @@ -98,17 +98,23 @@ class CourseTestCase(ModuleStoreTestCase): nonstaff.is_authenticated = True return client, nonstaff - def populate_course(self): + def populate_course(self, branching=2): """ - Add 2 chapters, 4 sections, 8 verticals, 16 problems to self.course (branching 2) + Add k chapters, k^2 sections, k^3 verticals, k^4 problems to self.course (where k = branching) """ user_id = self.user.id + self.populated_usage_keys = {} + def descend(parent, stack): - xblock_type = stack.pop(0) - for _ in range(2): + if not stack: + return + + xblock_type = stack[0] + for _ in range(branching): child = ItemFactory.create(category=xblock_type, parent_location=parent.location, user_id=user_id) - if stack: - descend(child, stack) + print child.location + self.populated_usage_keys.setdefault(xblock_type, []).append(child.location) + descend(child, stack[1:]) descend(self.course, ['chapter', 'sequential', 'vertical', 'problem']) diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 2959ba1391..381f016de6 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -756,7 +756,7 @@ def _compute_visibility_state(xblock, child_info, is_unit_with_changes): return VisibilityState.needs_attention is_unscheduled = xblock.start == DEFAULT_START_DATE is_live = datetime.now(UTC) > xblock.start - children = child_info and child_info['children'] + children = child_info and child_info.get('children', []) if children and len(children) > 0: all_staff_only = True all_unscheduled = True diff --git a/cms/djangoapps/contentstore/views/tests/test_item.py b/cms/djangoapps/contentstore/views/tests/test_item.py index c613ab59d6..bffbfda7bd 100644 --- a/cms/djangoapps/contentstore/views/tests/test_item.py +++ b/cms/djangoapps/contentstore/views/tests/test_item.py @@ -26,7 +26,7 @@ from student.tests.factories import UserFactory from xmodule.capa_module import CapaDescriptor from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import modulestore -from xmodule.modulestore.tests.factories import ItemFactory +from xmodule.modulestore.tests.factories import ItemFactory, check_mongo_calls from xmodule.x_module import STUDIO_VIEW, STUDENT_VIEW from xblock.exceptions import NoSuchHandlerError from opaque_keys.edx.keys import UsageKey, CourseKey @@ -83,7 +83,8 @@ class ItemTest(CourseTestCase): return self.response_usage_key(resp) -class GetItem(ItemTest): +@ddt.ddt +class GetItemTest(ItemTest): """Tests for '/xblock' GET url.""" def _get_container_preview(self, usage_key): @@ -100,6 +101,25 @@ class GetItem(ItemTest): self.assertIsNotNone(resources) return html, resources + @ddt.data( + (1, 21, 23, 35, 37), + (2, 22, 24, 38, 39), + (3, 23, 25, 41, 41), + ) + @ddt.unpack + def test_get_query_count(self, branching_factor, chapter_queries, section_queries, unit_queries, problem_queries): + self.populate_course(branching_factor) + # Retrieve it + with check_mongo_calls(chapter_queries): + self.client.get(reverse_usage_url('xblock_handler', self.populated_usage_keys['chapter'][-1])) + with check_mongo_calls(section_queries): + self.client.get(reverse_usage_url('xblock_handler', self.populated_usage_keys['sequential'][-1])) + with check_mongo_calls(unit_queries): + self.client.get(reverse_usage_url('xblock_handler', self.populated_usage_keys['vertical'][-1])) + with check_mongo_calls(problem_queries): + self.client.get(reverse_usage_url('xblock_handler', self.populated_usage_keys['problem'][-1])) + + def test_get_vertical(self): # Add a vertical resp = self.create_xblock(category='vertical') diff --git a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py index 15bf4282ff..3f1afda31c 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py @@ -10,6 +10,7 @@ from xmodule.modulestore.django import modulestore, clear_existing_modulestores from xmodule.modulestore import ModuleStoreEnum import datetime import pytz +from request_cache.middleware import RequestCache from xmodule.tabs import CoursewareTab, CourseInfoTab, StaticTab, DiscussionTab, ProgressTab, WikiTab from xmodule.modulestore.tests.sample_courses import default_block_info_tree, TOY_BLOCK_INFO_TREE from xmodule.modulestore.tests.mongo_connection import MONGO_PORT_NUM, MONGO_HOST @@ -275,6 +276,8 @@ class ModuleStoreTestCase(TestCase): # the next time they are accessed. # We do this at *both* setup and teardown just to be safe. clear_existing_modulestores() + # clear RequestCache to emulate its clearance after each http request. + RequestCache().clear_request_cache() # Call superclass implementation super(ModuleStoreTestCase, self)._post_teardown() diff --git a/common/lib/xmodule/xmodule/modulestore/tests/factories.py b/common/lib/xmodule/xmodule/modulestore/tests/factories.py index e59b30aec8..b394df3ac5 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/factories.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/factories.py @@ -217,12 +217,12 @@ class ItemFactory(XModuleFactory): @contextmanager -def check_exact_number_of_calls(object_with_method, method, num_calls, method_name=None): +def check_exact_number_of_calls(object_with_method, method_name, num_calls): """ Instruments the given method on the given object to verify the number of calls to the method is exactly equal to 'num_calls'. """ - with check_number_of_calls(object_with_method, method, num_calls, num_calls, method_name): + with check_number_of_calls(object_with_method, method_name, num_calls, num_calls): yield From 1db107c2ce7283a2370201313c311fff811eebbb Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Thu, 28 Aug 2014 14:40:21 -0400 Subject: [PATCH 3/6] Add memoization for has_changes --- .../contentstore/views/component.py | 1 + cms/djangoapps/contentstore/views/item.py | 2 +- .../contentstore/views/tests/test_item.py | 2 -- .../xmodule/xmodule/modulestore/__init__.py | 31 +++++++++++++++++++ .../xmodule/modulestore/mongo/draft.py | 2 +- 5 files changed, 34 insertions(+), 4 deletions(-) diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py index b050ecb433..b957d35d75 100644 --- a/cms/djangoapps/contentstore/views/component.py +++ b/cms/djangoapps/contentstore/views/component.py @@ -28,6 +28,7 @@ from opaque_keys.edx.keys import UsageKey from .access import has_course_access from django.utils.translation import ugettext as _ +from models.settings.course_grading import CourseGradingModel __all__ = ['OPEN_ENDED_COMPONENT_TYPES', 'ADVANCED_COMPONENT_POLICY_KEY', diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 381f016de6..9a5511cf66 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -571,7 +571,7 @@ def _get_xblock(usage_key, user): """ store = modulestore() try: - return store.get_item(usage_key) + return store.get_item(usage_key, depth=None) except ItemNotFoundError: if usage_key.category in CREATE_IF_NOT_FOUND: # Create a new one for certain categories only. Used for course info handouts. diff --git a/cms/djangoapps/contentstore/views/tests/test_item.py b/cms/djangoapps/contentstore/views/tests/test_item.py index bffbfda7bd..a7fac49877 100644 --- a/cms/djangoapps/contentstore/views/tests/test_item.py +++ b/cms/djangoapps/contentstore/views/tests/test_item.py @@ -1,9 +1,7 @@ """Tests for items views.""" -import os import json from datetime import datetime, timedelta import ddt -from unittest import skipUnless from mock import patch from pytz import UTC diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index df20eafec3..95957b95e5 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/common/lib/xmodule/xmodule/modulestore/__init__.py @@ -24,6 +24,7 @@ from opaque_keys import InvalidKeyError from opaque_keys.edx.locations import SlashSeparatedCourseKey from xblock.runtime import Mixologist from xblock.core import XBlock +import functools log = logging.getLogger('edx.modulestore') @@ -559,6 +560,36 @@ class ModuleStoreReadBase(ModuleStoreRead): raise ValueError(u"Cannot set default store to type {}".format(store_type)) yield + @staticmethod + def memoize_request_cache(func): + """ + Memoize a function call results on the request_cache if there's one. Creates the cache key by + joining the unicode of all the args with &; so, if your arg may use the default &, it may + have false hits + """ + @functools.wraps(func) + def wrapper(self, *args, **kwargs): + if self.request_cache: + cache_key = '&'.join([hashvalue(arg) for arg in args]) + if cache_key in self.request_cache.data.setdefault(func.__name__, {}): + return self.request_cache.data[func.__name__][cache_key] + + result = func(self, *args, **kwargs) + + self.request_cache.data[func.__name__][cache_key] = result + return result + else: + return func(self, *args, **kwargs) + return wrapper + +def hashvalue(arg): + """ + If arg is an xblock, use its location. otherwise just turn it into a string + """ + if isinstance(arg, XBlock): + return unicode(arg.location) + else: + return unicode(arg) class ModuleStoreWriteBase(ModuleStoreReadBase, ModuleStoreWrite): ''' diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py index 6aaa0e10f1..db84c48646 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py @@ -589,13 +589,13 @@ class DraftModuleStore(MongoModuleStore): _internal([root_usage.to_deprecated_son() for root_usage in root_usages]) self.collection.remove({'_id': {'$in': to_be_deleted}}, safe=self.collection.safe) + @MongoModuleStore.memoize_request_cache def has_changes(self, xblock): """ Check if the xblock or its children have been changed since the last publish. :param xblock: xblock to check :return: True if the draft and published versions differ """ - # don't check children if this block has changes (is not public) if self.compute_publish_state(xblock) != PublishState.public: return True From c14891e920f467a5500bd01b32bbd97a3d18ed50 Mon Sep 17 00:00:00 2001 From: Andy Armstrong Date: Thu, 28 Aug 2014 20:03:31 -0400 Subject: [PATCH 4/6] Pre-cache has_changes when computing ancestors --- cms/djangoapps/contentstore/views/item.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 9a5511cf66..5393f6bf68 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -596,6 +596,9 @@ def _get_module_info(xblock, rewrite_static_links=True): course_id=xblock.location.course_key ) + # Pre-cache has changes for the entire course because we'll need it for the ancestor info + modulestore().has_changes(modulestore().get_course(xblock.location.course_key, depth=None)) + # Note that children aren't being returned until we have a use case. return create_xblock_info(xblock, data=data, metadata=own_metadata(xblock), include_ancestor_info=True) From 4fcc59b3182cef48fb87da4be42a35d987e55650 Mon Sep 17 00:00:00 2001 From: Andy Armstrong Date: Fri, 29 Aug 2014 16:50:40 -0400 Subject: [PATCH 5/6] Merge branch 'release' into andya/merge-hotfix-2014-08-29 Conflicts: cms/djangoapps/contentstore/tests/test_import.py common/lib/xmodule/xmodule/modulestore/__init__.py common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py lms/djangoapps/courseware/tests/test_module_render.py --- lms/djangoapps/courseware/tests/test_module_render.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index 3dd5c87a7b..2f8ecab123 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -326,13 +326,8 @@ class TestTOC(ModuleStoreTestCase): self.request = factory.get(chapter_url) self.request.user = UserFactory() self.modulestore = self.store._get_modulestore_for_courseid(self.course_key) -<<<<<<< HEAD - self.toy_course = self.store.get_course(self.toy_loc, depth=2) - with check_mongo_calls(num_finds, num_sends): -======= with check_mongo_calls(num_finds, num_sends): self.toy_course = self.store.get_course(self.toy_loc, depth=2) ->>>>>>> release self.field_data_cache = FieldDataCache.cache_for_descriptor_descendents( self.toy_loc, self.request.user, self.toy_course, depth=2 ) From fb5658809095e26a68fb674c0259ba01ee5d9fd6 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 2 Sep 2014 10:20:47 -0400 Subject: [PATCH 6/6] Tweak mongo count numbers --- .../tests/test_mixed_modulestore.py | 28 +++++++++++-------- .../courseware/tests/test_module_render.py | 10 ++++--- 2 files changed, 23 insertions(+), 15 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py index 4cecbc8df3..4686ec3890 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -302,7 +302,7 @@ class TestMixedModuleStore(unittest.TestCase): # split is 2 (would be 3 on course b/c it looks up the wiki_slug in definitions) # TODO: LMS-11220: Document why draft find count is [2, 2] # TODO: LMS-11220: Document why split find count is [3, 3] - @ddt.data(('draft', [2, 2], 0), ('split', [2, 2], 0)) + @ddt.data(('draft', [2, 2], 0), ('split', [3, 3], 0)) @ddt.unpack def test_get_item(self, default_ms, max_find, max_send): self.initdb(default_ms) @@ -328,7 +328,7 @@ class TestMixedModuleStore(unittest.TestCase): # compared to get_item for the course, draft asks for both draft and published # TODO: LMS-11220: Document why split find count is 3 - @ddt.data(('draft', 8, 0), ('split', 2, 0)) + @ddt.data(('draft', 8, 0), ('split', 3, 0)) @ddt.unpack def test_get_items(self, default_ms, max_find, max_send): self.initdb(default_ms) @@ -357,7 +357,7 @@ class TestMixedModuleStore(unittest.TestCase): # split: 3 to get the course structure & the course definition (show_calculator is scope content) # before the change. 1 during change to refetch the definition. 3 afterward (b/c it calls get_item to return the "new" object). # 2 sends to update index & structure (calculator is a setting field) - @ddt.data(('draft', 11, 5), ('split', 6, 2)) + @ddt.data(('draft', 11, 5), ('split', 3, 2)) @ddt.unpack def test_update_item(self, default_ms, max_find, max_send): """ @@ -438,7 +438,9 @@ class TestMixedModuleStore(unittest.TestCase): component = self.store.publish(component.location, self.user_id) self.assertFalse(self.store.has_changes(component)) - @ddt.data(('draft', 8, 2), ('split', 13, 4)) + # TODO: LMS-11220: Document why split find count is 4 + # TODO: LMS-11220: Document why split send count is 3 + @ddt.data(('draft', 8, 2), ('split', 4, 3)) @ddt.unpack def test_delete_item(self, default_ms, max_find, max_send): """ @@ -457,7 +459,9 @@ class TestMixedModuleStore(unittest.TestCase): with self.assertRaises(ItemNotFoundError): self.store.get_item(self.writable_chapter_location) - @ddt.data(('draft', 9, 2), ('split', 13, 4)) + # TODO: LMS-11220: Document why split find count is 4 + # TODO: LMS-11220: Document why split send count is 3 + @ddt.data(('draft', 9, 2), ('split', 4, 3)) @ddt.unpack def test_delete_private_vertical(self, default_ms, max_find, max_send): """ @@ -502,7 +506,8 @@ class TestMixedModuleStore(unittest.TestCase): self.assertFalse(self.store.has_item(leaf_loc)) self.assertNotIn(vert_loc, course.children) - @ddt.data(('draft', 5, 1), ('split', 5, 2)) + # TODO: LMS-11220: Document why split find count is 2 + @ddt.data(('draft', 5, 1), ('split', 2, 2)) @ddt.unpack def test_delete_draft_vertical(self, default_ms, max_find, max_send): """ @@ -537,7 +542,7 @@ class TestMixedModuleStore(unittest.TestCase): # TODO: LMS-11220: Document why split find count is 5 # TODO: LMS-11220: Document why draft find count is 4 - @ddt.data(('draft', 4, 0), ('split', 4, 0)) + @ddt.data(('draft', 4, 0), ('split', 5, 0)) @ddt.unpack def test_get_courses(self, default_ms, max_find, max_send): self.initdb(default_ms) @@ -707,9 +712,9 @@ class TestMixedModuleStore(unittest.TestCase): # - load inheritance data # TODO: LMS-11220: Document why draft send count is 5 - # TODO: LMS-11220: Document why draft find count is 18 - # TODO: LMS-11220: Document why split find count is 16 - @ddt.data(('draft', [18, 5], 0), ('split', [14, 6], 0)) + # TODO: LMS-11220: Document why draft find count is [19, 6] + # TODO: LMS-11220: Document why split find count is [2, 2] + @ddt.data(('draft', [19, 6], 0), ('split', [2, 2], 0)) @ddt.unpack def test_path_to_location(self, default_ms, num_finds, num_sends): """ @@ -1039,7 +1044,8 @@ class TestMixedModuleStore(unittest.TestCase): # Sends: # - insert structure # - write index entry - @ddt.data(('draft', 3, 6), ('split', 7, 2)) + # TODO: LMS-11220: Document why split find count is 3 + @ddt.data(('draft', 3, 6), ('split', 3, 2)) @ddt.unpack def test_unpublish(self, default_ms, max_find, max_send): """ diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index 2f8ecab123..aa223d724f 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -333,8 +333,9 @@ class TestTOC(ModuleStoreTestCase): ) - # TODO: LMS-11220: Document why split find count is 21 - @ddt.data((ModuleStoreEnum.Type.mongo, 1, 0), (ModuleStoreEnum.Type.split, 5, 0)) + # TODO: LMS-11220: Document why split find count is 9 + # TODO: LMS-11220: Document why mongo find count is 4 + @ddt.data((ModuleStoreEnum.Type.mongo, 4, 0), (ModuleStoreEnum.Type.split, 9, 0)) @ddt.unpack def test_toc_toy_from_chapter(self, default_ms, num_finds, num_sends): with self.store.default_store(default_ms): @@ -361,8 +362,9 @@ class TestTOC(ModuleStoreTestCase): for toc_section in expected: self.assertIn(toc_section, actual) - # TODO: LMS-11220: Document why split find count is 21 - @ddt.data((ModuleStoreEnum.Type.mongo, 1, 0), (ModuleStoreEnum.Type.split, 5, 0)) + # TODO: LMS-11220: Document why split find count is 9 + # TODO: LMS-11220: Document why mongo find count is 4 + @ddt.data((ModuleStoreEnum.Type.mongo, 4, 0), (ModuleStoreEnum.Type.split, 9, 0)) @ddt.unpack def test_toc_toy_from_section(self, default_ms, num_finds, num_sends): with self.store.default_store(default_ms):