LUCENE-3857: exceptions from other threads in beforeclass/etc do not fail

the test (Dawid Weiss)

git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1298721 13f79535-47bb-0310-9956-ffa450edef68

lucene/CHANGES.txt

@@ -932,6 +932,9 @@ Documentation
 
 Build
 
+* LUCENE-3857: exceptions from other threads in beforeclass/etc do not fail 
+  the test (Dawid Weiss)
+
 * LUCENE-3847: LuceneTestCase will now check for modifications of System 
   properties before and after each test (and suite). If changes are detected,
   the test will fail. A rule can be used to reset system properties to

lucene/core/src/test/org/apache/lucene/util/junitcompat/TestExceptionInBeforeClassHooks.java

@@ -0,0 +1,96 @@
+package org.apache.lucene.util.junitcompat;
+
+import junit.framework.Assert;
+
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.junit.runner.JUnitCore;
+import org.junit.runner.Result;
+
+public class TestExceptionInBeforeClassHooks extends WithNestedTests {
+  public TestExceptionInBeforeClassHooks() {
+    super(true);
+  }
+
+  public static class Nested1 extends WithNestedTests.AbstractNestedTest {
+    @BeforeClass
+    public static void beforeClass() {
+      new Thread() {
+        public void run() {
+          throw new RuntimeException("foobar");
+        }
+      }.start();
+    }
+
+    public void test() {}
+  }
+
+  public static class Nested2 extends WithNestedTests.AbstractNestedTest {
+    public void test1() throws Exception {
+      Thread t = new Thread() {
+        public void run() {
+          throw new RuntimeException("foobar1");
+        }
+      };
+      t.start();
+      t.join();
+    }
+
+    public void test2() throws Exception {
+      Thread t = new Thread() {
+        public void run() {
+          throw new RuntimeException("foobar2");
+        }
+      };
+      t.start();
+      t.join();
+    }
+  }
+
+  public static class Nested3 extends WithNestedTests.AbstractNestedTest {
+    @Before
+    public void runBeforeTest() throws Exception {
+      Thread t = new Thread() {
+        public void run() {
+          throw new RuntimeException("foobar");
+        }
+      };
+      t.start();
+      t.join();
+    }
+
+    public void test1() throws Exception {
+    }
+  }
+
+  @Test
+  public void testExceptionInBeforeClassFailsTheTest() {
+    Result runClasses = JUnitCore.runClasses(Nested1.class);
+    Assert.assertEquals(1, runClasses.getFailureCount());
+    Assert.assertEquals(1, runClasses.getRunCount());
+    Assert.assertTrue(runClasses.getFailures().get(0).getTrace().contains("foobar"));
+  }
+
+  @Test
+  public void testExceptionWithinTestFailsTheTest() {
+    Result runClasses = JUnitCore.runClasses(Nested2.class);
+    Assert.assertEquals(2, runClasses.getFailureCount());
+    Assert.assertEquals(2, runClasses.getRunCount());
+    
+    String m1 = runClasses.getFailures().get(0).getTrace();
+    String m2 = runClasses.getFailures().get(1).getTrace();
+    Assert.assertTrue(
+        (m1.contains("foobar1") && m2.contains("foobar2")) ||
+        (m1.contains("foobar2") && m2.contains("foobar1")));
+  }
+  
+  @Test
+  public void testExceptionWithinBefore() {
+    Result runClasses = JUnitCore.runClasses(Nested3.class);
+    Assert.assertEquals(1, runClasses.getFailureCount());
+    Assert.assertEquals(1, runClasses.getRunCount());
+    Assert.assertTrue(runClasses.getFailures().get(0).getTrace().contains("foobar"));
+  }  
+  
+}

lucene/test-framework/src/java/org/apache/lucene/util/LuceneTestCase.java

@@ -104,6 +104,7 @@ import org.junit.internal.AssumptionViolatedException;
 import org.junit.rules.*;
 import org.junit.runner.*;
 import org.junit.runner.notification.RunListener;
+import org.junit.runners.model.MultipleFailureException;
 import org.junit.runners.model.Statement;
 
 /**
@@ -221,8 +222,6 @@ public abstract class LuceneTestCase extends Assert {
 
   private int savedBoolMaxClauseCount = BooleanQuery.getMaxClauseCount();
 
-  private volatile Thread.UncaughtExceptionHandler savedUncaughtExceptionHandler = null;
-
   /**
    * Some tests expect the directory to contain a single segment, and want to do tests on that segment's reader.
    * This is an utility method to help them.
@@ -235,17 +234,6 @@ public abstract class LuceneTestCase extends Assert {
     return (SegmentReader) subReaders[0];
   }
 
-  private static class UncaughtExceptionEntry {
-    public final Thread thread;
-    public final Throwable exception;
-
-    public UncaughtExceptionEntry(Thread thread, Throwable exception) {
-      this.thread = thread;
-      this.exception = exception;
-    }
-  }
-  private List<UncaughtExceptionEntry> uncaughtExceptions = Collections.synchronizedList(new ArrayList<UncaughtExceptionEntry>());
-
   // default codec
   private static Codec savedCodec;
   
@@ -281,11 +269,37 @@ public abstract class LuceneTestCase extends Assert {
    * Stores the currently class under test.
    */
   private static final StoreClassNameRule classNameRule = new StoreClassNameRule(); 
-  
+
+  /**
+   * Catch any uncaught exceptions on threads within the suite scope and fail the test/
+   * suite if they happen.
+   */
+  private static final UncaughtExceptionsRule uncaughtExceptionsRule = new UncaughtExceptionsRule(); 
+
+  /**
+   * This controls how suite-level rules are nested. It is important that _all_ rules declared
+   * in {@link LuceneTestCase} are executed in proper order if they depend on each 
+   * other.
+   */
   @ClassRule
   public static TestRule classRules = RuleChain
     .outerRule(new SystemPropertiesInvariantRule())
-    .around(classNameRule);
+    .around(classNameRule)
+    .around(uncaughtExceptionsRule);
+
+  /**
+   * This controls how individual test rules are nested. It is important that _all_ rules declared
+   * in {@link LuceneTestCase} are executed in proper order if they depend on each 
+   * other.
+   */
+  @Rule
+  public final TestRule ruleChain = RuleChain
+    .outerRule(new RememberThreadRule())
+    .around(new UncaughtExceptionsRule())
+    .around(new TestResultInterceptorRule())
+    .around(new SystemPropertiesInvariantRule())
+    .around(new InternalSetupTeardownRule())
+    .around(new SubclassSetupTeardownRule());
 
   @BeforeClass
   public static void beforeClassLuceneTestCaseJ4() {
@@ -443,6 +457,10 @@ public abstract class LuceneTestCase extends Assert {
     if (problem != null) {
       reportPartialFailureInfo();      
     }
+
+    if (uncaughtExceptionsRule.hasUncaughtExceptions()) {
+      testsFailed = true;
+    }
     
     // if verbose or tests failed, report some information back
     if (VERBOSE || testsFailed || problem != null) {
@@ -607,19 +625,6 @@ public abstract class LuceneTestCase extends Assert {
     }
   }
 
-  /**
-   * This controls how rules are nested. It is important that _all_ rules declared
-   * in {@link LuceneTestCase} are executed in proper order if they depend on each 
-   * other.
-   */
-  @Rule
-  public final TestRule ruleChain = RuleChain
-    .outerRule(new RememberThreadRule())
-    .around(new TestResultInterceptorRule())
-    .around(new SystemPropertiesInvariantRule())
-    .around(new InternalSetupTeardownRule())
-    .around(new SubclassSetupTeardownRule());
-
   /**
    * Internal {@link LuceneTestCase} setup before/after each test.
    */
@@ -629,15 +634,26 @@ public abstract class LuceneTestCase extends Assert {
       return new Statement() {
         @Override
         public void evaluate() throws Throwable {
-          setUpInternal();
           // We simulate the previous behavior of @Before in that
           // if any statement below us fails, we just propagate the original
           // exception and do not call tearDownInternal.
+          setUpInternal();
+          final ArrayList<Throwable> errors = new ArrayList<Throwable>();
+          try {
+            // But we will collect errors from statements below and wrap them
+            // into a multiple so that tearDownInternal is called.
+            base.evaluate();
+          } catch (Throwable t) {
+            errors.add(t);
+          }
+          
+          try {
+            tearDownInternal();
+          } catch (Throwable t) {
+            errors.add(t);
+          }
 
-          // TODO: [DW] should this really be this way? We could use
-          // JUnit's MultipleFailureException and propagate both?
-          base.evaluate();
-          tearDownInternal();
+          MultipleFailureException.assertEmpty(errors);
         }
       };
     }
@@ -653,36 +669,6 @@ public abstract class LuceneTestCase extends Assert {
     Thread.currentThread().setName("LTC-main#seed=" + 
         new ThreeLongs(staticSeed, seed, LuceneTestCaseRunner.runnerSeed));
 
-    savedUncaughtExceptionHandler = Thread.getDefaultUncaughtExceptionHandler();
-    Thread.setDefaultUncaughtExceptionHandler(new Thread.UncaughtExceptionHandler() {
-      public void uncaughtException(Thread t, Throwable e) {
-        // org.junit.internal.AssumptionViolatedException in older releases
-        // org.junit.Assume.AssumptionViolatedException in recent ones
-        if (e.getClass().getName().endsWith("AssumptionViolatedException")) {
-          String where = "<unknown>";
-          for (StackTraceElement elem : e.getStackTrace()) {
-            if ( ! elem.getClassName().startsWith("org.junit")) {
-              where = elem.toString();
-              break;
-            }
-          }
-          System.err.print("NOTE: Assume failed at " + where + " (ignored):");
-          if (VERBOSE) {
-            System.err.println();
-            e.printStackTrace(System.err);
-          } else {
-            System.err.print(" ");
-            System.err.println(e.getMessage());
-          }
-        } else {
-          testsFailed = true;
-          uncaughtExceptions.add(new UncaughtExceptionEntry(t, e));
-          if (savedUncaughtExceptionHandler != null)
-            savedUncaughtExceptionHandler.uncaughtException(t, e);
-        }
-      }
-    });
-
     savedBoolMaxClauseCount = BooleanQuery.getMaxClauseCount();
 
     if (useNoMemoryExpensiveCodec) {
@@ -781,15 +767,7 @@ public abstract class LuceneTestCase extends Assert {
     // this won't throw any exceptions or fail the test
     // if we change this, then change this logic
     checkRogueThreadsAfter();
-    // restore the default uncaught exception handler
-    Thread.setDefaultUncaughtExceptionHandler(savedUncaughtExceptionHandler);
-    
-    try {
-      checkUncaughtExceptionsAfter();
-    } catch (Throwable t) {
-      if (problem == null) problem = t;
-    }
-    
+
     try {
       // calling assertSaneFieldCaches here isn't as useful as having test
       // classes call it directly from the scope where the index readers
@@ -816,7 +794,7 @@ public abstract class LuceneTestCase extends Assert {
       throw new RuntimeException(problem);
     }
   }
-  
+
   /** check if the test still has threads running, we don't want them to 
    *  fail in a subsequent test and pass the blame to the wrong test */
   private void checkRogueThreadsAfter() {
@@ -825,26 +803,10 @@ public abstract class LuceneTestCase extends Assert {
       if (!testsFailed && rogueThreads > 0) {
         System.err.println("RESOURCE LEAK: test method: '" + getName()
             + "' left " + rogueThreads + " thread(s) running");
-        // TODO: fail, but print seed for now
-        if (uncaughtExceptions.isEmpty()) {
-          reportAdditionalFailureInfo();
-        }
       }
     }
   }
   
-  /** see if any other threads threw uncaught exceptions, and fail the test if so */
-  private void checkUncaughtExceptionsAfter() {
-    if (!uncaughtExceptions.isEmpty()) {
-      System.err.println("The following exceptions were thrown by threads:");
-      for (UncaughtExceptionEntry entry : uncaughtExceptions) {
-        System.err.println("*** Thread: " + entry.thread.getName() + " ***");
-        entry.exception.printStackTrace(System.err);
-      }
-      fail("Some threads threw uncaught exceptions!");
-    }
-  }
-
   private final static int THREAD_STOP_GRACE_MSEC = 10;
   // jvm-wide list of 'rogue threads' we found, so they only get reported once.
   private final static IdentityHashMap<Thread,Boolean> rogueThreads = new IdentityHashMap<Thread,Boolean>();

lucene/test-framework/src/java/org/apache/lucene/util/UncaughtExceptionsRule.java

@@ -0,0 +1,115 @@
+package org.apache.lucene.util;
+
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.lang.Thread.UncaughtExceptionHandler;
+import java.util.ArrayList;
+import java.util.List;
+
+import org.junit.rules.TestRule;
+import org.junit.runner.Description;
+import org.junit.runners.model.MultipleFailureException;
+import org.junit.runners.model.Statement;
+
+/**
+ * Subscribes to
+ * {@link Thread#setDefaultUncaughtExceptionHandler(java.lang.Thread.UncaughtExceptionHandler)}
+ * and causes test/ suite failures if uncaught exceptions are detected.
+ */
+public class UncaughtExceptionsRule implements TestRule {
+  // This was originally volatile, but I don't think it needs to be. It's the same
+  // thread accessing it, always.
+  private UncaughtExceptionHandler savedUncaughtExceptionHandler;
+
+  public static class UncaughtExceptionEntry {
+    public final Thread thread;
+    public final Throwable exception;
+
+    public UncaughtExceptionEntry(Thread thread, Throwable exception) {
+      this.thread = thread;
+      this.exception = exception;
+    }
+  }
+
+  @SuppressWarnings("serial")
+  private static class UncaughtExceptionsInBackgroundThread extends RuntimeException {
+    public UncaughtExceptionsInBackgroundThread(UncaughtExceptionEntry e) {
+      super("Uncaught exception by thread: " + e.thread, e.exception);
+    }
+  }
+
+  // Lock on uncaughtExceptions to access.
+  private final List<UncaughtExceptionEntry> uncaughtExceptions = new ArrayList<UncaughtExceptionEntry>();
+
+  @Override
+  public Statement apply(final Statement s, final Description d) {
+    return new Statement() {
+      public void evaluate() throws Throwable {
+        final ArrayList<Throwable> errors = new ArrayList<Throwable>();
+        try {
+          setupHandler();
+          s.evaluate();
+        } catch (Throwable t) {
+          errors.add(t);
+        } finally {
+          restoreHandler();
+        }
+
+        synchronized (uncaughtExceptions) {
+          for (UncaughtExceptionEntry e : uncaughtExceptions) {
+            errors.add(new UncaughtExceptionsInBackgroundThread(e));
+          }
+          uncaughtExceptions.clear();
+        }
+
+        MultipleFailureException.assertEmpty(errors);
+      }
+    };
+  }
+ 
+  /**
+   * Just a check if anything's been caught.
+   */
+  public boolean hasUncaughtExceptions() {
+    synchronized (uncaughtExceptions) {
+      return !uncaughtExceptions.isEmpty();
+    }
+  }
+  
+  private void restoreHandler() {
+    Thread.setDefaultUncaughtExceptionHandler(savedUncaughtExceptionHandler);    
+  }
+
+  private void setupHandler() {
+    savedUncaughtExceptionHandler = Thread.getDefaultUncaughtExceptionHandler();
+    Thread.setDefaultUncaughtExceptionHandler(new Thread.UncaughtExceptionHandler() {
+      public void uncaughtException(Thread t, Throwable e) {
+        // org.junit.internal.AssumptionViolatedException in older releases
+        // org.junit.Assume.AssumptionViolatedException in recent ones
+        if (e.getClass().getName().endsWith("AssumptionViolatedException")) {
+          String where = "<unknown>";
+          for (StackTraceElement elem : e.getStackTrace()) {
+            if (!elem.getClassName().startsWith("org.junit")) {
+              where = elem.toString();
+              break;
+            }
+          }
+          System.err.print("NOTE: Uncaught exception handler caught a failed assumption at " 
+              + where + " (ignored):");
+        } else {
+          synchronized (uncaughtExceptions) {
+            uncaughtExceptions.add(new UncaughtExceptionEntry(t, e));
+          }
+
+          StringWriter sw = new StringWriter();
+          sw.write("\n===>\nUncaught exception by thread: " + t + "\n");
+          PrintWriter pw = new PrintWriter(sw);
+          e.printStackTrace(pw);
+          pw.flush();
+          sw.write("<===\n");
+          System.err.println(sw.toString());
+        }
+      }
+    });
+  }  
+}