Run python tests under goal.

Review Request #339 — Created May 13, 2014 and discarded

benjyw
pants
pants-reviews
jsirois, wickman
This introduces a new task to run python tests.

It leaves the old-pants test running intact, so
there's no change for existing users.

Required switching python targets to new-style
resources, with automatic conversion of old-style
file resources behind the scenes. So, again,
there's no change for existing users/repos.
time PANTS_DEV=1 ./pants goal test tests/python/pants_test:all
time PANTS_DEV=1 ./pants tests/python/pants_test:all
  • 1
  • 0
  • 2
  • 0
  • 3
Description From Last Updated
it seems like you're reimplementing the SetupPythonEnvironment task. maybe use that instead? WI wickman
JS
  1. One problem and it should be pretty straight forward to rig a unit test for it.
  2. The resource_paths() call yields paths relative to their target_base over potentially many different target_bases.  If one of those is != library.target_base this breaks.
    1. Fixed a while ago in a separate change.
  3. src/python/pants/python/test_builder.py (Diff revision 1)
     
     
    This needs a BUILD addition for workunit ... or maybe keep the test builder workunit free for now and just add defaulted stdout, stderr as args to run and let the task keep the workunit stream origins to itself.
    1. Kept the test builder workunit free. Also added the build dep on workunit for the task.
  4. We really need a utility or a context property that looks at isatty and respects env vars that we can temper coloring with.  We have internal complaints about this.  I'm fine with a TODO / issue and a follow-up
  5. consider: https://github.com/twitter/commons/blob/master/src/python/twitter/common/contextutil/__init__.py#L34
            with environment_as(COLUMNS=str(int(cols) - 30)):
              ...
  6. s/pytest_/python_/
  7. 
      
JS
  1. Brian may or may not have time to look at this, but it would be good to add him to the review and give him a fair chance to chime in.
  2. 
      
BE
WI
  1. Is this intended to just be a stop-gap so that tests can be run in a unified manner, or are you actually planning on putting work into a Python backend for goal?
    
    Given that we've written a Python backend for goal once before, if we do want to staff that effort, I'd like to mirror the previous implementation as much as possible.  The 18 month old diff is here, purely as a historical document: https://gist.github.com/wickman/8d34a7c578f8c5be44bf  I'm not going to block you from putting this code on master as-is, but I'd like to give some background on how we did it before.
    
    The design back then involved a few setup phases:
      setup -> roots -> resolve
    
    setup is responsible for setting up all the python interpreters / paths necessary to execute all the subsequent python tasks
    
    roots establishes which target closures will be operated upon (e.g. do you want to union them all together or treat them on a case by case basis?)  this determines the granularity of subsequent tasks.
    
    resolve resolves all the external dependencies for all python roots.
    
    Then from there everything is just a "ChrootTask" which is essentially a helper class to invoke PEX environments in various ways.  For example the test runner is just a ChrootTask with a pytest entry point, but otherwise is resolved and built the same as all other chroots.  Lint was just a pylint entry point, CreatePex just called .freeze() on the environment, etc.  And since 'setup' allowed you to pick N interpreters instead of 1, you could run each of the things (e.g. tests) with multiple interpreters simultaneously.
    
    
    
    1. I'm pretty darn sure this is a stop-gap.
    2. This is a stop-gap, if you want to call it that, so we can move forward on getting rid of command. I don't want that to be contingent on a full reimplementation of the python backend. The latter work is up in the air right now, as it's not staffed or prioritized, whereas the former needs to happen for a variety of reasons. For example, getting rid of command will make implementing the new cmd-line flags scheme easier. 
      
      So the goal here is to monkey-patch the existing Python support into goal in a way that doesn't make things worse, but also doesn't get in the way of a proper reimplementation along the lines you suggest later on. 
      
      Does that make sense?
  2. 
      
WI
  1. 
      
  2. 'test_target' will always be truthy in python 3.x, since filter() just produces a view.
    
    do test_targets = list(filter(...)) or create a comprehension
    1. Good catch. Fixed.
  3. it seems like you're reimplementing the SetupPythonEnvironment task.  maybe use that instead?
    1. That seems to be pretty broken, e.g., it relies on PythonRoot, which is really broken. What's the intended idiomatic use of python_root? 
  4. 
      
BE
  1. 
      
  2. Post pl's refactor this code is no longer relevant anyway... 
    
    I've sent a code review for a separate change that completely re-implements new-style resource support for python. That shouldn't suffer from the issue you mention here.
    
    I'll pick up the rest of this change after that other one is in master.
    1. Picking this up again... I've addressed the comments, but will close this review in favor of a new one, as the underlying code has changed so much, this diff is no longer relevant.
  3. 
      
BE
Review request changed

Status: Discarded

Loading...