On Wed, Nov 19, 2014 at 4:20 PM, Maxim Khutornenko <ma...@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. > The executor and the client are both multi-threaded Python applications (arguably not very Pythonic either) and using patch prevents us from writing thread-safe tests. > > On Wed, Nov 19, 2014 at 4:02 PM, Joshua Cohen <jco...@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 <kevi...@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 > >> >
