Skip to content
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

Support both side_effect and return_value in a more human way #66731

Closed
dimaqq mannequin opened this issue Oct 2, 2014 · 5 comments
Closed

Support both side_effect and return_value in a more human way #66731

dimaqq mannequin opened this issue Oct 2, 2014 · 5 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@dimaqq
Copy link
Mannequin

dimaqq mannequin commented Oct 2, 2014

BPO 22541
Nosy @bitdancer, @voidspace, @dimaqq

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2014-10-03.03:48:28.316>
created_at = <Date 2014-10-02.13:29:56.054>
labels = ['type-feature', 'library']
title = 'Support both side_effect and return_value in a more human way'
updated_at = <Date 2014-10-03.03:48:28.315>
user = 'https://github.com/dimaqq'

bugs.python.org fields:

activity = <Date 2014-10-03.03:48:28.315>
actor = 'michael.foord'
assignee = 'none'
closed = True
closed_date = <Date 2014-10-03.03:48:28.316>
closer = 'michael.foord'
components = ['Library (Lib)']
creation = <Date 2014-10-02.13:29:56.054>
creator = 'Dima.Tisnek'
dependencies = []
files = []
hgrepos = []
issue_num = 22541
keywords = []
message_count = 5.0
messages = ['228224', '228225', '228226', '228227', '228303']
nosy_count = 3.0
nosy_names = ['r.david.murray', 'michael.foord', 'Dima.Tisnek']
pr_nums = []
priority = 'normal'
resolution = 'wont fix'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue22541'
versions = ['Python 3.5']

@dimaqq
Copy link
Mannequin Author

dimaqq mannequin commented Oct 2, 2014

Original use case:

I want to mock something that on invocation does 2 things:

  • updates database
  • returns custom, unrelated value

The problem:
side_effect is a catch-all setting that is used for:

  • true side-effects
  • return values
  • exceptions

Moreover, side_effect takes precedence over return_value, unless the earlier returns mock.DEFAULT. Very quirky.

Proposed change option 1:
Side-effect is not used as return value; explicit return value may be used for that purpose.
(probably changes a lot of tests)

Proposed change option 2:
return_value, being more specific and explicit, takes precedence over side_effect; that is side_effect is still executed but it's rv is lost if overwritten by return_value is set.
(this is unlikely to change existing tests, as it is currently very odd to use both side_effect and return_value at the same time.)

Current workaround 1:
mock.Mock(side_effect=lambda *args: [db.update(), mock.DEFAULT][-1],
return_value=42)

Current workaround 2:
mock.Mock(side_effect=lambda *args: [db.update(), 42][-1])

@dimaqq dimaqq mannequin added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Oct 2, 2014
@bitdancer
Copy link
Member

Can you explain why you can't use side_effect to set the return value?

@dimaqq
Copy link
Mannequin Author

dimaqq mannequin commented Oct 2, 2014

I can, as shown in workaround #2.

However it's hackey when true side-effect's own return value has nothing in common with return value for the mock.

The shortest workaround I managed is this:

mock.Mock(side_effect=lambda *arg: db.update(...) or my_return_value) # relies on db.update returning None

or more explicit like this:

mock.Mock(side_effect=lamda *arg: db.update(...) or mock.DEFAULT, return_value=my_return_value) # -"-

@bitdancer
Copy link
Member

Oh, sorry, I missed the workarounds when I read the issue. So your issue is you want to use a lambda rather than a full function for the side effect and it looks ugly.

I agree that given the names 'side_effect' and 'return_value' you'd think return_value would take precedence. On the other hand, the API is what it is, and I'm not sure we can change it for backward compatibility reasons. Option 1 is definitely not on the table, and the problem with option 2 is that it is easy to imagine a side_effect that conditionally returns mock.DEFAULT.

@voidspace
Copy link
Contributor

One of the use cases for side_effect is for dynamically changing the returned value based on input - so your option 1 just doesn't work.

Using two line functions would make your code easier to read, and then your "workarounds" would look natural instead of awkward.

Maybe return_value should take precedence over side_effect if both are set.

It would probably be more pythonic to raise an exception in that case - although that would be annoying if you're just reconfiguring a mock. I've also written side_effect functions that accessed the return_value, which is probably the genesis of the current API.

As you point out, both your suggested changes are backwards incompatible, so I'm closing this issue.

@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

2 participants