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

JS
  1. NB: I took advantage of the fact we now officially support JDK 8 as a minimum.
  2. 
      
WI
  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. 
      
JS
JS
JS
ZU
  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. 
      
JS
JS
ZU
  1. Ship It!
  2. 
      
ST
  1. Thanks!

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

    1. It doesn't - killed
  3. 
      
JS
JS
JS
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...