Enhance parallel testing junit_tests

Review Request #3707 — Created April 16, 2016 and submitted

zundel
pants
zundel/respin-parallel-methods-pants
3191, 3210, 3262
3753
c400c81...
pants-reviews
benjyw, gmalmquist, jsirois, stuhood

Surfaces EXPERIMENTAL support for parallel testing of methods within a test class.
- Adds a 'concurrency' parameter to set test concurrency to 'SERIAL' 'PARALLEL_CLASSES', 'PARALLEL_METHODS', or 'PARALLEL_BOTH'
- Adds a 'threads' parameter to junit_tests to control concurrency.
- Adds a --test-junit-default-concurrency option
- PARALLEL_BOTH will allow both classes and methods to run in parallel for the entire test run
- PARALLEL_METHODS (just run methods within a class in parallel) is flagged as not implemented
- Added a number of unit test cases and integration tests to exercise these features

Note that there is a bug in that the @TestSerial annotation is not respected when the -parallel-methods flag is in use. See https://github.com/pantsbuild/pants/issues/3209

Followup work for this change is to add an @TestParallelMethods annotation and come up with a more rational way to
pass concurrency options to the junit-runner backend.

Integration test and additional unit tests added.
CI is green at https://travis-ci.org/pantsbuild/pants/builds/125429738

  • 0
  • 0
  • 5
  • 0
  • 5
Description From Last Updated
ZU
  1. This looks like a huge change but it really shouldn't be too bad - there's a bunch of testing I added to backfill coverage for the different JUnit concurrency settings that bulked up the PR.

  2. 
      
BE
  1. 
      
  2. type: jvm_optoins

  3. Nit: period.

  4. I think we prefer parens to line continuation.

  5. Nit: .

  6. This is all quite confusing. For uniformity, could we have a single -concurrency flag to the junit runner, and a single --concurrency option to this task? Or is the impedance mismatch necessary?

    1. I think you are right. Unfortunately, fixing this in junit-runner would require another publishing cycle. I need to get in there and do some other work to make this feature work correctly:

      • Add a @TestParallelMethods annotation
      • Fix the bug in https://github.com/pantsbuild/pants/issues/3209

      Fixing that bug is going to require some more extensive refactoring in ConsoleRunnerImpl. I would like to get these tests in first before hacking that up.

      What I'm thinking of is that Right now it parses the command line arguments and makes 'Request' objects all at the same time. I think I need to split some of this apart:

      • Make parsing specs separate from building JUnit requests
      • Don't wait until you get to the scheduling phase to look at the @Test* annotations.
      • Tag tests with a concurrency override before building the request object
  7. 
      
ZU
BE
  1. Ship It!
  2. 
      
ZU
PS
  1. Ship It!
  2. 
      
WE
  1. This change should not affect Twitter CI's Pants execution. The option we are using is "--test-junit-parallel-threads=8", which is not affected by this change.

  2. 
      
ZU
ZU
ZU
ZU
  1. 
      
  2. Made these all uppercase and made them public fields so we could share them with the --default-concurrency option.

  3. Taking a look at the maven surefire test runner, it gives an option to parallelize running classes, the methods within a class, or both. We don't have anything like the 'just parallelize the methods within a class' option in our runner yet.

  4. 
      
ZU
ZU
GM
  1. LGTM; just some nits.

  2. nit: weird indentation

  3. Possibly, if any tests do things like System.getProperty()

  4. nit: the '+' sign is extraneous here

  5. maybe elifs here?

  6. 
      
CH
  1. Ship It!
  2. 
      
ZU
ZU
Review request changed

Status: Closed (submitted)

Change Summary:

Thanks for the reviews Benjy, Wei, Garrett, and Chris. In master @ d5c18bd

Loading...