A fast mode for python tests.

Review Request #405 — Created May 23, 2014 and submitted

benjyw
pants
pants-reviews
jsirois, wickman
This mode runs all tests in a single chroot and a single python interpreter,
instead of building a new chroot and spawning a new interpreter for every
test target.

Benefits: 1) The pants tests run in 12 seconds instead of 160 seconds.
          2) Test isolation can actually hide problems sometimes.

Drawbacks: Individual chroots are better for verifying that a test
           target's dependencies are self-contained, i.e., that they
           reflect all the actual deps. When running multiple tests
           in a single chroot, test B could provide a dep that test A
           relies on but doesn't declare, thus hiding a missing dep
           problem.

This drawback is minor - it only verifies that the test target's
deps are good, not the deps of the target being tested.

If this works out we can consider making --fast the default in the future. The
equivalent functionality is already all we do for jvm tests: we throw all
our tests at JUnit in one pass.

Note that this change relies on https://rbcommons.com/s/twitter/r/403/
and will ultimately be merged with it before pushing.

Note also that this change gets rid of some crufty features of python_tests, namely
the ability to specify entry_point, soft_dependencies and timeouts in BUILD files.
[On the merged changes]

Ran all pants unittests in regular and fast mode.

WI
  1. logic-wise looks good to me.
    
    now that we've moved back to pytest as the default runner, i'd love to apply my coverage merging change to pantsbuild.  it also has a parallel test runner, which serializes chroot building but parallelizes test runs.  now that BUILD parsing is thread-safe, we can probably parallelize the chroot building part too.
    1. BUILD file parsing isn't necessarily thread safe, in fact it is probably not thread safe.  However it would be much easier now to do a sweep of that class and force it to be thread safe.
    2. but you removed the chdirs, right?
    3. Yes, but there are now more moving parts for deciding whether a BUILD file has been parsed and the result cached, whether the TargetProxies generated by parsing the BUILD file have been registered, etc.  _Probably_ the worst that could happen is reparsing a BUILD file that didn't need it, but I'm not sure of this at all.
  2. src/python/pants/python/test_builder.py (Diff revision 1)
     
     
     
    our style guide prohibits trailing \.  instead we prefer ()s, e.g.
    
    fail_hard = ('PANTS_PYTHON'... and
        'PANTS_PY_COVERAGE' not in os.environ)
    
    1. Fixed. I think this style is wrong, but I bow to the will of the majority.
    2. \s in python is like semicolons in js.  religious differences i guess.
  3. src/python/pants/python/test_builder.py (Diff revision 1)
     
     
    alternately flip and use genexpr
    
    return 0 if all(rc.success for rc in results.values()) else 1
    1. That is more readable. Done.
  4. src/python/pants/python/test_builder.py (Diff revision 1)
     
     
    []s not necessary
  5. 
      
BE
Review request changed

Status: Closed (submitted)

Loading...