aurora-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Brian Wickman <wick...@apache.org>
Subject Re: [DISCUSS] Deprecate use of mock.patch?
Date Fri, 21 Nov 2014 23:57:41 GMT
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
> > > > >
> > > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message