On Fri, Nov 21, 2014 at 3:57 PM, Brian Wickman <wick...@apache.org> wrote:
> 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.)
Why is running our presumably thread-safe code in a multi-threaded test
environment not desirable?
>
> On Fri, Nov 21, 2014 at 3:28 PM, David McLaughlin <da...@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 <kevi...@apache.org>
> wrote:
> >
> > > On Fri, Nov 21, 2014 at 2:54 PM, Joe Smith <yasumo...@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 <wfar...@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 <
> > > > > kswee...@twitter.com.invalid> wrote:
> > > > >
> > > > > > Brian and Bill, do you have any thoughts here?
> > > > > >
> > > > > > On Wednesday, November 19, 2014, Joshua Cohen <
> > > jco...@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
> > > > > > > <kswee...@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 <
> > > > > jco...@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 <
> > > > > ma...@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 <
> > > > > > > jco...@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 <
> > > > > > kevi...@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
> > > > > >
> > > > >
> > > >
> > >
> >
>
