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