Eliminate use of mox in favor of mock.

Review Request #4143 — Created Aug. 7, 2016 and submitted

jsirois
pants
jsirois/mox/eliminate
3760
4142
7db40a5...
pants-reviews
kwlzn
3rdparty/python/requirements.txt                |  1 -
 tests/python/pants_test/base/BUILD              |  2 +-
 tests/python/pants_test/base/test_hash_utils.py | 34 +++++++++++----------
 tests/python/pants_test/process/BUILD           |  2 +-
 tests/python/pants_test/process/test_xargs.py   | 81 ++++++++++++++++++++++++++++----------------------
 tests/python/pants_test/util/BUILD              |  1 -
 tests/python/pants_test/util/test_dirutil.py    | 36 ++++++++++------------
 7 files changed, 83 insertions(+), 74 deletions(-)

Locally green:

./pants test \
    tests/python/pants_test/{base:hash_utils,process,util:dirutil}

CI went green here:
https://travis-ci.org/pantsbuild/pants/builds/151082843

JS
JS
  1. The fixed-up python 3rdparty include docs are rendered here:
      http://pantsbuild.github.io/staging/jsirois/mox/eliminate/3rdparty_py.html
    
    This is as compared to production docs here:
      http://www.pantsbuild.org/3rdparty_py.html
  2. 
      
JS
KW
  1. lgtm! two take-em-or-leave-em comments.

  2. tests/python/pants_test/base/test_hash_utils.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    I realize you're just porting mox->mock here, but this strikes me as something that doesn't actually need to be mocked (e.g. could just verify the behavior of hash_all/hash_file against a live hashlib.md5 instance)?

    1. Agreed - I was being a bit to mechanical.
  3. tests/python/pants_test/util/test_dirutil.py (Diff revision 2)
     
     
     
     
     

    these mocks are fairly straightforward since they're at the function level - but as a general rule of thumb it's a good idea to always pass (..., autospec=True, spec_set=True) when using mock.patch for strictness:

    >>> with mock.patch('os.getpid') as mock_getpid:
    ...   os.getpid.this_doesnt_exist()
    ... 
    <MagicMock name='getpid.this_doesnt_exist()' id='4343352400'>
    

    vs:

    >>> with mock.patch('os.getpid', autospec=True, spec_set=True) as mock_getpid:
    ...   os.getpid.this_doesnt_exist()
    ... 
    Traceback (most recent call last):
      File "<input>", line 2, in <module>
        os.getpid.this_doesnt_exist()
      File "/private/var/folders/3t/xkwqrkld4xxgklk2s4n41jb80000gn/T/tmpVDWgGV/.deps/mock-2.0.0-py2.py3-none-any.whl/mock/mock.py", line 698
    , in __getattr__
        raise AttributeError("Mock object has no attribute %r" % name)
    AttributeError: Mock object has no attribute 'this_doesnt_exist'
    
  4. 
      
JS
KW
  1. Ship It!
  2. 
      
JS
JS
JS
JS
Review request changed

Status: Closed (submitted)

Change Summary:

Now on master:

git log -1 origin/master
commit db053de2ad0bb5db0ce12532654c70321ac3e7f3
Author: John Sirois <john.sirois@gmail.com>
Date:   Tue Aug 9 19:25:15 2016 -0600

    Eliminate use of mox in favor of mock.
    
    Testing Done:
    Locally green:
    ```
    ./pants test \
        tests/python/pants_test/{base:hash_utils,process,util:dirutil}
    ```
    
    CI went green here:
      https://travis-ci.org/pantsbuild/pants/builds/151082843
    
    Bugs closed: 3760
    
    Reviewed at https://rbcommons.com/s/twitter/r/4143/
Loading...