aurora-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Maxim Khutornenko <ma...@apache.org>
Subject Re: [DISCUSS] Deprecate use of mock.patch?
Date Thu, 20 Nov 2014 00:20:55 GMT
I am with Joshua on this. The increased complexity and indirection is
not the tradeoff I would fight for. The lack of coverage is a bigger
problem in my opinion. Requiring patch-less unit tests may just
encourage a proliferation of un-pythonic patterns and more obstacles
on the way to improving our python code coverage.

On Wed, Nov 19, 2014 at 4:02 PM, Joshua Cohen <jcohen@twopensource.com> wrote:
> As I mentioned in that review, I'm not sold on the idea. I feel that it
> leads to a fair amount of extra code that exists solely to support testing.
> One of the nice things about dynamic languages is they allow you to avoid
> this sort of boilerplate. The main problem in that review is just that the
> wrong thing was being patched (instead of patching build_properties
> directly we should have patched from_pex). That being said, I can't
> actually argue against your points, they're all valid, I'm just not
> convinced that they're worth the tradeoff ;).
>
> On Wed, Nov 19, 2014 at 3:38 PM, Kevin Sweeney <kevints@apache.org> wrote:
>
>> Hi folks,
>>
>> I wanted to have a discussion about the usage of mock.patch in our unit
>> tests. In my opinion its use is a code smell versus writing production code
>> to have explicit injection points for test dependencies.
>>
>> The review at https://reviews.apache.org/r/28250/ is a good example of why
>> I think the patch approach is brittle: in this case the test code patched
>> out
>>
>>   @patch('twitter.common.python.pex.PexInfo.build_properties',
>> new_callable=PropertyMock)
>>
>>
>> but the production code didn't actually call PexInfo.build_properties -
>> rather it called PexInfo.from_pex, which usually returns a PexInfo
>> instance, which has a build_properties property. So this test only worked
>> when PexInfo.from_pex(sys.argv[0]) actually returned a valid PexInfo, but
>> due to the way patching works there's no way to ensure that the
>> function-under-test was the one calling the mocked method.
>>
>> In my opinion an explicit injection approach is preferable, via the use of
>> defaulted private method parameters like:
>>
>> def production_function(arg1, arg2, ..., _print=print,
>> _from_pex=PexInfo.from_pex):
>>    # use _print, _from_pex
>>
>> or
>>
>> class ProductionClass(object):
>>   def __init__(self, arg1, arg2, ..., _print=print,
>> _from_pex=PexInfo.from_pex):
>>     self._print = _print
>>     self._from_pex = _from_pex
>>
>>   def method(self):
>>     # Use self._print, self._from_pex, etc
>>
>> Then tests can explicitly replace the dependencies of the unit-under-test
>> with mocks:
>>
>> def test_production_function():
>>   mock_print = create_autospec(print, spec_set=True)
>>   mock_pex_info = create_autospec(PexInfo, instance=True, spec_set=True)
>>   mock_from_pex = create_autospec(PexInfo.from_pex, spec_set=True,
>> return_value=mock_pex_info)
>>   mock_pex_info.build_properties = {}
>>
>>
>>   production_function(arg1, arg2, ..., _print=mock_print,
>> _from_pex=mock_from_pex)
>>   # or
>>   prod = ProductionClass(arg1, arg2, ..., _print=mock_print,
>> _from_pex=mock_from_pex)
>>   prod.method()
>>
>>   mock_print.assert_called_once_with("Some string")
>>   # other assertions about what the class-under-test did with the mocked
>> deps
>>
>> There are a good number of properties that this allows:
>>
>> 1. No unused dependencies - if a parameter is unused the linter will still
>> complain
>> 2. Can't mock out something the unit-under-test isn't actually using - if
>> you give a kwarg parameter that isn't defined the test will raise a
>> TypeError
>> 3. No action-at-a-distance - you can't mock the dependency-of-a-dependency
>> (in this case PexInfo.build_properties instead of PexInfo.from_pex)
>> 4. Thread-safety - patch is not thread-safe as it's temporarily replacing
>> global state for the duration of the test.
>>
>> I'd like to propose that we consider use of mock.patch in our tests to be a
>> code smell that should be refactored to use explicit injection at our
>> earliest convenience
>>

Mime
View raw message