Remove unneeded args4j handler registrations that cause failures in tests and rename TestParser

Review Request #3583 — Created March 18, 2016 and submitted

cheister
pants
remove-args4j-registerhandler-calls
3059
pants-reviews
zundel
Remove unneeded args4j handler registrations that cause failures in tests and rename TestParser

Travis CI: https://travis-ci.org/pantsbuild/pants/builds/116981417

This fixes the problem seen in https://rbcommons.com/s/twitter/r/3571/ and https://rbcommons.com/s/twitter/r/2406/ and recorded in https://github.com/pantsbuild/pants/issues/1727

It's hard to reproduce the problem in an isolated test but in https://rbcommons.com/s/twitter/r/3571/ it will always happen if you change the order of arguments in ConsoleRunnerTestHelper#runTestAndReturnXmlFile and move the -xmlreport flag before the -outdir flag then run ./pants test tests/java::

It seems to be a problem when the Parser runs OptionHandlerRegistry.getRegistry().registerHandler(boolean.class, BooleanOptionHandler.class); in the ParserTest code and then any ConsoleRunnerTests are run with the -outdir change above. The error is

1) testXmlReportAllPassing(org.pantsbuild.tools.junit.impl.ConsoleRunnerTest)
                     java.lang.RuntimeException: Classloading error during test discovery for /var/folders/cc/7wn2nwjn1gd4cd08fpp3zb6r00015s/T/junit1339494313910125036/testOutputDir
                        at org.pantsbuild.tools.junit.impl.ConsoleRunnerImpl.notFoundError(ConsoleRunnerImpl.java:613)
                        at org.pantsbuild.tools.junit.impl.ConsoleRunnerImpl.parseRequests(ConsoleRunnerImpl.java:500)
                        at org.pantsbuild.tools.junit.impl.ConsoleRunnerImpl.run(ConsoleRunnerImpl.java:372)
                        at org.pantsbuild.tools.junit.impl.ConsoleRunnerImpl.main(ConsoleRunnerImpl.java:753)
                        at org.pantsbuild.tools.junit.impl.ConsoleRunnerTestHelper.runTestAndReturnXmlFile(ConsoleRunnerTestHelper.java:71)
                        at org.pantsbuild.tools.junit.impl.ConsoleRunnerTestHelper.runTestAndParseXml(ConsoleRunnerTestHelper.java:79)
                        at org.pantsbuild.tools.junit.impl.ConsoleRunnerTest.testXmlReportAllPassing(ConsoleRunnerTest.java:194)
                        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
                        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
                        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
                        at java.lang.reflect.Method.invoke(Method.java:498)
                        at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
                        at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
                        at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
                        at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
                        at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
                        at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
                        at org.junit.rules.ExternalResource$1.evaluate(ExternalResource.java:48)
                        at org.junit.rules.RunRules.evaluate(RunRules.java:20)
                        at __shaded_by_pants__.org.pantsbuild.tools.junit.withretry.BlockJUnit4ClassRunnerWithRetry$InvokeWithRetry.evaluate(BlockJUnit4ClassRunnerWithRetry.java:57)
                        at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
                        at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
                        at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
                        at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
                        at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
                        at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
                        at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
                        at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
                        at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
                        at __shaded_by_pants__.org.pantsbuild.tools.junit.impl.CompositeRequest.runChild(CompositeRequest.java:66)
                        at __shaded_by_pants__.org.pantsbuild.tools.junit.impl.ConcurrentCompositeRequest$1$1.run(ConcurrentCompositeRequest.java:36)
                        at __shaded_by_pants__.org.pantsbuild.tools.junit.impl.ConcurrentRunnerScheduler.finished(ConcurrentRunnerScheduler.java:85)
                        at __shaded_by_pants__.org.pantsbuild.tools.junit.impl.ConcurrentCompositeRequest$1.evaluate(ConcurrentCompositeRequest.java:45)
                        at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
                        at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
                        at __shaded_by_pants__.org.pantsbuild.tools.junit.impl.ConsoleRunnerImpl.run(ConsoleRunnerImpl.java:420)
                        at __shaded_by_pants__.org.pantsbuild.tools.junit.impl.ConsoleRunnerImpl.main(ConsoleRunnerImpl.java:753)
                        at org.pantsbuild.tools.junit.ConsoleRunner.main(ConsoleRunner.java:12)

Which seems to indicate that the ConsoleRunnerImpl parser is not pulling the -outdir directory off of the command line and is instead trying to interpret it as a test class with tests. When ConsoleRunnerImpl tries to load the file to look for test methods it fails with the error above.

Both ParserTest and ConsoleRunnerTest pass when run in isolation so it seems to indicate that some global state is not getting cleaned up between the tests. The OptionHandlerRegistry.getRegistry() is global singleton in args4j and it looks like leaving the pants version BooleanOptionHandler registered will mess up any other tests that run after the ParserTest and do not want the different functionality.

  1. FWIW, there were patches to OptionRegistrationHandler in args4j 2.33. I updated the version and it had no impact on the bug.

  2. 
      
  1. I think I see now what the root of the problem is. Some of our tests use the pansbuild Parser() class which updates the singleton in OptionHandlerRegistry

    However, our junit runner does not use the pantsbuild Parser() class. It calls args4j's native CmdLineParser directly.

    The regular CmdLineParser doesn't expect our boolean override (which behaves differently with Possibly this state is sticking around and causing the argument parsing to fail when you run all the tests together. I looked around in OptionHandlerRegistry and there is no way to clear or reset this singleton without a patch to args4j.

    I tried re-implementing junit runner using Parser() and there are functionality differences. Currently, to pass -test-shard we use

    -test-shard 0/2
    

    That doesn't work out of the box if you just replace if you change it over to Parser, you have to use:

    -test-shard=0/2
    

    This is because Parser.parse() builds up ParserProperties and tacks on:

                .withOptionValueDelimiter("=")
    

    Since junit runner is an internal tool intended only for pants, it is possible that we could use Parser() consistently in all of our command line tools and just change all references, but it would be a breaking change meaning that you couldn't back down to a previous version of junit runner.

  2. If we remove this, we lose an ability:

    -xmlreport
    

    will work

    -xmlreport=true
    -xmlreport=false
    

    will not work. the behavior changes to

    -xmlreport true
    -xmlreport false
    

    (Which I think is silly by the way)

    I don't think we depend on this behavior in any of these tools that use Parser (jar tool). If that is case we should just remove our local BooleanOptionHandler implementation. But this is probably not the only side effect, there is something fundamentally broken in args4j when you are trying to test multiple parser implmenetations in the same classloader (the singleton design)

    1. Wow, I totally glossed over the fact that the BooleanOptionHandler.class was a custom class in the Pants build and not the args4j BooleanOptionHandler. I updated the PR to test the custom functionality so tests will fail if we accidentally remove it in the future.

      I also have an alternative fix of just reseting the BooleanOptionHandler to the args4j value at the end of the Parser.parse method. This should really only affect tests since I can't image two different methods parsing the command line in normal code, and it seems like an easy place to keep the registering and unregistering of the handler in one place.

  3. 
      
  1. Ship It!
  2. 
      
  1. One more thing, I think there might be some tests we can remove @Ignore from now.

    ./tools/junit/runner/ConsoleRunnerConsoleOutputTest.java:  @Ignore("https://github.com/pantsbuild/pants/issues/1720")
    ./tools/junit/runner/ConsoleRunnerConsoleOutputTest.java:  @Ignore("https://github.com/pantsbuild/pants/issues/1720")
    
    1. Yeah, I might look at doing that in a separate PR.

  2. 
      
  1. 
      
  2. 
      
  1. Thanks for the patch Chris and for the review Stu. In master @ b128436

  2. 
      
Review request changed

Status: Closed (submitted)

Loading...