bpo-35330: Don't call the wrapped object if side_effect is set#10973
Merged
Conversation
side_effect is setside_effect is set
cjw296
approved these changes
Dec 7, 2018
cjw296
left a comment
Contributor
There was a problem hiding this comment.
A couple of minor niggles which would be good to tweak, but this looks like a very nice piece of work!
Contributor
There was a problem hiding this comment.
Why is this a mock? Is it important that Real has an attribute or could this just be:
class Real(object): pass
?
Contributor
Author
There was a problem hiding this comment.
That was indeed a bad copy-paste. 😞 .
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
Contributor
Author
|
Thanks @cjw296, I've updated it with the suggestion. Well spotted! 👍 |
Contributor
|
Thanks @mariocj89 for the PR, and @cjw296 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6. |
Contributor
|
Thanks @mariocj89 for the PR, and @cjw296 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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