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: 
 <CAOOJoEyjY=QftNMo6m8rez-SKhGNoctCn+Bd=6b7QT42sOcsdQ@mail.gmail.com>
References: 
 <CAAATh-Yj0tbkRKTesVOXQ=YuD3RMtrRSYWnNMs7Go7UF8MS_Dg@mail.gmail.com>
	<CAMnduq-APuPJuoCEkVLJCqwtcoVQunu7xS5zQyhcSdjSnPFPFA@mail.gmail.com>
	<CAOTkfX4xpBRASdaoBo4Gf2_CY+ccvOfrBePrNm1w3R3p1HJ=EA@mail.gmail.com>
	<CAMnduq9s5kuaMjZHdp2=9J__yKTpLEP4B4zE88hEug2VmSgF=A@mail.gmail.com>
	<CAAATh-ZMMXFATZvtMvkJh9joyo4kPdgM-2JUuHvkihnFEws+pg@mail.gmail.com>
	<CAMnduq_EDUpf=hnTfpDoqc+wivHni4oOXEk6xGsyBj5r7+cr7w@mail.gmail.com>
	<CAAATh-bRr+o4OiRm0z0B+-F+RUSk_WBKy8T_kOA8YiQE+3OoTw@mail.gmail.com>
	<CAGRA8uNunn_psLb7EtiGPoKW+jNP=emdtnCZWa+YmsyQj3skHA@mail.gmail.com>
	<CA+y6w6p1KXbt3_DB4d5bxGGk7oHZhh6h7NtTmu5=5W9sThxbTA@mail.gmail.com>
	<CAAATh-YtEg8_-rk1bA747CderAnXbVb74uCNuQ-keDT1SXceJQ@mail.gmail.com>
	<CAOOJoEyjY=QftNMo6m8rez-SKhGNoctCn+Bd=6b7QT42sOcsdQ@mail.gmail.com>
Date: Fri, 21 Nov 2014 15:57:41 -0800
Message-ID: 
 <CAFTdr0D6eKv0gv-RNHjhf-eDT-AmtZSQ9s65_aBY8R68=qATog@mail.gmail.com>
Subject: Re: [DISCUSS] Deprecate use of mock.patch?
From: Brian Wickman <wickman@apache.org>
To: dev@aurora.incubator.apache.org
Content-Type: multipart/alternative; boundary=089e010d8a82dcfe0205086735d1

--089e010d8a82dcfe0205086735d1
Content-Type: text/plain; charset=UTF-8

I sent an e-mail about this a couple days but it got swallowed since I
hadn't updated my Apache mail settings.

My opinion is the same as David's.  Explicitly calling out every stubbable
method in constructors / functions is just Guiceifying our code.  mock
wouldn't be part of the Python standard library if it wasn't sensible to
some degree.  That isn't to say we can't do injected mocks -- search for
'_class' in src/main/python to see some examples where we do this -- but it
means that it shouldn't necessarily be the approach when mock.patch does
what we need succinctly (caveat multi-threading etc, but ideally our tests
aren't running in multi-threaded environments -- that's a separate problem
we should fix.)

On Fri, Nov 21, 2014 at 3:28 PM, David McLaughlin <david@dmclaughlin.com>
wrote:

> I'm -1 to blanket scrubbing of legitimately useful testing techniques like
> mock.patch. I find mock.patch a lot cleaner than polluting the interface
> with every function the code under test might need to call.
>
> I'm definitely +1 to not mocking things deeper than 1 in the stack. It's
> one of the strangest parts of our test suites, but IMO it's caused more by
> the fact than many of "unit" tests are actually testing multiple units, to
> the point where they are almost integration tests. I'd like to see us focus
> on improving that.
>
> On Fri, Nov 21, 2014 at 3:17 PM, Kevin Sweeney <kevints@apache.org> wrote:
>
> > On Fri, Nov 21, 2014 at 2:54 PM, Joe Smith <yasumoto7@gmail.com> wrote:
> >
> > > I'm with Josh and Maxim here.
> > >
> > > I'd prefer to hang onto mock.patch but keep our use of create_autospec
> > and
> > > spec_set=True, along with asserting the mock_calls list.
> > >
> > > In addition, we should only patch one layer deep- one time I patched ~3
> > > layers deep then needed to refactor, which meant all of my tests needed
> > to
> > > change, which didn't give me any assurance about my actual behavior
> > > anymore.
> > >
> > > How do you propose to enforce that patches stay one layer deep across
> > refactors?
> >
> >
> > > On Fri, Nov 21, 2014 at 1:09 PM, Bill Farner <wfarner@apache.org>
> wrote:
> > >
> > > > I see using mock.patch as equivalent to extending a class and
> changing
> > > the
> > > > behavior of a method, which i strongly prefer to avoid.  At the very
> > > least,
> > > > we should strictly avoid patching behavior layers down in the stack
> of
> > > what
> > > > is being tested.
> > > >
> > > > I'd be happy with punting on a best practice to eradicate mock.patch
> as
> > > > long as we accept that patching things N layers deep in the call
> stack
> > > (for
> > > > N > 1) is an anti-pattern that we need to scrub our code of.
> > > >
> > > >
> > > > -=Bill
> > > >
> > > > On Thu, Nov 20, 2014 at 10:11 AM, Kevin Sweeney <
> > > > ksweeney@twitter.com.invalid> wrote:
> > > >
> > > > > Brian and Bill, do you have any thoughts here?
> > > > >
> > > > > On Wednesday, November 19, 2014, Joshua Cohen <
> > jcohen@twopensource.com
> > > >
> > > > > wrote:
> > > > >
> > > > > > That's a fairly contrived example though, as most Java classes
> > don't
> > > > > expose
> > > > > > a mechanism for injecting mocks.
> > > > > >
> > > > > > I think points #3 and #4 make the strongest case for why we'd
> want
> > to
> > > > > avoid
> > > > > > this (though I don't believe we currently run tests in parallel
> so
> > #4
> > > > is
> > > > > > more of a nice-to-have). If it's generally limited to additional
> > > method
> > > > > > args (and the review pointed at here is an outlier due to the way
> > > > > > @app.command works) I'm on board.
> > > > > >
> > > > > > On Wed, Nov 19, 2014 at 4:59 PM, Kevin Sweeney
> > > > > > <ksweeney@twitter.com.invalid
> > > > > > > wrote:
> > > > > >
> > > > > > > I don't think this a dynamic vs static language thing - if this
> > > were
> > > > > Java
> > > > > > > we could just as easily do
> > > > > > >
> > > > > > > public class MyTest {
> > > > > > >   private PrintStream oldSystemOut;
> > > > > > >
> > > > > > >   @Before
> > > > > > >   public void setUp() {
> > > > > > >     oldSystemOut = System.out;
> > > > > > >     System.setOut(mockPrintStream);
> > > > > > >   }
> > > > > > >
> > > > > > >   @After
> > > > > > >   public void tearDown() {
> > > > > > >     System.setOut(oldSystemOut);
> > > > > > >   }
> > > > > > > }
> > > > > > >
> > > > > > > in our tests but that's mutable global state and makes our code
> > > > brittle
> > > > > > for
> > > > > > > exactly the same 4 reasons as above.
> > > > > > >
> > > > > > > I don't think there's anything about Python that makes mutable
> > > global
> > > > > > state
> > > > > > > an inherently better idea.
> > > > > > >
> > > > > > > On Wed, Nov 19, 2014 at 4:33 PM, Joshua Cohen <
> > > > jcohen@twopensource.com
> > > > > > <javascript:;>>
> > > > > > > wrote:
> > > > > > >
> > > > > > > > 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
> > > > > > <javascript:;>>
> > > > > > > > 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 <javascript:;>
> > > > > > > >
> > > > > > > > > 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
> > > > > > <javascript:;>>
> > > > > > > > > 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
> > > > > > > > > >>
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > Kevin Sweeney
> > > > > > > @kts
> > > > > > >
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Sent from Gmail Mobile
> > > > >
> > > >
> > >
> >
>

--089e010d8a82dcfe0205086735d1--