Mailing-List: contact dev-help@aurora.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@aurora.incubator.apache.org
MIME-Version: 1.0
In-Reply-To: 
 <CAOTkfX4xpBRASdaoBo4Gf2_CY+ccvOfrBePrNm1w3R3p1HJ=EA@mail.gmail.com>
References: 
 <CAAATh-Yj0tbkRKTesVOXQ=YuD3RMtrRSYWnNMs7Go7UF8MS_Dg@mail.gmail.com>
	<CAMnduq-APuPJuoCEkVLJCqwtcoVQunu7xS5zQyhcSdjSnPFPFA@mail.gmail.com>
	<CAOTkfX4xpBRASdaoBo4Gf2_CY+ccvOfrBePrNm1w3R3p1HJ=EA@mail.gmail.com>
Date: Fri, 21 Nov 2014 15:16:43 -0800
Message-ID: 
 <CAAATh-bpCWZ618u2uC-W5e=68mkHKftLrBRdM59+rjE_JUog0A@mail.gmail.com>
Subject: Re: [DISCUSS] Deprecate use of mock.patch?
From: Kevin Sweeney <kevints@apache.org>
To: Aurora <dev@aurora.incubator.apache.org>
Content-Type: multipart/alternative; boundary=001a11c224185217f8050866a3d7

--001a11c224185217f8050866a3d7
Content-Type: text/plain; charset=UTF-8

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.
>
The executor and the client are both multi-threaded Python applications
(arguably not very Pythonic either) and using patch prevents us from
writing thread-safe tests.

>
> 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
> >>
>

--001a11c224185217f8050866a3d7--