aurora-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David McLaughlin <da...@dmclaughlin.com>
Subject Re: [DISCUSS] Deprecate use of mock.patch?
Date Fri, 21 Nov 2014 23:28:11 GMT
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