side_effect is set
#10973
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
side_effect is setside_effect is set
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
A couple of minor niggles which would be good to tweak, but this looks like a very nice piece of work!
| @@ -558,6 +558,16 @@ def test_wraps_calls(self): | |||
| real.assert_called_with(1, 2, fish=3) | |||
|
|
|||
|
|
|||
| def test_wraps_prevents_automatic_creation_of_mocks(self): | |||
| class Real(object): | |||
| attribute = Mock() | |||
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Why is this a mock? Is it important that Real has an attribute or could this just be:
class Real(object): pass
?
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
That was indeed a bad copy-paste. 😞 .
|
|
||
|
|
||
| def test_customize_wrapped_object_with_return_value_and_side_effect(self): | ||
| # side_effect should always take preference over return_value. |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
preference -> precedence
Add more tests to validate how `wraps` interacts with other features of mocks.
When a object is wrapped using `Mock(wraps=...)`, if an user sets a `side_effect` in one of their methods, return the value of `side_effect` and don't call the original object.
When a `Mock` is called, it should return looking up in the following order: `side_effect`, `return_value`, `wraps`. If any of the first two return `mock.DEFAULT`, lookup in the next option. It makes no sense to check for `wraps` returning default, as it is supposed to be the original implementation and there is nothing to fallback to.
1be1416 to
08d1291
Compare
|
Thanks @cjw296, I've updated it with the suggestion. Well spotted! 👍 |
|
Thanks @mariocj89 for the PR, and @cjw296 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6. |
|
Thanks @mariocj89 for the PR, and @cjw296 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7. |
When a
Mockinstance was used to wrap an object, ifside_effectis used in one of the mocks of it methods, don't call the original implementation and return the result of using the side effect the same waythat it is done with return_value.
This PR also includes a refactor via commit 1a28aab, as after the change the code became even harder to read. The refactor (as it now uses common code), also fixes the test case
test_customize_wrapped_object_with_side_effect_iterable_with_default, as when adding the tests I realized that if a mock has a side effect that is an iterable and one of the values returnsDEFAULT, it will default toreturn_valueif present but not to the value inwraps, which is surprising given that if instead of an iterable it is a callable the behaviour is as expected to callwraps.Let me know if you want me to take this to a different PR and issue, as it might be worth properly explaining it. I can also get the fix without the refactor, but it'd get quite messy, basically copying the last lines into the iterator part of
side_effect.https://bugs.python.org/issue35330