Mailing-List: contact dev-help@aurora.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@aurora.incubator.apache.org
Received-SPF: pass (nike.apache.org: domain of yasumoto7@gmail.com designates
 209.85.220.44 as permitted sender)
MIME-Version: 1.0
In-Reply-To: 
 <CAGRA8uNunn_psLb7EtiGPoKW+jNP=emdtnCZWa+YmsyQj3skHA@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>
From: Joe Smith <yasumoto7@gmail.com>
Date: Fri, 21 Nov 2014 14:54:22 -0800
Message-ID: 
 <CA+y6w6p1KXbt3_DB4d5bxGGk7oHZhh6h7NtTmu5=5W9sThxbTA@mail.gmail.com>
Subject: Re: [DISCUSS] Deprecate use of mock.patch?
To: dev@aurora.incubator.apache.org
Content-Type: multipart/alternative; boundary=e89a8ff1cc3aa68b1e0508665440

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

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.

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

--e89a8ff1cc3aa68b1e0508665440--