-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add optional side_effect_meta parameter #31
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,6 @@ | ||
from __future__ import annotations | ||
|
||
from typing import Any | ||
from unittest import mock | ||
|
||
from django.test import TestCase | ||
|
@@ -146,44 +149,50 @@ def test__run_func__no_return_value(self): | |
def test_func(): | ||
pass | ||
|
||
registry._run_func(test_func, return_value=None) | ||
meta = registry.SideEffectMeta(label="foo", return_value=None) | ||
registry._run_func(test_func, meta=meta) | ||
|
||
def test__run_func__with_return_value(self): | ||
"""Test the _run_func function passes through the return_value if required.""" | ||
|
||
def test_func(**kwargs): | ||
assert "return_value" in kwargs | ||
|
||
# return_value not passed through, so fails | ||
registry._run_func(test_func) | ||
# self.assertRaises(KeyError, registry._run_func, test_func) | ||
registry._run_func(test_func, return_value=None) | ||
meta = registry.SideEffectMeta(label="foo", return_value=None) | ||
registry._run_func(test_func, meta=meta) | ||
|
||
def test__run_func__aborts_on_error(self): | ||
"""Test the _run_func function handles ABORT_ON_ERROR correctly.""" | ||
|
||
def test_func(): | ||
raise Exception("Pah") | ||
|
||
meta = registry.SideEffectMeta(label="foo", return_value=None) | ||
|
||
# error is logged, but not raised | ||
with mock.patch.object(settings, "ABORT_ON_ERROR", False): | ||
self.assertFalse(settings.ABORT_ON_ERROR) | ||
registry._run_func(test_func, return_value=None) | ||
registry._run_func(test_func, meta=meta) | ||
|
||
# error is raised | ||
with mock.patch.object(settings, "ABORT_ON_ERROR", True): | ||
self.assertTrue(settings.ABORT_ON_ERROR) | ||
self.assertRaises(Exception, registry._run_func, test_func) | ||
self.assertRaises(Exception, registry._run_func, test_func, meta=meta) | ||
|
||
def test__run_func__signature_mismatch(self): | ||
"""Test the _run_func function always raises SignatureMismatch.""" | ||
|
||
def test_func(): | ||
raise Exception("Pah") | ||
|
||
meta = registry.SideEffectMeta(label="foo", return_value=None) | ||
with mock.patch.object(settings, "ABORT_ON_ERROR", False): | ||
self.assertRaises( | ||
registry.SignatureMismatch, registry._run_func, test_func, 1 | ||
registry.SignatureMismatch, | ||
registry._run_func, | ||
test_func, | ||
1, | ||
meta=meta, | ||
) | ||
|
||
|
||
|
@@ -223,27 +232,63 @@ def test_func(): | |
self.assertEqual(r.by_label_contains("foo"), {"foo": [test_func]}) | ||
self.assertEqual(r.by_label_contains("food"), {}) | ||
|
||
@mock.patch("side_effects.registry._run_func") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test has been removed (not rewritten). It had hidden failures, and was incorrect: (The |
||
def test__run_side_effects__no_return_value(self, mock_run): | ||
"""Test return_value is not passed""" | ||
def test__run_side_effects__with_side_effect_meta(self) -> None: | ||
"""Test the meta object is passed if the function requires it explictly.""" | ||
actual_call_a = mock.Mock() | ||
actual_call_b = mock.Mock() | ||
actual_call_c = mock.Mock() | ||
r = registry.Registry() | ||
|
||
def handler_a(*, side_effect_meta: registry.SideEffectMeta) -> None: | ||
actual_call_a(side_effect_meta=side_effect_meta) | ||
|
||
def handler_b(*args: Any, side_effect_meta: registry.SideEffectMeta) -> None: | ||
actual_call_b(*args, side_effect_meta=side_effect_meta) | ||
|
||
def handler_c( | ||
*, side_effect_meta: registry.SideEffectMeta, **kwargs: Any | ||
) -> None: | ||
actual_call_c(side_effect_meta=side_effect_meta, **kwargs) | ||
|
||
r.add("a", handler_a) | ||
r.add("b", handler_b) | ||
r.add("c", handler_c) | ||
|
||
def no_return_value(*args, **kwargz): | ||
assert "return_value" not in kwargz | ||
meta_a = registry.SideEffectMeta(label="a", return_value=None) | ||
r._run_side_effects(meta=meta_a) | ||
actual_call_a.assert_called_once_with(side_effect_meta=meta_a) | ||
|
||
meta_b = registry.SideEffectMeta(label="b", return_value=None) | ||
r._run_side_effects(1, 2, 3, meta=meta_b) | ||
actual_call_b.assert_called_once_with(1, 2, 3, side_effect_meta=meta_b) | ||
|
||
meta_c = registry.SideEffectMeta(label="c", return_value=None) | ||
r._run_side_effects(meta=meta_c, x=1, y=2) | ||
actual_call_c.assert_called_once_with(side_effect_meta=meta_c, x=1, y=2) | ||
|
||
def test__run_side_effects__side_effect_meta_must_be_keyword_only(self) -> None: | ||
"""Test that the meta object is not passed if not a keyword-only param.""" | ||
meta = registry.SideEffectMeta(label="foo", return_value=None) | ||
r = registry.Registry() | ||
r.add("foo", no_return_value) | ||
r._run_side_effects("foo") | ||
r._run_side_effects("foo", return_value=None) | ||
|
||
def handler(side_effect_meta: registry.SideEffectMeta, *args, **kwargs: Any) -> None: | ||
pass | ||
|
||
r.add(meta.label, handler) | ||
self.assertRaises(registry.SignatureMismatch, r._run_side_effects, meta=meta) | ||
|
||
def test__run_side_effects__with_return_value(self): | ||
"""Test return_value is passed""" | ||
"""Test return_value is passed if the function has **kwargs.""" | ||
actual_call = mock.Mock() | ||
r = registry.Registry() | ||
|
||
def has_return_value(*args, **kwargs): | ||
assert "return_value" in kwargs | ||
actual_call(*args, **kwargs) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using a mock here to monitor the parameters means that the assertion can't be accidentally swallowed by the |
||
|
||
r.add("foo", has_return_value) | ||
r._run_side_effects("foo", return_value=None) | ||
meta = registry.SideEffectMeta(label="foo", return_value=None) | ||
r._run_side_effects(meta=meta) | ||
actual_call.assert_called_once_with(return_value=None) | ||
|
||
def test_try_bind_all(self): | ||
def foo1(return_value): | ||
|
@@ -267,5 +312,8 @@ def foo5(arg1, **kwargs): | |
r.add("foo", foo3) | ||
r.add("foo", foo4) | ||
r.add("foo", foo5) | ||
r.try_bind_all("foo", 1) | ||
self.assertRaises(registry.SignatureMismatch, r.try_bind_all, "foo", 1, 2) | ||
meta = registry.SideEffectMeta(label="foo", return_value=None) | ||
r.try_bind_all(1, meta=meta) | ||
self.assertRaises( | ||
registry.SignatureMismatch, r.try_bind_all, 1, 2, meta=meta | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're temporarily adding complexity here, but if the
return_value
were to be removed in favour ofside_effect_meta
, we could simplify this signature checking logic considerably.The
return_value
bind check could be removed immediately, but also theside_effect_meta
check is simpler. Crucially,return_value
requires us to check whether the signature can be bound, because it is injected into**kwargs
when not explicitly present as a parameter, whereas the need forside_effect_meta
can be reduced to checking for a particular parameter name & type in the signature.This potentially means that signatures could be checked
inside the decorators_
has_side_effectsand
is_side_effect_of(once
run_side_effects` was removed) - i.e. on initialisation, rather than at runtime, every time a side effect is triggered.