diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstream_sync_integration.py b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstream_sync_integration.py index 78730fe6a0dc..d408319d4375 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstream_sync_integration.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstream_sync_integration.py @@ -13,12 +13,12 @@ from openedx.core.djangoapps.content_libraries.tests import ContentLibrariesRestApiTest from cms.djangoapps.contentstore.xblock_storage_handlers.xblock_helpers import get_block_key_string from xmodule.modulestore.django import modulestore -from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, ImmediateOnCommitMixin from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory @ddt.ddt -class CourseToLibraryTestCase(ContentLibrariesRestApiTest, ModuleStoreTestCase): +class CourseToLibraryTestCase(ContentLibrariesRestApiTest, ImmediateOnCommitMixin, ModuleStoreTestCase): """ Tests that involve syncing content from libraries to courses. """ diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py index cf3838ac3719..1dcc3ca9031a 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py @@ -23,7 +23,7 @@ from common.djangoapps.student.tests.factories import UserFactory from openedx.core.djangoapps.content_libraries import api as lib_api from xmodule.modulestore.django import modulestore -from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase +from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase, ImmediateOnCommitMixin from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory from .. import downstreams as downstreams_views @@ -406,7 +406,7 @@ def test_400(self, sync: str): assert video_after.upstream is None -class DeleteDownstreamViewTest(SharedErrorTestCases, SharedModuleStoreTestCase): +class DeleteDownstreamViewTest(SharedErrorTestCases, ImmediateOnCommitMixin, SharedModuleStoreTestCase): """ Test that `DELETE /api/v2/contentstore/downstreams/...` severs a downstream's link to an upstream. """ @@ -596,6 +596,7 @@ def test_204(self, mock_decline_sync): @ddt.ddt class GetUpstreamViewTest( _BaseDownstreamViewTestMixin, + ImmediateOnCommitMixin, SharedModuleStoreTestCase, ): """ @@ -1424,6 +1425,7 @@ def test_200_get_ready_to_sync_duplicated_top_level_parents(self): class GetDownstreamSummaryViewTest( _BaseDownstreamViewTestMixin, + ImmediateOnCommitMixin, SharedModuleStoreTestCase, ): """ diff --git a/cms/djangoapps/contentstore/tests/test_upstream_downstream_links.py b/cms/djangoapps/contentstore/tests/test_upstream_downstream_links.py index 90fec8471651..5c3ba8386480 100644 --- a/cms/djangoapps/contentstore/tests/test_upstream_downstream_links.py +++ b/cms/djangoapps/contentstore/tests/test_upstream_downstream_links.py @@ -14,7 +14,7 @@ from common.djangoapps.student.tests.factories import UserFactory from openedx.core.djangolib.testing.utils import skip_unless_cms -from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, ImmediateOnCommitMixin from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory from ..models import ContainerLink, LearningContextLinksStatus, LearningContextLinksStatusChoices, ComponentLink @@ -265,7 +265,12 @@ def test_call_for_nonexistent_course(self): @skip_unless_cms -class TestUpstreamLinksEvents(ModuleStoreTestCase, OpenEdxEventsTestMixin, BaseUpstreamLinksHelpers): +class TestUpstreamLinksEvents( + ImmediateOnCommitMixin, + ModuleStoreTestCase, + OpenEdxEventsTestMixin, + BaseUpstreamLinksHelpers, +): """ Test signals related to managing upstream->downstream links. """ diff --git a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py index 3fae9d996fd2..2ca03ccf892b 100644 --- a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py +++ b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py @@ -16,7 +16,7 @@ from openedx_tagging.core.tagging.models import Tag from organizations.models import Organization from xmodule.modulestore.django import contentstore, modulestore -from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, upload_file_to_course +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, upload_file_to_course, ImmediateOnCommitMixin from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory, ToyCourseFactory, LibraryFactory from cms.djangoapps.contentstore.utils import reverse_usage_url @@ -400,7 +400,7 @@ def test_paste_with_assets(self): assert source_pic2_hash != dest_pic2_hash # Because there was a conflict, this file was unchanged. -class ClipboardPasteFromV2LibraryTestCase(OpenEdxEventsTestMixin, ModuleStoreTestCase): +class ClipboardPasteFromV2LibraryTestCase(OpenEdxEventsTestMixin, ImmediateOnCommitMixin, ModuleStoreTestCase): """ Test Clipboard Paste functionality with a "new" (as of Sumac) library """ diff --git a/openedx/core/djangoapps/content/course_overviews/tests/test_signals.py b/openedx/core/djangoapps/content/course_overviews/tests/test_signals.py index 49adf5540003..960be6c6eadb 100644 --- a/openedx/core/djangoapps/content/course_overviews/tests/test_signals.py +++ b/openedx/core/djangoapps/content/course_overviews/tests/test_signals.py @@ -13,7 +13,11 @@ from xmodule.data import CertificatesDisplayBehaviors from xmodule.modulestore import ModuleStoreEnum -from xmodule.modulestore.tests.django_utils import TEST_DATA_ONLY_SPLIT_MODULESTORE_DRAFT_PREFERRED, ModuleStoreTestCase +from xmodule.modulestore.tests.django_utils import ( + TEST_DATA_ONLY_SPLIT_MODULESTORE_DRAFT_PREFERRED, + ModuleStoreTestCase, + ImmediateOnCommitMixin, +) from xmodule.modulestore.tests.factories import CourseFactory, check_mongo_calls from ..models import CourseOverview @@ -23,7 +27,7 @@ @ddt.ddt -class CourseOverviewSignalsTestCase(ModuleStoreTestCase): +class CourseOverviewSignalsTestCase(ImmediateOnCommitMixin, ModuleStoreTestCase): """ Tests for CourseOverview signals. """ @@ -43,16 +47,14 @@ def assert_changed_signal_sent(self, changes, mock_signal): ) # changing display name doesn't fire the signal - with self.captureOnCommitCallbacks(execute=True) as callbacks: - course.display_name = course.display_name + 'changed' - course = self.store.update_item(course, ModuleStoreEnum.UserID.test) + course.display_name = course.display_name + 'changed' + course = self.store.update_item(course, ModuleStoreEnum.UserID.test) assert not mock_signal.called # changing the given field fires the signal - with self.captureOnCommitCallbacks(execute=True) as callbacks: - for change in changes: - setattr(course, change.field_name, change.changed_value) - self.store.update_item(course, ModuleStoreEnum.UserID.test) + for change in changes: + setattr(course, change.field_name, change.changed_value) + self.store.update_item(course, ModuleStoreEnum.UserID.test) assert mock_signal.called def test_caching(self): diff --git a/openedx/core/djangoapps/content/search/tests/test_handlers.py b/openedx/core/djangoapps/content/search/tests/test_handlers.py index 33d0e4db8378..7ccbec687e98 100644 --- a/openedx/core/djangoapps/content/search/tests/test_handlers.py +++ b/openedx/core/djangoapps/content/search/tests/test_handlers.py @@ -11,7 +11,11 @@ from common.djangoapps.student.tests.factories import UserFactory from openedx.core.djangoapps.content_libraries import api as library_api from openedx.core.djangolib.testing.utils import skip_unless_cms -from xmodule.modulestore.tests.django_utils import TEST_DATA_SPLIT_MODULESTORE, ModuleStoreTestCase +from xmodule.modulestore.tests.django_utils import ( + TEST_DATA_SPLIT_MODULESTORE, + ModuleStoreTestCase, + ImmediateOnCommitMixin, +) try: @@ -26,7 +30,7 @@ @patch("openedx.core.djangoapps.content.search.api.MeilisearchClient") @override_settings(MEILISEARCH_ENABLED=True) @skip_unless_cms -class TestUpdateIndexHandlers(ModuleStoreTestCase, LiveServerTestCase): +class TestUpdateIndexHandlers(ImmediateOnCommitMixin, ModuleStoreTestCase, LiveServerTestCase): """ Test that the search index is updated when XBlocks and Library Blocks are modified """ @@ -80,7 +84,9 @@ def test_create_delete_xblock(self, meilisearch_client): "access_id": course_access.id, "modified": created_date.timestamp(), } + meilisearch_client.return_value.index.return_value.update_documents.assert_called_with([doc_sequential]) + with freeze_time(created_date): vertical = self.store.create_child(self.user_id, sequential.location, "vertical", "test_vertical") doc_vertical = { @@ -119,6 +125,7 @@ def test_create_delete_xblock(self, meilisearch_client): doc_sequential["display_name"] = "Updated Sequential" doc_vertical["breadcrumbs"][1]["display_name"] = "Updated Sequential" doc_sequential["modified"] = modified_date.timestamp() + meilisearch_client.return_value.index.return_value.update_documents.assert_called_with([ doc_sequential, doc_vertical, diff --git a/openedx/core/djangoapps/content_tagging/tests/test_tasks.py b/openedx/core/djangoapps/content_tagging/tests/test_tasks.py index d0e10ecfb7ae..997fffbaad8e 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_tasks.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_tasks.py @@ -13,7 +13,11 @@ from common.djangoapps.student.tests.factories import UserFactory from openedx.core.djangolib.testing.utils import skip_unless_cms -from xmodule.modulestore.tests.django_utils import TEST_DATA_SPLIT_MODULESTORE, ModuleStoreTestCase +from xmodule.modulestore.tests.django_utils import ( + TEST_DATA_SPLIT_MODULESTORE, + ModuleStoreTestCase, + ImmediateOnCommitMixin, +) from openedx.core.djangoapps.content_libraries.api import ( create_library, create_library_block, delete_library_block, restore_library_block ) @@ -59,6 +63,7 @@ def setUp(self): @override_waffle_flag(CONTENT_TAGGING_AUTO, active=True) class TestAutoTagging( # type: ignore[misc] LanguageTaxonomyTestMixin, + ImmediateOnCommitMixin, ModuleStoreTestCase, LiveServerTestCase ): diff --git a/xmodule/modulestore/__init__.py b/xmodule/modulestore/__init__.py index f3aee2a58f24..51118cc42e67 100644 --- a/xmodule/modulestore/__init__.py +++ b/xmodule/modulestore/__init__.py @@ -12,6 +12,7 @@ from collections import defaultdict from contextlib import contextmanager from operator import itemgetter +from django.db import transaction from opaque_keys.edx.keys import AssetKey, CourseKey from opaque_keys.edx.locations import Location # For import backwards compatibility @@ -322,6 +323,10 @@ def on_commit_changes_to(self, course_key, fn): """ Call some callback when the currently active bulk operation has saved """ + # If we're in a MySQL transaction, so the new version will only be committed to the + # SplitModulestoreCourseIndex table after the MySQL transaction is closed. + def wrapped_fn(): + transaction.on_commit(fn) # Check if a bulk op is active. If so, defer fn(); otherwise call it immediately. # Note: calling _get_bulk_ops_record() here and then checking .active can have side-effects in some cases # because it creates an entry in the defaultdict if none exists, so we check if the record is active using @@ -329,9 +334,9 @@ def on_commit_changes_to(self, course_key, fn): # so we check it this way: if course_key and course_key.for_branch(None) in self._active_bulk_ops.records: bulk_ops_record = self._active_bulk_ops.records[course_key.for_branch(None)] - bulk_ops_record.defer_until_commit(fn) + bulk_ops_record.defer_until_commit(wrapped_fn) else: - fn() # There is no active bulk operation - call fn() now. + wrapped_fn() # There is no active bulk operation - call wrapped_fn() now. def _is_in_bulk_operation(self, course_key, ignore_case=False): """ diff --git a/xmodule/modulestore/tests/django_utils.py b/xmodule/modulestore/tests/django_utils.py index 952a88800f7c..f20aaa3a56d3 100644 --- a/xmodule/modulestore/tests/django_utils.py +++ b/xmodule/modulestore/tests/django_utils.py @@ -613,6 +613,35 @@ def update_course(self, course, user_id): return updated_course +class ImmediateOnCommitMixin: + """ + Mixin for tests that want `on_commit` callbacks to run immediately, + even under TestCase (which normally wraps tests in a transaction + that never commits). + Especially useful when the test needs to execute an event that occurs after an `on_commit` + """ + + @classmethod + def setUpClass(cls): + super_cls = super() + if hasattr(super_cls, 'setUpClass'): + super_cls.setUpClass() + # Patch `transaction.on_commit` so that callbacks run immediately + cls._on_commit_patcher = patch( + 'django.db.transaction.on_commit', + side_effect=lambda func, **kwargs: func() + ) + cls._on_commit_patcher.start() + + @classmethod + def tearDownClass(cls): + # Stop patching, restore original behavior + cls._on_commit_patcher.stop() + super_cls = super() + if hasattr(super_cls, 'tearDownClass'): + super_cls.tearDownClass() + + def upload_file_to_course(course_key, contentstore, source_file, target_filename): ''' Uploads the given source file to the given course, and returns the content of the file. diff --git a/xmodule/tests/__init__.py b/xmodule/tests/__init__.py index 786836c050b5..d8a203f34ba8 100644 --- a/xmodule/tests/__init__.py +++ b/xmodule/tests/__init__.py @@ -13,7 +13,7 @@ from functools import wraps from unittest.mock import Mock -from django.test import TestCase +from django.test import TransactionTestCase from opaque_keys.edx.keys import CourseKey from path import Path as path @@ -306,7 +306,7 @@ def __getitem__(self, index): return str(self)[index] -class CourseComparisonTest(TestCase): +class CourseComparisonTest(TransactionTestCase): """ Mixin that has methods for comparing courses for equality. """