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 78730fe6a0..d408319d43 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 freezegun import freeze_time 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 cf3838ac37..1dcc3ca903 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.roles import CourseStaffRole 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 @@ class PutDownstreamViewTest(SharedErrorTestCases, SharedModuleStoreTestCase): 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 @@ class DeleteDownstreamSyncViewtest( @ddt.ddt class GetUpstreamViewTest( _BaseDownstreamViewTestMixin, + ImmediateOnCommitMixin, SharedModuleStoreTestCase, ): """ @@ -1424,6 +1425,7 @@ class GetUpstreamViewTest( 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 90fec84716..5c3ba83864 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 openedx_events.tests.utils import OpenEdxEventsTestMixin 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 @@ class TestRecreateUpstreamLinks(ModuleStoreTestCase, OpenEdxEventsTestMixin, Bas @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 3fae9d996f..2ca03ccf89 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_events.tests.utils import OpenEdxEventsTestMixin 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 @@ class ClipboardPasteTestCase(ModuleStoreTestCase): 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 49adf55400..960be6c6ea 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 pytz import UTC 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 @@ Change = namedtuple("Change", ["field_name", "initial_value", "changed_value"]) @ddt.ddt -class CourseOverviewSignalsTestCase(ModuleStoreTestCase): +class CourseOverviewSignalsTestCase(ImmediateOnCommitMixin, ModuleStoreTestCase): """ Tests for CourseOverview signals. """ @@ -43,16 +47,14 @@ class CourseOverviewSignalsTestCase(ModuleStoreTestCase): ) # 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 33d0e4db83..7ccbec687e 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 organizations.tests.factories import OrganizationFactory 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 @@ except RuntimeError: @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 @@ class TestUpdateIndexHandlers(ModuleStoreTestCase, LiveServerTestCase): "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 @@ class TestUpdateIndexHandlers(ModuleStoreTestCase, LiveServerTestCase): 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 d0e10ecfb7..997fffbaad 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 organizations.models import Organization 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 @@ class LanguageTaxonomyTestMixin: @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 f3aee2a58f..51118cc42e 100644 --- a/xmodule/modulestore/__init__.py +++ b/xmodule/modulestore/__init__.py @@ -12,6 +12,7 @@ from abc import ABCMeta, abstractmethod 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 @@ class BulkOperationsMixin: """ 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 @@ class BulkOperationsMixin: # 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 952a88800f..f20aaa3a56 100644 --- a/xmodule/modulestore/tests/django_utils.py +++ b/xmodule/modulestore/tests/django_utils.py @@ -613,6 +613,35 @@ class ModuleStoreTestCase( 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 786836c050..d8a203f34b 100644 --- a/xmodule/tests/__init__.py +++ b/xmodule/tests/__init__.py @@ -13,7 +13,7 @@ from contextlib import contextmanager 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 @@ class LazyFormat: return str(self)[index] -class CourseComparisonTest(TestCase): +class CourseComparisonTest(TransactionTestCase): """ Mixin that has methods for comparing courses for equality. """