aurora-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joshua Cohen <jco...@twopensource.com>
Subject Re: [DISCUSS] Deprecate use of mock.patch?
Date Thu, 20 Nov 2014 00:33:18 GMT
I'm actually waffling on my stance. I tried to frame it mentally in the
context of how I'd handle the same use case in javascript (a language I'm
much more comfortable with than Python), and I'd have a hard time arguing
in favor of a similar mechanism there (e.g. in node.js patching require to
globally inject a mock, ugh). I think my objection in the case of this
review is more due @app.command forcing us to delegate the injection to an
extracted method.

I tried to get a feel for what was more "pythonic" by searching for uses of
@mock.patch versus an injected mock from create_autospec on GitHub. The
former was definitely more common, but there's plenty of cases of the
latter, and they looked cleaned enough to me. I'm leaning towards lifting
my objection, though I'd love to hear thoughts from folks who have more
python experience (e.g. Brian, Joe) as well.

On Wed, Nov 19, 2014 at 4:20 PM, Maxim Khutornenko <maxim@apache.org> wrote:

> 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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message