Fix handling of method specs.

Review Request #4258 — Created Sept. 23, 2016 and submitted

jsirois
pants
jsirois/issues/3837/fix_missing_test_handling
3902
0068ff7...
pants-reviews
stuhood, wisechengyi, zundel
Previously the method spec handling code did not guard against non-test
classes and would throw an NPE. This change adds a failing test and
fixes the handling to deal with `Optional`. The `Spec` class is made
immutable in the process to improve the code's ability to be reasoned
about.

 3rdparty/jvm/org/hamcrest/BUILD                                  | 17 +++++++++
 src/java/org/pantsbuild/tools/junit/impl/ConsoleRunnerImpl.java  | 15 +++-----
 src/java/org/pantsbuild/tools/junit/impl/Spec.java               | 40 +++++++++++++---------
 src/java/org/pantsbuild/tools/junit/impl/SpecException.java      |  8 ++---
 src/java/org/pantsbuild/tools/junit/impl/SpecParser.java         | 93 ++++++++++++++++++++++----------------------------
 testprojects/tests/java/org/pantsbuild/testproject/matcher/BUILD |  1 +
 tests/java/org/pantsbuild/tools/junit/impl/BUILD                 |  7 ++++
 tests/java/org/pantsbuild/tools/junit/impl/SpecParserTest.java   | 55 +++++++++++++++--------------
 tests/java/org/pantsbuild/tools/junit/impl/SpecTest.java         | 13 +++----
 tests/java/org/pantsbuild/tools/junit/lib/BUILD                  |  2 +-
 10 files changed, 135 insertions(+), 116 deletions(-)

I noticed this issue when attempting the following:

./pants test.junit \
  --no-timeouts \
  --open \
  --coverage-open \
  --test=tests/java/org/pantsbuild/args4j/ParserTest.java#test{Failure,Success} \
  tests/java/org/pantsbuild/args4j/::
...
11:26:39 00:03       [run]
                     OpenJDK 64-Bit Server VM warning: ignoring option MaxPermSize=256m; support was removed in 8.0
                     OpenJDK 64-Bit Server VM warning: ignoring option UseSplitVerifier; support was removed in 8.0
                     Auto-detected 8 processors, using -parallel-threads=8
                     Exception in thread "main" java.lang.NullPointerException
                        at __shaded_by_pants__.org.pantsbuild.tools.junit.impl.SpecParser.addMethod(SpecParser.java:130)
                        at __shaded_by_pants__.org.pantsbuild.tools.junit.impl.SpecParser.parse(SpecParser.java:61)
                        at __shaded_by_pants__.org.pantsbuild.tools.junit.impl.ConsoleRunnerImpl.run(ConsoleRunnerImpl.java:455)
                        at __shaded_by_pants__.org.pantsbuild.tools.junit.impl.ConsoleRunnerImpl.main(ConsoleRunnerImpl.java:845)
                        at org.pantsbuild.tools.junit.ConsoleRunner.main(ConsoleRunner.java:12)

                   Test failed: 
                   java org.pantsbuild.tools.junit.ConsoleRunner ... exited non-zero (1); 0 failed 0 targets.
...

Locally green using ./pants test-changed.

CI went green here:
https://travis-ci.org/pantsbuild/pants/builds/162902557

  1. NB: I took advantage of the fact we now officially support JDK 8 as a minimum.
  2. 
      
  1. 
      
  2. Reversing the order of specs.asMap().containsKey(s.getSpecClass()) && !s.getMethods().isEmpty() may lead to less computation.

    1. The LoadingCache was not maintaining insertion (creation) order which is relied upon in other tests (consistent sharding needs it). I switched to a straight-up LHM since java8 supports convenient getOrCreate via computeIfAbsent so I think this one is now moot.

  3. 
      
  1. 
      
  2. I'm not sure I agree with this refactoring. You are changing the meaning of this method and reducing its utility to only being able to add a single method to the ones passed in. Why is this important? If we want to make this an immutable object then could we just get rid of addMethod()? I think this is confusing.

    1. There is exactly 1 use, so the utility is the same afaict.  The meaning has changed, but so did the docs - perhaps the name needs to change - its just a factory.  Perhaps `withMethod`?  Without a factory method that adds a test method, the private constructor would need to be made public and this would further require a redundant passing in of the same test class and an awkward construction of the new list of methods - the factory method does both now.
    2. I missed answering the why - I was uncomfortable with mutating map values.  Certainly fine to do, but the `#method` case is a small one and not perf sensitive at all, so having the clarity of immutability struck me as a win.  That said, I always favor immutability in java unless there are pressing reasons not to and this may not be a shared bias.  I could certainly revert this portion of the change.
    3. I agree with the immutability. For now, would you mind just changing the name? Maybe what would be better here is to change to use builder pattern. Also, I was checking out RBCommons' new email change and it seems to be working! I switched my rbcommons email back to my work account.

    4. Name changed - I think it reads more to expectations now.
  3. 
      
  1. Ship It!
  2. 
      
  1. Thanks!

  2. Now that you've filled in the deps, does this still need strict_deps=False?

    1. It doesn't - killed
  3. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

Now on master:

git log -1 origin/master
commit f46e0702f4409d982b79cab9a3789b35090d46a4
Author: John Sirois <john.sirois@gmail.com>
Date:   Mon Sep 26 20:01:03 2016 -0600

    Fix handling of method specs.
    
    Previously the method spec handling code did not guard against non-test
    classes and would throw an NPE. This change adds a failing test and
    fixes the handling to deal with `Optional`. The `Spec` class is made
    immutable in the process to improve the code's ability to be reasoned
    about.
    
    Testing Done:
    I noticed this issue when attempting the following:
    ```
    ./pants test.junit \
      --no-timeouts \
      --open \
      --coverage-open \
      --test=tests/java/org/pantsbuild/args4j/ParserTest.java#test{Failure,Success} \
      tests/java/org/pantsbuild/args4j/::
    ...
    11:26:39 00:03       [run]
                         OpenJDK 64-Bit Server VM warning: ignoring option MaxPermSize=256m; support was removed in 8.0
                         OpenJDK 64-Bit Server VM warning: ignoring option UseSplitVerifier; support was removed in 8.0
                         Auto-detected 8 processors, using -parallel-threads=8
                         Exception in thread "main" java.lang.NullPointerException
                            at __shaded_by_pants__.org.pantsbuild.tools.junit.impl.SpecParser.addMethod(SpecParser.java:130)
                            at __shaded_by_pants__.org.pantsbuild.tools.junit.impl.SpecParser.parse(SpecParser.java:61)
                            at __shaded_by_pants__.org.pantsbuild.tools.junit.impl.ConsoleRunnerImpl.run(ConsoleRunnerImpl.java:455)
                            at __shaded_by_pants__.org.pantsbuild.tools.junit.impl.ConsoleRunnerImpl.main(ConsoleRunnerImpl.java:845)
                            at org.pantsbuild.tools.junit.ConsoleRunner.main(ConsoleRunner.java:12)
    
                       Test failed:
                       java org.pantsbuild.tools.junit.ConsoleRunner ... exited non-zero (1); 0 failed 0 targets.
    ...
    ```
    
    Locally green using `./pants test-changed`.
    
    CI went green here:
      https://travis-ci.org/pantsbuild/pants/builds/162902557
    
    Bugs closed: 3902
    
    Reviewed at https://rbcommons.com/s/twitter/r/4258/
Loading...