Permalink
Browse files

Added the ability to mock properties.

  • Loading branch information...
1 parent 719cf01 commit 2c9a5ea6cc4ae07510b8c394638900013d5ab223 Bryce Covert committed Mar 16, 2012
Showing with 101 additions and 12 deletions.
  1. +69 −12 flexmock.py
  2. +32 −0 tests/flexmock_test.py
View
@@ -38,7 +38,7 @@
AT_LEAST = 'at least'
AT_MOST = 'at most'
EXACTLY = 'exactly'
-UPDATED_ATTRS = ['should_receive', 'should_call', 'new_instances']
+UPDATED_ATTRS = ['should_receive', 'should_receive_property', 'should_call', 'new_instances']
@has207

has207 Mar 17, 2012

I don't believe in adding to the API to support a new type of mockable "thing". This should be handled correctly by both should_receive and should_call when a property is being mocked

@brycecovert

brycecovert Mar 20, 2012

Owner

How do you think a property should be denoted (if at all)?

I could imagine making it fail over to a property if it's invoked that way, but I'm not crazy about that, because you really shouldn't be able to call with_args or anything like that for properties. Also, I'd like introspection on objects to show up correctly, and a getattr solution wouldn't show the property in a dir().

I could also see using some sort of character to show that the received 'thing' is a property. This would be cool, because you can use it for chaining as well. I.e.,
mock.should_receive("@foo").and_return(500)
mock.should_receive("something.@foo.bar").and_return(200)

What do you think?

@has207

has207 Mar 20, 2012

I don't think you need anything special to denote the property, in theory it would just work like any function. with_args should naturally fail, but it can figure out that you're dealing with a property without having the user specify it explicitly.

@brycecovert

brycecovert Mar 20, 2012

Owner

For a partial mock, that makes sense. but how would you expect it to distinguish between these two?
f = flexmock().should_receive("foo").and_return("bar").mock()

which did it set up the expectation for?
f.foo
f.foo()

@has207

has207 Mar 21, 2012

I think using should_receive on a fake object (non-partial mock) is pretty meaningless. flexmock supports it but I don't really see much value in doing something like that, and simply setting it with flexmock(foo=bar) is really the right approach. I'm also not sure that it would actually be meaningful to attach properties to fake objects. You already have full freedom to return whatever you want on them, with or without parens, so in what case to you see that being a useful thing?

@brycecovert

brycecovert Mar 21, 2012

Owner

I see where you're coming from now. There are 2 use cases that I don't think are well supported.

  1. Django model properties sometimes raise exceptions. The most notable case is following a foreign key when the value is null. This is impossible to test presently in flexmock. We would want to test this quite a bit.

  2. I admit that this is more of an edge case, but sometimes the same property is called more than once in a method, and you want to return different values each time. Essentially, properties can only be stubbed, not mocked, right now.

What do you think?

@has207

has207 Mar 22, 2012

Ok, so just to clarify, you're talking about adding a property to a fake object created with flexmock() rather than a partial mock created with flexmock(something), right?

I think that should basically work like this:

foo = flexmock(bar=property(lambda self: 'baz'))

This currently doesn't work as the property is added to the object directly rather than to its class.

The right solution would be to create a new intermediate class that inherits from flexmock.Mock that foo instantiates instead of flexmock.Mock, and detect property values when assigning them to the class rather than the instance. This actually seems pretty straightforward to implement, I'll create a new issue to track it.

@brycecovert

brycecovert via email Mar 22, 2012

Owner
@has207

has207 Mar 23, 2012

Well, if you really must have the setup of the property be on one line you can stick an eval into the lambda ;)

foo = flexmock(bar=property(lambda: eval(compile('raise Exception()', '', 'single'))

[I know, I know...]

I wish Python didn't have such a crippled lambda implementation, especially considering there's always an escape hatch.. But is the two line setup (in those very infrequent cases where you actually need this sort of thing) really that hard?

def raiser(e): raise e
foo = flexmock(bar=property(lamba s: raiser(Exception))

The flexmock().should_receive("@bar").and_raise(Exception) usage just doesn't feel right. It seems to me that if you want to be doing this sort of thing you should partially mock an actual instance rather than making a fake that has built-in expectations. If anything, I'd actually like to deprecate that sort of thing altogether by providing a better alternative than to encourage it further.

And yeah, there's no support for properties on partial mocks yet(!), but I think that would be the right goal to work toward. I'm very reluctant to add anything along the lines of "@bar" as it's a departure for the API in general, and my feeling is that the API needs to be simplified rather than augmented at this stage.

-Herman

P.S. Please don't get discouraged if I'm coming off as too negative about your ideas, I think this is a good discussion to have and explore the options, and has already given me a number of ideas as well as highlighted the need for something that I always considered a nice-to-have in the past but I'm starting to see there are real use-cases and it's worthwhile investing a bit of time to implement. So thank you for following up, please keep it coming! :)

class FlexmockError(Exception):
@@ -140,11 +140,12 @@ class Expectation(object):
raise.
"""
- def __init__(self, mock, name=None, return_value=None, original_method=None):
+ def __init__(self, mock, name=None, return_value=None, original_method=None, is_property=False):
self.method = name
self.modifier = EXACTLY
self.original_method = original_method
self.args = None
+ self.is_property = is_property
value = ReturnValue(return_value)
self.return_values = return_values = []
self._replace_with = None
@@ -177,8 +178,9 @@ def __getattribute__(self, name):
return _getattr(self, 'times')(2)
elif name == 'never':
return _getattr(self, 'times')(0)
- elif name in ('at_least', 'at_most', 'ordered', 'one_by_one'):
+ elif name in ('at_least', 'at_most', 'ordered', 'one_by_one', 'should_receive_property'):
@has207

has207 Mar 17, 2012

this list only contains aliases that aren't already defined as functions (to maintain backwards compat and allow them to be called without parens)

return _getattr(self, name)()
+
elif name == 'mock':
return _getattr(self, 'mock')()
else:
@@ -230,6 +232,9 @@ def with_args(self, *kargs, **kwargs):
Returns:
- self, i.e. can be chained with other Expectation methods
"""
+ if self.is_property:
+ self.__raise(FlexmockError, 'Cannot use with_args on properties')
+
self.args = {'kargs': kargs, 'kwargs': kwargs}
return self
@@ -361,6 +366,9 @@ def ordered(self):
Returns:
- self, i.e. can be chained with other Expectation methods
"""
+ if self.is_property:
+ self.__raise(FlexmockError, 'cannot order properties')
+
self._ordered = True
FlexmockContainer.ordered.append(self)
return self
@@ -512,6 +520,36 @@ def __call__(self, *kargs, **kwargs):
"""Hack to make Expectation.mock() work with parens."""
return self
+ def check_that_property_is_not_already_mocked(self, prop):
+ for expectation in FlexmockContainer.flexmock_objects[self]:
+ if expectation.method == prop and expectation.is_property == False:
+ raise FlexmockError('%s has already been mocked as a method.' % prop)
+
+ def check_that_method_is_not_already_mocked(self, prop):
+ for expectation in FlexmockContainer.flexmock_objects[self]:
+ if expectation.method == prop and expectation.is_property == True:
+ raise FlexmockError('%s has already been mocked as a property.' % prop)
+
@has207

has207 Mar 17, 2012

The above two should be protected (leading underscore) so as not to mix with the public API

+ def should_receive_property(self, prop):
+ """Adds a property Expectation to the provided class or instance.
+
+ Args:
+ - method: string name of the method to add
+
+ Returns:
+ - Expectation object
+ """
+ if self not in FlexmockContainer.flexmock_objects:
+ FlexmockContainer.flexmock_objects[self] = []
+
+ self.check_that_property_is_not_already_mocked(prop)
+
+ expectation = self._create_expectation(prop, is_property=True)
+ if expectation not in FlexmockContainer.flexmock_objects[self]:
+ FlexmockContainer.flexmock_objects[self].append(expectation)
+ self._update_property(expectation, prop)
+ return expectation
+
def should_receive(self, method):
"""Adds a method Expectation to the provided class or instance.
@@ -540,10 +578,13 @@ def should_receive(self, method):
exc_msg = 'old-style classes do not have a __new__() method'
raise FlexmockError(exc_msg)
if chained_methods:
- return_value = Mock()
+ return_value = flexmock()
@has207

has207 Mar 17, 2012

flexmock() is the public entry point to the API, it should not be used internally

chained_expectation = return_value.should_receive(chained_methods)
if self not in FlexmockContainer.flexmock_objects:
FlexmockContainer.flexmock_objects[self] = []
+
+ self.check_that_method_is_not_already_mocked(method)
+
expectation = self._create_expectation(method, return_value)
if expectation not in FlexmockContainer.flexmock_objects[self]:
FlexmockContainer.flexmock_objects[self].append(expectation)
@@ -588,19 +629,27 @@ def new_instances(self, *kargs):
else:
raise FlexmockError('new_instances can only be called on a class mock')
- def _create_expectation(self, method, return_value=None):
+ def _create_expectation(self, method, return_value=None, is_property=False):
if method in [x.method for x in
FlexmockContainer.flexmock_objects[self]]:
expectation = [x for x in FlexmockContainer.flexmock_objects[self]
if x.method == method][0]
expectation = Expectation(
self._object, name=method, return_value=return_value,
- original_method=expectation.original_method)
+ original_method=expectation.original_method, is_property=is_property)
else:
expectation = Expectation(
- self._object, name=method, return_value=return_value)
+ self._object, name=method, return_value=return_value, is_property=is_property)
return expectation
+
+ def _update_property(self, expectation, prop):
+ def get_property_value(prop):
+ return method_instance(self)
+
+ method_instance = self._create_mock_method(prop)
+ setattr(self._object.__class__, prop, property(fget=lambda self: get_property_value(prop)))
+
def _update_method(self, expectation, method):
method_instance = self._create_mock_method(method)
obj = self._object
@@ -763,19 +812,19 @@ def _format_args(method, arguments):
return '%s(%s)' % (method, args)
-def _create_partial_mock(obj_or_class, **kwargs):
+def _create_partial_mock(mock_class, obj_or_class, **kwargs):
matches = [x for x in FlexmockContainer.flexmock_objects
if x._object is obj_or_class]
if matches:
mock = matches[0]
else:
- mock = Mock()
+ mock = mock_class()
mock._object = obj_or_class
for method, return_value in kwargs.items():
mock.should_receive(method).and_return(return_value)
if not matches:
FlexmockContainer.add_expectation(mock, Expectation(obj_or_class))
- if (_attach_flexmock_methods(mock, Mock, obj_or_class) and
+ if (_attach_flexmock_methods(mock, mock_class, obj_or_class) and
not _isclass(mock._object)):
mock = mock._object
return mock
@@ -918,10 +967,18 @@ def flexmock(spec=None, **kwargs):
Returns:
Mock object if no spec is provided. Otherwise return the spec object.
"""
+
+ class MockImpl(Mock):
+ """
+ This class exists to make each instance of a mock be unique class,
+ making it so we can add properties.
+ """
+ pass
+
if spec is not None:
- return _create_partial_mock(spec, **kwargs)
+ return _create_partial_mock(MockImpl, spec, **kwargs)
else:
- return Mock(**kwargs)
+ return MockImpl(**kwargs)
# RUNNER INTEGRATION
View
@@ -1011,6 +1011,38 @@ def method1(self):
flexmock(foo).should_receive('method1.method2.method3').and_return('bar')
assertEqual('bar', foo.method1().method2().method3())
+ def test_should_mock_properties(self):
+ mock = flexmock()
+ mock.should_receive_property('foo').and_return('bar')
+ assert mock.foo == 'bar'
+
+ def test_should_not_allow_with_args_on_properties(self):
+ assertRaises(FlexmockError, lambda: flexmock().should_receive_property('foo').with_args("bar"))
+
+ def test_should_allow_multiple_returns_on_properties(self):
+ mock = flexmock()
+ mock.should_receive_property('foo').and_return('bar').and_return('baz')
+ assert mock.foo == 'bar'
+ assert mock.foo == 'baz'
+
+ def test_should_mock_property_and_verify(self):
+ mock = flexmock()
+ mock.should_receive_property('foo').and_return('bar').once
+ mock.foo
+ assertRaises(MethodCallError, lambda: mock.foo)
+
+ def test_should_fail_if_you_try_to_add_properties_and_method_with_same_name(self):
+ mock = flexmock()
+ mock.should_receive('foo').and_return('bar')
+ assertRaises(FlexmockError, lambda: mock.should_receive_property('foo'))
+
+ mock.should_receive_property('baz').and_return('')
+ assertRaises(FlexmockError, lambda: mock.should_receive('baz'))
+
+ def test_should_fail_if_you_try_to_order_properties(self):
+ mock = flexmock()
+ assertRaises(FlexmockError, lambda: mock.should_receive_property('foo').ordered)
+
def test_flexmock_should_replace_method(self):
class Foo:
def method(self, arg):

7 comments on commit 2c9a5ea

First off, thank you for working on this!

Some comments:

  1. I don't want to bloat the API by adding a should_receive_property method -- both should_receive and should_call need to handle this correctly just as they do for bound/unbound methods and module level functions. There should be nothing special from an API perspective when dealing with properties.
  2. I don't think this actually handles properties correctly. For instance, what would happen when you call flexmock(Class).should_receive_property vs flexmock(Instance).should_receive_property -- these cases are tricky and will require creating a new state object to attach to the parent class, dispatching calls to the mocked instance vs. all other instances of the class and tearing down correctly in flexmock_teardown.
  3. Attribute handling is a very different issue from properties and should be done separately -- I don't see much in the patch for it but your mail mentions stuff like should_have_attribute (see point 1 about API bloat). What would should_have_attribute add that assert doesn't provide already?

It's unlikely I'll have the time to work on these myself in the near term, but I'm happy to give more concrete guidance if you want to give it another stab :)

Cheers,

-Herman

Owner

brycecovert replied Mar 20, 2012

Thanks for the feedback. I think that being able to mock properties will greatly helpful for us. I'll take your feedback and make some more changes.

Thanks,
Bryce

Sounds good, I agree that having properties support would be cool (and I think unique as far as mocking libs go). Just FYI on how I envision this working:

flexmock(Class).should_receive(property) is the easy case so it's tempting to just do the standard replacement as is already done with regular methods. However, the more interesting case will be flexmock(Instance).should_receive(property) where you will basically need to figure out the Instance's class, and replace the property there. However, if you just replace it directly it will affect all other instances of Class (which would be the wrong behavior).

So what you really need to do is replace it with a method that keeps a lookup table of which instances are mocked, and what the replacement method should be used for each instance. This is because a user can potentially do flexmock(Instance1).should_receive(property).and_do(foo) as well as flexmock(Instance2).should_receive(property).and_do(bar) where foo and bar can be different methods that are called instead, etc. So the property-replacing function will in some sense replicate much of the logic that flexmock already does for normal functions, but tailored specifically to properties. A sort of mini-flexmock inside flexmock. There is already a FlexmockContainer singleton class that I use to keep other kinds of flexmock state so it should be easy to just stick the state object in there as well. Just make sure it gets cleaned up properly alongside other state in the teardown.

I think it would be possible to refactor main flexmock logic to make it reusable for properties and avoid some of the duplication, but I don't think that's as essential as getting it correct, so feel free to implement it separately as a first pass.

Hope that helps!

Cheers,

-Herman

Interesting note: I started playing around with property mocks and it seems like flexmock already supports it reasonably well on python2.x (for instances only) since it updates dict on the mocked instance and that takes lookup preference over the property which is defined on the class. Likewise, teardown just removes the dict entry and things just sort of work out...

This doesn't work on python3.x where the resolution order seems to have changed and property definitions win over local attributes on the instance. I'm also not 100% sure it's currently side-effect free when the property is looked up, though I think it should be.. Anyway, the correct solution would be the one I described earlier, but I'm actually a bit surprised that it seems to already work in what should be the majority common usage case.

BTW the above is the result of the support I added for replacing attributes earlier, so it wasn't working prior to that commit, nor does it support expectations as it's just replacing the property with an attribute. But it's kind of a happy accident that it should enable mocking properties, and now it puts more pressure on me to actually make it work correctly across all python versions and add expectation support.

Owner

brycecovert replied Mar 30, 2012

I'd tend to encourage defining the method that raises the custom exception separately and assign it as a property. Or alternately, using a partial mock once those have property support. I haven't really used Django though so I don't have a good sense of what would be the ideal solution. Any suggestions?

Please sign in to comment.