Update junit-runner to version 1.0.9 and test new experimental runner logic

Review Request #3925 — Created May 23, 2016 and submitted

benjyw, gmalmquist, stuhood
  • Bumps junit-runner to version 1.0.9 to include new experimental runner logic
  • Bump removal version of --default-parallel to 1.3.0 (since this never made it into 1.0.0 and the deprecation cycle is to last 2 minor versions)
  • Don't pass -default-parallel down to the runner any more
  • Adds a test to insure that the deprecated --default-parallel option still works.
  • The old "ParallelMethods" Junit tests were really testing the "ParallelClassesAndMethods" logic. Renamed them.
  • Created new "ParallelMethods" Junit tests both in testprojects and the Junit runner mock tests.
  • Updated the concurrency integration test to run both with the legacy and experimental runner.
  • Added caveat about using test sharding with the PARALLEL_METHODS
  • Cleanup comments that referenced the old -parallel-methods and deprecated -default-parallel option

CI green at https://travis-ci.org/pantsbuild/pants/builds/134707560

  • 0
  • 0
  • 2
  • 0
  • 2
Description From Last Updated
  1. Hold off on reviewing, looks like another change I had in my branch hasn't made it in yet (removing -parallel-methods from java_tests.py)

  2. This seems odd because it looks like there should be a JunitTestsConcurrencyIntegrationTest.USE_EXPERIMENTAL_RUNNER = False at the outset, but I guess that's taken care of by the setup method.

    1. Yes. I had it there initially but I removed it because every test needs it set to False to start off with, but not every test is decorated with @ensure_experimental I'll add it back just to make things crystal clear.

  3. This looks weird. What's args[0]?

    maybe use:

    def run_pants_with_workdir(self, variable, *args, **kwargs)?

    1. I guess that was a little obscure. The first argument to the run_pants_with_workdir() method is a list of options to pass to pants on the command line.
      But I don't have to pass them through this way, I just listed out the args.

  2. I think this should just be command.append now? Since args would previously have been referring to (command, workdir).

    1. Thanks Garrett, it turns out that this logic was bypassed when I added setUp() to the class. Apparently my decorator wrapper function ran before setUp() causing USE_EXPERIMENTAL_RUNNER to always be False.

      I moved the tests that don't use @ensure_experimental into a separate test class, then temporarily added a raise to make sure my tests were being called with the right arguments.

  1. Thanks!

    Is it definitely too late to rename the BOTH option? When we add the next concurrency mode, BOTH will no longer make sense.

    1. No, I don't consider it too late, what would you like, PARALLEL_CLASSES_METHODS ?


    3. Yea, sounds good.

  2. This seems like a duplicate of the test in testprojects. Is it useful to test it at both levels, or is testing it at this level sufficient?

    1. The testprojects is useful for testing the end-to-end integration of plumbing all the options through pants code. These allow you to test junit runner standalone without pushing a new jar to nexus. The logic isn't trivial - you have to consider the command line flags, legacy options, and overrides in the BUILD files.


    3. Landed in https://rbcommons.com/s/twitter/r/3962/

Review request changed

Status: Closed (submitted)

Change Summary:

Thanks Stu and Garrett. Commit 5bd0053

  1. I've landed this to help get it into the next 1.1.0-pre release branch before I headed out on vacation this week. If there are any problems, please revert it for me in my abscence.