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
