Fix a scope bug for java agent manifest writing.

Review Request #768 — Created July 28, 2014 and submitted

jsirois
pants
jsirois/publish/fix_agent_manifest
401
pants-reviews
jinfeng, johanoskarsson
commit 9ebaa5dc21b22175af5581f906fef157e01a6d06
Author: John Sirois <jsirois@twitter.com>
Date:   Sat Jul 26 00:23:17 2014 -0600

    Fix a scope bug for java agent manifest writing.
    
    Also fix a JavaAgent validation bug revelead by adding a test for
    JarBuilder agent handling.

 src/python/pants/backend/jvm/targets/java_agent.py |  2 +-
 src/python/pants/backend/jvm/tasks/jar_task.py     | 34 ++++++++++++------------
 tests/python/pants_test/tasks/test_jar_task.py     | 71 ++++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 84 insertions(+), 23 deletions(-)
$ PANTS_DEV=1 ./pants goal test tests/python/pants_test/tasks/:jar_task --test-pytest-options=-v
...
18:05:24 00:01   [test]
18:05:24 00:01     [pytest]
18:05:24 00:01       [run]
                     ============== test session starts ===============
                     platform linux2 -- Python 2.6.9 -- py-1.4.22 -- pytest-2.5.2 -- /usr/bin/python2.6
                     plugins: cov, timeout
                     collected 6 items 
                     
                     tests/python/pants_test/tasks/test_jar_task.py:111: JarTaskTest.test_custom_manifest PASSED
                     tests/python/pants_test/tasks/test_jar_task.py:86: JarTaskTest.test_overwrite_write PASSED
                     tests/python/pants_test/tasks/test_jar_task.py:102: JarTaskTest.test_overwrite_writestr PASSED
                     tests/python/pants_test/tasks/test_jar_task.py:57: JarTaskTest.test_update_write PASSED
                     tests/python/pants_test/tasks/test_jar_task.py:73: JarTaskTest.test_update_writestr PASSED
                     tests/python/pants_test/tasks/test_jar_task.py:133: JarBuilderTest.test_agent_manifest PASSED
                     
                     ============ 6 passed in 1.27 seconds ============
                     
18:05:26 00:03     [junit]
18:05:26 00:03     [specs]
               SUCCESS
JS
  1. Travis CI away here: https://travis-ci.org/pantsbuild/pants/builds/31001358
  2. 
      
JI
  1. 
      
  2. wow....goot catch, but then makes me wonder should we add some unittests for this class? I checked tests/python/pants_test/backend, it's very very sparse there...
    
    or are this class's functionalities covered in some other outer classes' integration tests?
    
    what's our policies around writing unittests and IT tests? is UT mandatory?
    1. This JarTaskTest caught the bug.  Agreed that it would be good to have target tests too though for any test validating inputs like this.  That said - I'd like to defer since this RB does add coverage.
    2. To be clear, the unit test passes both premain and agent_class and the old logic always raised TargetDefinitionException in that case.  So I needed to fix to get this test working.
    3. no, i'm not suggesting for this RB to add that. It can be deferred as some of our hackweek/tech debt projects. But I want to get a clarification on how we should move forward dealing with addition of new code.
    4. Oh - also - tests do not mirror src for the backends.  We used to have {src,tests}/.../{targets,tasks}.  Although in the src tree these got moved to backends/* in the tests tree they did not.  That needs to be cleaned up at some point for discoverability of tests.
    5. Thanks for pushing - isolated test and fix here: https://rbcommons.com/s/twitter/r/770/
      I may merge this RB to master ahead of that RB, but I'll rebase adjust whoever wins since they both have the same JavaAgent fix.
  3. 
      
JS
  1. Thanks Jin - submitted @ https://github.com/pantsbuild/pants/commit/49f27fec9a92540b22ea7a58d1c989a54ab43d2c
  2. 
      
JS
Review request changed

Status: Closed (submitted)

Loading...