[#1849] Exceptions from controllers are not detected in FunctionalTests

framework/src/play/test/FunctionalTest.java

@@ -31,12 +31,16 @@
 import play.Invoker;
 import play.Invoker.InvocationContext;
 import play.classloading.enhancers.ControllersEnhancer.ControllerInstrumentation;
+import play.exceptions.JavaExecutionException;
+import play.exceptions.UnexpectedException;
 import play.libs.F.Action;
 import play.mvc.ActionInvoker;
 import play.mvc.Controller;
 import play.mvc.Http;
 import play.mvc.Http.Request;
 import play.mvc.Http.Response;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Future;
 import play.mvc.Router.ActionDefinition;
 import play.mvc.Scope.RenderArgs;
 
@@ -65,7 +69,7 @@ public static Response GET(Object url) {
 
     /**
      * sends a GET request to the application under tests.
-     * 
+     *
      * @param url
      *            relative url such as <em>"/products/1234"</em>
      * @param followRedirect
@@ -94,7 +98,7 @@ public static Response GET(Object url, boolean followRedirect) {
 
     /**
      * Sends a GET request to the application under tests.
-     * 
+     *
      * @param request
      *            The given request
      * @param url
@@ -144,7 +148,7 @@ public static Response POST(Object url, String contenttype, InputStream body) {
 
     /**
      * Sends a POST request to the application under tests.
-     * 
+     *
      * @param request
      *            The given request
      * @param url
@@ -178,7 +182,7 @@ public static Response POST(Request request, Object url, String contenttype, Inp
 
     /**
      * Sends a POST request to the application under tests as a multipart form. Designed for file upload testing.
-     * 
+     *
      * @param url
      *            relative url such as <em>"/products/1234"</em>
      * @param parameters
@@ -243,7 +247,7 @@ public static Response PUT(Object url, String contenttype, String body) {
 
     /**
      * Sends a PUT request to the application under tests.
-     * 
+     *
      * @param request
      *            The given request
      * @param url
@@ -281,7 +285,7 @@ public static Response DELETE(String url) {
 
     /**
      * Sends a DELETE request to the application under tests.
-     * 
+     *
      * @param request
      *            The given request
      * @param url
@@ -310,7 +314,7 @@ public static Response DELETE(Request request, Object url) {
 
     public static void makeRequest(final Request request, final Response response) {
         final CountDownLatch actionCompleted = new CountDownLatch(1);
-        TestEngine.functionalTestsExecutor.submit(new Invoker.Invocation() {
+        final Future<?> invocationResult = TestEngine.functionalTestsExecutor.submit(new Invoker.Invocation() {
 
             @Override
             public void execute() throws Exception {
@@ -353,9 +357,27 @@ public InvocationContext getInvocationContext() {
 
         });
         try {
+            // We can not simply wait on the future result because of how continuations
+            // are implemented. Only when the latch is counted down the action is really
+            // completed. Therefore, wait on the latch.
             if (!actionCompleted.await(30, TimeUnit.SECONDS)) {
                 throw new TimeoutException("Request did not complete in time");
             }
+        } catch (Exception ex) {
+            throw new RuntimeException(ex);
+        }
+        try {
+            // We still call this to raise any exception that might have
+            // occurred during execution of the invocation.
+            invocationResult.get();
+        }
+        catch (ExecutionException e) {
+            throw unwrapOriginalException(e);
+        }
+        catch (Exception e) {
+            throw new RuntimeException(e);
+        }
+        try {
             if (savedCookies == null) {
                 savedCookies = new HashMap<>();
             }
@@ -377,6 +399,21 @@ public InvocationContext getInvocationContext() {
         }
     }
 
+    private static RuntimeException unwrapOriginalException(final ExecutionException e) {
+        // Check if the original exceptions fits the usual patterns. If yes, throw the very
+        // original runtime exception
+        final Throwable executionCause = e.getCause();
+        if (executionCause != null
+              && (executionCause instanceof JavaExecutionException || executionCause instanceof UnexpectedException)) {
+            final Throwable originalCause = executionCause.getCause();
+            if (originalCause != null && originalCause instanceof RuntimeException) {
+                throw (RuntimeException) originalCause;
+            }
+        }
+        // As a last fallback, just wrap everything up
+        return new RuntimeException(e);
+    }
+
     public static Response makeRequest(Request request) {
         Response response = newResponse();
         makeRequest(request, response);
@@ -436,7 +473,7 @@ public static Request newRequest() {
     // Assertions
     /**
      * Asserts a <em>2OO Success</em> response
-     * 
+     *
      * @param response
      *            server response
      */
@@ -446,7 +483,7 @@ public static void assertIsOk(Response response) {
 
     /**
      * Asserts a <em>404 (not found)</em> response
-     * 
+     *
      * @param response
      *            server response
      */
@@ -456,7 +493,7 @@ public static void assertIsNotFound(Response response) {
 
     /**
      * Asserts response status code
-     * 
+     *
      * @param status
      *            expected HTTP response code
      * @param response
@@ -468,7 +505,7 @@ public static void assertStatus(int status, Response response) {
 
     /**
      * Exact equality assertion on response body
-     * 
+     *
      * @param content
      *            expected body content
      * @param response
@@ -480,7 +517,7 @@ public static void assertContentEquals(String content, Response response) {
 
     /**
      * Asserts response body matched a pattern or contains some text.
-     * 
+     *
      * @param pattern
      *            a regular expression pattern or a regular text, ( which must be escaped using Pattern.quote)
      * @param response
@@ -495,7 +532,7 @@ public static void assertContentMatch(String pattern, Response response) {
     /**
      * Verify response charset encoding, as returned by the server in the Content-Type header. Be aware that if no
      * charset is returned, assertion will fail.
-     * 
+     *
      * @param charset
      *            expected charset encoding such as "utf-8" or "iso8859-1".
      * @param response
@@ -509,7 +546,7 @@ public static void assertCharset(String charset, Response response) {
 
     /**
      * Verify the response content-type
-     * 
+     *
      * @param contentType
      *            expected content-type without any charset extension, such as "text/html"
      * @param response
@@ -524,7 +561,7 @@ public static void assertContentType(String contentType, Response response) {
 
     /**
      * Exact equality assertion on a response header value
-     * 
+     *
      * @param headerName
      *            header to verify. case-insensitive
      * @param value
@@ -539,7 +576,7 @@ public static void assertHeaderEquals(String headerName, String value, Response
 
     /**
      * obtains the response body as a string
-     * 
+     *
      * @param response
      *            server response
      * @return the response body as an <em>utf-8 string</em>

samples-and-tests/just-test-cases/app/controllers/StatusCodes.java

@@ -0,0 +1,36 @@
+package controllers;
+
+import play.jobs.Job;
+import play.mvc.Controller;
+import play.mvc.Http.Response;
+
+public class StatusCodes extends Controller {
+
+  public static void justOkay() {
+    renderText("Okay");
+  }
+
+  public static void rendersNotFound() {
+    notFound();
+  }
+
+  public static void rendersUnauthorized() {
+    unauthorized();
+  }
+
+  public static void usesContinuation() {
+    final String text = await(new Job<String>() {
+      @Override
+      public String doJobWithResult() throws Exception {
+        return "Job completed successfully";
+      }
+    }.now());
+    Response.current().status = Integer.valueOf(201);
+    renderText(text);
+  }
+
+  public static void throwsException() throws Exception {
+    throw new UnsupportedOperationException("Whoops");
+  }
+
+}

samples-and-tests/just-test-cases/app/controllers/Transactional.java

@@ -19,9 +19,8 @@ public static void readOnlyTest() {
         tag2.name = "TransactionalTest";
         post.tags.add(tag1);
         post.tags.add(tag2);
-        post.save();
-        // since this is read only the count will not go up with successive 
-        // calls as the Post we just stored will be rolled back
+        post.save(); // since this is read only the request will fail with javax.persistence.TransactionRequiredException
+
         renderText("Wrote 1 post: total is now " + Post.count());
     }
 

samples-and-tests/just-test-cases/app/controllers/WithContinuations.java

@@ -163,14 +163,14 @@ public static void rollbackWithContinuationsThatWorks() {
 
     public static void streamedResult() {
         response.contentType = "text/html";
-        response.writeChunk("<h1>This page should load progressively in about 3 second</h1>");
+        response.writeChunk("<h1>This page should load progressively in about a few seconds</h1>");
         long s = System.currentTimeMillis();
         for(int i=0; i<100; i++) {
             await(10);
             response.writeChunk("<h2>Hello " + i + "</h2>");
         }
         long t = System.currentTimeMillis() - s;
-        response.writeChunk("Time: " + t + ", isOk->" + (t > 1000 && t < 10000));
+        response.writeChunk("Time: " + t + ", isOk->" + (t > 1000 && t < 25000));
     }
 
     public static void loopWithCallback() {
@@ -206,7 +206,7 @@ public void invoke() {
                     await(10, this);
                 } else {
                     long t = System.currentTimeMillis() - s.get();
-                    response.writeChunk("Time: " + t + ", isOk->" + (t > 1000 && t < 10000));
+                    response.writeChunk("Time: " + t + ", isOk->" + (t > 1000 && t < 25000));
                 }                
             }
         };

samples-and-tests/just-test-cases/conf/routes

@@ -5,8 +5,8 @@
 GET         /                               Application.index
 PUT         /sayHello                       Application.hello
 GET         /aGetForm                       Application.aGetForm
-GET         /aGetForm/                      Application.aGetForm2 
-GET         /optionalSlash/?                Application.optional 
+GET         /aGetForm/                      Application.aGetForm2
+GET         /optionalSlash/?                Application.optional
 GET         /index                          Application.index
 
 GET         /re/{<[a-z]+>re}                Application.ok
@@ -83,5 +83,11 @@ GET     /databinding/changeLanguage/{lang}/?                DataBinding.changeLa
 
 GET		/useAwaitViaOtherClass				WithContinuations.ControllerWithoutContinuations.useAwaitViaOtherClass
 
+GET   /status/ok/             StatusCodes.justOkay
+GET   /status/not-found/      StatusCodes.rendersNotFound
+GET   /status/unauthorized/   StatusCodes.rendersUnauthorized
+POST  /status/job/            StatusCodes.usesContinuation
+GET   /status/failure/        StatusCodes.throwsException
+
 # module
 *         /               module:secure

samples-and-tests/just-test-cases/test/BinaryTest.java

@@ -28,7 +28,7 @@ public void setUp() {
         Response deletedResponse = GET(deleteURL);
         assertStatus(200, deletedResponse);
     }
-    
+
     @Test
     public void testUploadSomething() {
         URL imageURL = reverse(); {
@@ -235,11 +235,13 @@ public void testGetEmptyBinary() {
         assertTrue(Binary.emptyInputStreamClosed);
     }
 
-    @Test
+    @Test(expected = Exception.class)
     public void testGetErrorBinary() {
-        Response response = GET("/binary/getErrorBinary");
-        // This does not work. See Lighthouse ticket #1637.
-        // assertStatus(500, response);
-        assertTrue(Binary.errorInputStreamClosed);
+        try {
+            GET("/binary/getErrorBinary");
+        }
+        finally {
+            assertTrue(Binary.errorInputStreamClosed);
+        }
     }
 }