junit-runner: Separate out parsing specs from making list of requests

Review Request #3846 — Created May 9, 2016 and submitted

zundel
pants
zundel/add-spec-parser
3369
ece63ea...
pants-reviews
benjyw, gmalmquist, jsirois, stuhood

Essentially, a yak shave for adding support for an experimental test runner.

  • Rename classes that are ParentRunner subclasses
  • Create a class that parses and validates command line specs for tests
  • Add tests for annotation overrides
  • Add a flag to the runner to turn on an experimental test runner (for future use)

Added some new unit tests

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

  • 0
  • 0
  • 3
  • 1
  • 4
Description From Last Updated
  1. 
      
  2. Maybe rbcommons is just derping but this indentation doesn't look right

  3. Is there a reason for this snake_case or is this just an artifact of looking at too much python?

    1. Yes, I think you zeroed in on the problem on my end! I will fix it!

  4. Are you sure we want to enforce this? Will this work with Cucumber tests methods which may contain special symbols?

    1. To my knowledge, the parameters on the command line refer to classname#method name. This isn't the same as the Description that gets generated.

  5. 
      
  1. All small stuff, lgtm.
  2. These new files should have the license header.
  3. Most of the code in new files in this RB (if not all) can be package private.
  4. final for both
  5. How about s/Collection/Iterable/ and Iterables.isEmpty since this class only needs Iterables otherwise.
  6. s/if(/if (/
  7. 
      
  1. Clearly I have gotten too comfortable with our expanded unit test coverage because I never tried running this from a local jar. I committed this and then ran a jar publish but ran into lots of errors after bumping the junit-runner to the latest version with stuff like this:

     84) initializationError(org.pantsbuild.tools.jar.JarEntryCopierTest$ChecksumEntry)
                         java.lang.Exception: Test class should have exactly one public zero-argument constructor
    
     103) initializationError(org.pantsbuild.tools.jar.JarBuilderTest$WriteTest$14)
                         java.lang.Exception: The class org.pantsbuild.tools.jar.JarBuilderTest$WriteTest$14 is not public.
    
     107) initializationError(org.pantsbuild.tools.jar.JarBuilderTest$WriteTest$15)
                         java.lang.Exception: Test class should have exactly one public constructor
    
     109) initializationError(org.pantsbuild.tools.jar.JarBuilderTest$DuplicatePolicyTests)
                         java.lang.Exception: No runnable methods
    
                            at org.junit.runners.BlockJUnit4ClassRunner.validateOnlyOneConstructor(BlockJUnit4ClassRunner.java:158)
                            at org.junit.runners.BlockJUnit4ClassRunner.validateConstructor(BlockJUnit4ClassRunner.java:147)
                            at org.junit.runners.BlockJUnit4ClassRunner.collectInitializationErrors(BlockJUnit4ClassRunner.java:127)
                            at org.junit.runners.ParentRunner.validate(ParentRunner.java:416)
                            at org.junit.runners.ParentRunner.<init>(ParentRunner.java:84)
                            at org.junit.runners.BlockJUnit4ClassRunner.<init>(BlockJUnit4ClassRunner.java:65)
                            at __shaded_by_pants__.org.pantsbuild.tools.junit.withretry.BlockJUnit4ClassRunnerWithRetry.<init>(BlockJUnit4ClassRunnerWithRetry.java:26)
                            at __shaded_by_pants__.org.pantsbuild.tools.junit.withretry.JUnit4BuilderWithRetry.runnerForClass(JUnit4BuilderWithRetry.java:27)
                            at org.junit.runners.model.RunnerBuilder.safeRunnerForClass(RunnerBuilder.java:59)
                            at org.junit.internal.builders.AllDefaultPossibilitiesBuilder.runnerForClass(AllDefaultPossibilitiesBuilder.java:26)
                            at org.junit.runners.model.RunnerBuilder.safeRunnerForClass(RunnerBuilder.java:59)
                            at __shaded_by_pants__.org.pantsbuild.tools.junit.impl.AnnotatedClassRequest.getRunner(AnnotatedClassRequest.java:43)
                            at __shaded_by_pants__.org.pantsbuild.tools.junit.impl.CompositeRequestRunner.runChild(CompositeRequestRunner.java:53)
                            at __shaded_by_pants__.org.pantsbuild.tools.junit.impl.ConcurrentCompositeRequestRunner$1$1.run(ConcurrentCompositeRequestRunner.java:37)
                            at __shaded_by_pants__.org.pantsbuild.tools.junit.impl.ConcurrentRunnerScheduler.finished(ConcurrentRunnerScheduler.java:95)
                            at __shaded_by_pants__.org.pantsbuild.tools.junit.impl.ConcurrentCompositeRequestRunner$1.evaluate(ConcurrentCompositeRequestRunner.java:46)
                            ...
    
  2. 
      
  1. 
      
  2. @cheister: bug found by findbugs!

  3. All of this was simply copied in from ConsoleRunnerImpl

  4. 
      
  1. Thanks!

  2. Should include mention that this is serial (if it is) to differentiate it from ConcurrentCompositeRequestRunner

  3. src/java/org/pantsbuild/tools/junit/impl/ConsoleRunnerImpl.java (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
     
     

    IMO, placing a return at the end of each clause would be clearer than mutating a variable defined outside of either.

  4. A docstring would be good... the word Spec is crazy overloaded.

  5. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

Thanks Stu, John, Garrett. Commit 0dbaded79cbfafe4b55834b0d7d3c90b4a32e74b in master

Loading...