aurora-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kevin Sweeney <kevi...@apache.org>
Subject [DISCUSS] Deprecate use of mock.patch?
Date Wed, 19 Nov 2014 23:38:57 GMT
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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message