Fix JUnit -fail-fast, add test for early exit hook and remove unused code

Review Request #4081 — Created July 14, 2016 and submitted

cheister
pants
junit-fail-fast-et-al
3672
pants-reviews
stuhood, zundel

Replace AbortableListner with ShutdownListener because the AbortableListener did not work with -fail-fast but we still want to keep the functionality of displaying a test summary if the tests are shutdown unexpectedly and add an integration test for the ShutdownListener.

Remove ForwardingListener and ListenerRegistry because they were only being used by the AbortableListener but also had a bug when one listener threw an exception none of the rest of the listeners would get executed. An example of this is what caused the need for https://rbcommons.com/s/twitter/r/4060/

Instead of the fix from https://rbcommons.com/s/twitter/r/4060/, move the testRunFinishFailed test to JUnitConsoleImplTest.java where we can more easily test the output and fix the problem in the ConsoleListener instead.

Add a FailFastListener and FailFastRunner so -fail-fast returns a better test summary and add a test for it.

Remove exitStatus from ConsoleRunnerImpl because it is unused and fix some typos and deprecation warnings in related JUnitConsole classes.

Move the output mode tests from integration tests to the ConsoleRunnerImpl because they can be unit tested instead of using the full integration setup.

Add test for -per-test-timer

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

ST
  1. @Chris: Would you mind expanding the review description? There is a bit too much going on here to follow without some explanation.

    1. Sure. The main changes are
      1) Replacing AbortableListner with ShutdownListener because the AbortableListener did not work with -fail-fast but we still want to keep the functionality of displaying a test summary if the tests are shutdown unexpectedly and add an integration test for the ShutdownListener.
      2) Remove ForwardingListener and ListenerRegistry because they were only being used by the AbortableListener but also had a bug when one listener threw an exception none of the rest of the listeners would get executed. An example of this is what caused the need for https://rbcommons.com/s/twitter/r/4060/
      3) Instead of the quick fix from https://rbcommons.com/s/twitter/r/4060/, move the testRunFinishFailed test to JUnitConsoleImplTest.java where we can more easily test the output and fix the problem in the ConsoleListener instead.
      4) Add a FailFastListener and FailFastRunner so -fail-fast works with serially run tests and add a test for it.
      5) Remove exitStatus from ConsoleRunnerImpl because it is unused and fix some typos and deprecation warnings in related JUnitConsole classes.
      6) Finally move the output mode tests from integration tests to the ConsoleRunnerImpl because they can be unit tested instead of using the full integration setup.

    2. sounds good to me. I think that @stu was asking you to put this in the Description field of the RB commons entry so that it would make it into the changelog

    3. Yep, please do that.

  2. FWIW, as mentioned on pants-devel@, we don't actually use fail-fast, and would be fine with deprecating it for removal.

    1. Given the extra complexity, I would probably even prefer removing it.

    2. I can remove it from the PR? Or we could just say it will only work with tests run serially? It now works and is tested for that case.

    3. I think we'd need to run it through a deprecation cycle before removing it. For the serial question, is it the case that it sort of worked before in the concurrent case, or was it just broken? If it was just broken, I think requiring serial execution is fine, but we should make sure the serial requirement is effectively communicated to the user.

    4. Overall, this looks like a simplification, if anything. I haven't used the feature, but I can think of times when I probably would have like to have it (like testing a broken test suite that is very long and failing somewhere in the middle)

    5. Looks like using the FailFastRunner with multiple threads and the runLegacy works fine. I should have tested it sooner. I updated the code and added a test for it.

  3. 
      
ZU
  1. 
      
  2. I like this, but I see that ConsoleListener is subclassed. Are we going to print out the failure multiple times?

    1. It's subclassed by PerTestConosoleListener. It will only get displayed multiple times if you add multiple ConsoleListeners. I'm not sure why you would want to do that, but you would get everything printed to the console multiple times if you did.

      Currently we only install one ConsoleListener at a time

          if (perTestTimer) {
            core.addListener(new PerTestConsoleListener(swappableOut.getOriginal()));
          } else {
            core.addListener(new ConsoleListener(swappableOut.getOriginal()));
          }
      
  3. I don't see where this gets installed?

    1. The FailFastListener is only used by the FailFastRunner which gets installed in runLegacy when parallelThreads is 0.

          for (Request request : requests) {
            if (failFast) {
              result = core.run(new FailFastRunner(request.getRunner()));
            } else {
              result = core.run(request);
            }
            failures += result.getFailureCount();
          }
      
  4. 
      
ZU
  1. Ship It!
  2. 
      
CH
CH
ZU
  1. 
      
  2. Square has -per-test-timer and -output-mode settings configured to non-default values in pants.ini, its great to have proper tests for them now.

  3. I'm glad you were able to add a test that actually checks the output for this case.

  4. Great to hava new test for this condition too. In your test it calls System.exit(0), this should make sure that we actually exit with a non-zero exit value.

  5. 
      
ST
  1. Thanks Chris.

    Please move the description of what you did in this change out of the "Testing Done" section of the review, and into the "Description" section.

  2. Is this accessed from multiple threads? Does it need to be marked volatile?

    1. If the RunListener is not marked as @RunListener.ThreadSafe then JUnit will automatically wrap the RunListener in a SynchronizedRunListener that will synchronize all the methods.

  3. Please add javadocs to the new runners/listeners.

    Also, I wonder whether collecting the various runner/listener pairs together in parent static classes might clean things up? IE, FailFast.Listener, FailFast.Runner

    1. Yeah. I was thinking we should move all of the RunListeners out of ConsoleRunnerImpl and into their own files. e.g. we do this for the ShutdownListener but not for StreamCapturingListener.

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

    Factory functions on the static parent classes I suggested earlier?

    Alternatively, will reiterate: I don't think there are any consumers of -fail-fast. If you want to just remove it, that would be fine too.

  5. 
      
CH
CH
ST
  1. Ship It!
  2. 
      
CH
Review request changed

Status: Closed (submitted)

Change Summary:

Merged as f1c708e0dc4b9780fbcc34d18611247e20f7c55d

Loading...