aurora-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kevin Sweeney <kswee...@twitter.com.INVALID>
Subject Re: [DISCUSS] Deprecate use of mock.patch?
Date Thu, 20 Nov 2014 00:59:37 GMT
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>
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>
> 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>
> > 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>
> > 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

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