diff --git a/CHANGELOG.md b/CHANGELOG.md index 581bff0..b7af21f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,11 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Added - Django 6.0 has been added to the test matrix. +- In tests, an error will now be raised when opening a transaction if there are pre-existing unhandled after-commit callbacks. + The pre-existing callbacks would previously run when `transaction` exits. + This helps catch order-of-execution bugs in tests. + The error can be silenced by setting the `SUBATOMIC_CATCH_UNHANDLED_AFTER_COMMIT_CALLBACKS_IN_TESTS` setting to `False` + to facilitate gradual adoption of this stricter rule. ### Fixed diff --git a/docs/reference/settings.md b/docs/reference/settings.md index 29b4d23..1ba5241 100644 --- a/docs/reference/settings.md +++ b/docs/reference/settings.md @@ -30,5 +30,25 @@ progressively enable after-commit callbacks in tests by using [`override_settings`][override_settings] on a per-test basis. +## `SUBATOMIC_CATCH_UNHANDLED_AFTER_COMMIT_CALLBACKS_IN_TESTS` + +(default: `True`) + +[`transaction`][django_subatomic.db.transaction] +will raise `_UnhandledCallbacks` in tests +if it detects any lingering unhandled after-commit callbacks +when it's called. +Note: because this exception represents a programming error, +it starts with an underscore to discourage anyone from catching it. + +This highlights order-of-execution issues in tests +caused by after-commit callbacks having not been run. +This can only happen in tests without after-commit callback simulation +(such as those using Django's `atomic` directly), +because in live systems after commit callbacks are always handled or discarded. + +The error can be silenced by setting this to `False`, +in which case, the lingering callbacks will be run by the transaction after it commits. + [override_settings]: https://docs.djangoproject.com/en/stable/topics/testing/tools/#django.test.override_settings [django-settings]: https://docs.djangoproject.com/en/stable/topics/settings/ diff --git a/src/django_subatomic/db.py b/src/django_subatomic/db.py index 25a3a77..fe3abf8 100644 --- a/src/django_subatomic/db.py +++ b/src/django_subatomic/db.py @@ -194,11 +194,25 @@ def _execute_on_commit_callbacks_in_tests(using: str | None = None) -> Generator - Django 4.2's `run_and_clear_commit_hooks` function: https://github.com/django/django/blob/stable/4.2.x/django/db/backends/base/base.py#L762-L779 """ + only_in_testcase_transaction = _innermost_atomic_block_wraps_testcase(using=using) + + if ( + getattr( + settings, "SUBATOMIC_CATCH_UNHANDLED_AFTER_COMMIT_CALLBACKS_IN_TESTS", True + ) + and only_in_testcase_transaction + ): + connection = django_transaction.get_connection(using) + callbacks = connection.run_on_commit + if callbacks: + raise _UnhandledCallbacks(tuple(callback for _, callback, _ in callbacks)) + yield + if ( # See Note [Running after-commit callbacks in tests] getattr(settings, "SUBATOMIC_RUN_AFTER_COMMIT_CALLBACKS_IN_TESTS", True) - and _innermost_atomic_block_wraps_testcase(using=using) + and only_in_testcase_transaction ): connection = django_transaction.get_connection(using) callbacks = connection.run_on_commit @@ -284,6 +298,20 @@ class _UnexpectedDanglingTransaction(Exception): open_dbs: frozenset[str] +@attrs.frozen +class _UnhandledCallbacks(Exception): + """ + Raised in tests when unhandled callbacks are found before opening a transaction. + + This happens when after-commit callbacks are registered + but not run before trying to open a database transaction. + + The best solution is to ensure the after-commit callbacks are run. + """ + + callbacks: tuple[Callable[[], object], ...] + + # Note [After-commit callbacks require a transaction] # ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ # After-commit callbacks may only be registered when a transaction is open. diff --git a/tests/test_db.py b/tests/test_db.py index 0ff83a5..3047111 100644 --- a/tests/test_db.py +++ b/tests/test_db.py @@ -194,6 +194,102 @@ def raises() -> None: assert error_raised is True assert counter.count == 1 + @pytest.mark.parametrize( + "transaction_manager", + (db.transaction, db.transaction_if_not_already), + ) + def test_unhandled_callbacks_cause_error( + self, transaction_manager: DBContextManager + ) -> None: + """ + If callbacks from a previous atomic context remain, raise an error. + """ + counter = Counter() + + # Django's `atomic` leaves unhandled after-commit actions on exit. + with django_transaction.atomic(): + db.run_after_commit(counter.increment) + + # `transaction` will raise when it finds the unhandled callback. + with pytest.raises(db._UnhandledCallbacks) as exc_info: # noqa: SLF001 + with transaction_manager(): + ... + + assert counter.count == 0 + assert exc_info.value.callbacks == (counter.increment,) + + @pytest.mark.parametrize( + "transaction_manager", + (db.transaction, db.transaction_if_not_already), + ) + def test_unhandled_callbacks_check_can_be_disabled( + self, transaction_manager: DBContextManager + ) -> None: + """ + We can disable the check for unhandled callbacks. + """ + counter = Counter() + + # Django's `atomic` leaves unhandled after-commit actions on exit. + with django_transaction.atomic(): + db.run_after_commit(counter.increment) + + # Run after-commit callbacks when `transaction` exits, + # even if that means running them later than is realistic. + with override_settings( + SUBATOMIC_CATCH_UNHANDLED_AFTER_COMMIT_CALLBACKS_IN_TESTS=False + ): + with transaction_manager(): + assert counter.count == 0 + + assert counter.count == 1 + + @pytest.mark.parametrize( + "transaction_manager", + (db.transaction, db.transaction_if_not_already), + ) + def test_handled_callbacks_are_not_an_error( + self, transaction_manager: DBContextManager + ) -> None: + """ + Already-handled checks do not cause an error. + """ + counter = Counter() + + # Callbacks are handled by `transaction` and removed from the queue. + with db.transaction(): + db.run_after_commit(counter.increment) + assert counter.count == 0 + assert counter.count == 1 + + # The callbacks have been handled, so a second `transaction` does not raise. + with transaction_manager(): + pass + + # The callback was not run a second time. + assert counter.count == 1 + + @pytest.mark.parametrize( + "transaction_manager", + (db.transaction, db.transaction_if_not_already), + ) + def test_callbacks_ignored_by_transaction_if_not_already( + self, transaction_manager: DBContextManager + ) -> None: + """ + `transaction_if_not_already` ignores after-commit callbacks if a transaction already exists. + """ + counter = Counter() + + with transaction_manager(): + db.run_after_commit(counter.increment) + with db.transaction_if_not_already(): + assert counter.count == 0 + assert counter.count == 0 + + # The callback is run when the outermost transaction exits. + assert counter.count == 1 + class TestTransactionRequired: @_parametrize_transaction_testcase