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 <kevi...@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 >
