Fix bug that caused pex not to cache the results of translation.

Review Request #1666 — Created Jan. 25, 2015 and submitted

ugodiggi
pex
ugodiggi:cache_translation_results
https://github.com/pantsbuild/pex/pull/35
573a5e5...
pants-reviews
patricklaw, wickman

Fix bug that caused pex not to cache the results of translation.

I'm under the impression that this is a simple "typo" error, and the intention was also to pass through the parameter to the chained translators.

Of course it's possible that this was intentional and I'm really missing the point :-)

This works well and seems to cache artifacts correctly in the UrbanCompass code base.

Of course I also run the tests (only for py27 though, that's all I have on this system):

$ tox
GLOB sdist-make: /Users/ugo/development/pex/setup.py
pypy-requests create: /Users/ugo/development/pex/.tox/pypy-requests
ERROR: InterpreterNotFound: pypy
py27-requests create: /Users/ugo/development/pex/.tox/py27-requests
py27-requests installdeps: pytest, twitter.common.contextutil>=0.3.1,<0.4.0, twitter.common.dirutil>=0.3.1,<0.4.0, twitter.common.lang>=0.3.1,<0.4.0, twitter.common.testing>=0.3.1,<0.4.0, wheel, mock, requests, responses
py27-requests inst: /Users/ugo/development/pex/.tox/dist/pex-0.8.3.zip
py27-requests runtests: PYTHONHASHSEED='2621098136'
py27-requests runtests: commands[0] | py.test
============================================================================================================ test session starts =============================================================================================================
platform darwin -- Python 2.7.8 -- py-1.4.26 -- pytest-2.6.4
collected 56 items

tests/test_archiver.py .
tests/test_compatibility.py .
tests/test_crawler.py ......
tests/test_environment.py ...
tests/test_finders.py ....
tests/test_http.py ..........
tests/test_integration.py ...
tests/test_interpreter.py .
tests/test_iterator.py ...
tests/test_link.py ...
tests/test_package.py ..
tests/test_pep425.py ..
tests/test_pex.py ....
tests/test_pex_bootstrapper.py .
tests/test_pex_builder.py .
tests/test_pex_info.py .
tests/test_platform.py .....
tests/test_resolver.py ..
tests/test_util.py ...

========================================================================================================= 56 passed in 8.33 seconds ==========================================================================================================
py34-requests create: /Users/ugo/development/pex/.tox/py34-requests
ERROR: InterpreterNotFound: python3.4
style create: /Users/ugo/development/pex/.tox/style
style installdeps: twitter.checkstyle
style inst: /Users/ugo/development/pex/.tox/dist/pex-0.8.3.zip
style runtests: PYTHONHASHSEED='2621098136'
style runtests: commands[0] | twitterstyle -n ImportOrder /Users/ugo/development/pex/pex /Users/ugo/development/pex/tests
isort-check create: /Users/ugo/development/pex/.tox/isort-check
isort-check installdeps: isort
isort-check inst: /Users/ugo/development/pex/.tox/dist/pex-0.8.3.zip
isort-check runtests: PYTHONHASHSEED='2621098136'
isort-check runtests: commands[0] | isort -ns init.py -rc -c /Users/ugo/development/pex/pex /Users/ugo/development/pex/tests
__________________ summary ___________________
SKIPPED: pypy-requests: InterpreterNotFound: pypy
py27-requests: commands succeeded
SKIPPED: py34-requests: InterpreterNotFound: python3.4
style: commands succeeded
isort-check: commands succeeded
congratulations :)

JS
  1. For reference, this bug - I do think its a typo bug - was introduced here: https://rbcommons.com/s/twitter/r/785

    1. My shipit is non-binding here and you'll need a release from Wickman iiuc anyhow so I'll pass patch-in duty to Brian.

    2. Yep that was understood :-)
      Thank you for the speedy look all the same!

  2. 
      
PA
  1. =John. Wickman is the final authority here, though this patch seems like a no-brainer.

  2. 
      
NH
  1. Could you add a regression test that covers this behavior?

    1. Can you give me more details on what you would like me to test?

      I can easily do something like this if you'd like:

      translators = [mock.MagicMock() for i in range(3)]
      translators[1].translate.return_value = "fake_success"
      t = ChainedTranslator(*translators)
      t.translate("fake_package", "fake_into")
      translators[0].translate.assert_called_with("fake_package", into="fake_into")
      translators[1].translate.assert_called_with("fake_package", into="fake_into")
      assert not translators[2].translate.called
      
    2. Sure. It just seemed to me from skimming the affected code that it was only covered with integration tests. If this has the potential to make perf better, it'd be nice to be sure that it doesn't get typoed out during another refactor.

      For a unit test of just ensuring that into is passed to the called translator, all you'd need is

      translator = mock.MagicMock()
      
      t = ChainedTranslator(translator)
      t.translate("fake_package", "fake_into")
      
      translator.translate.assert_called_with("fake_package", into="fake_into")
      

      It'd be good if there were a separate test for the "pick first" feature. Maybe something like this:

      initial_empty_translator = mock.MagicMock()
      translator_with_value = mock.MagicMock()
      translator_with_value.translate.return_value = "fake_success"
      translator_after_value = mock.MagicMock()
      
      t = ChainedTranslator(translator)
      result = t.translate("fake_package", "fake_into")
      
      assert initial_empty_translator.translate.called
      assertEqual result, "fake_success"
      assert not translator_after_value.translate.called
      
  2. 
      
IT
  1. Ship It!
  2. 
      
UG
NH
  1. Thanks for the tests!

  2. 
      
MA
  1. This got merged https://github.com/pantsbuild/pex/commit/49705fb6cc40b9bdc178ccde4f95ae6763aafdb5
    so this can be closed. Ugo, could you mark it as submitted?

  2. 
      
UG
Review request changed

Status: Closed (submitted)

Loading...