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

Fraserhardy · 2018-07-18T12:40:47Z

Lighthouse #1849 from PR #778

Changes have been ported over from an existing PR that became out of date.
This fixes a pretty critical issue where functional tests will pass even if the controller being called throws an exception.

The new changes actually throw the exception up to the tests so that it can be handled within the test. Expected exceptions can be checked for correctly in this way.

…g issues

…y expect

# Conflicts: # framework/src/play/test/FunctionalTest.java # samples-and-tests/just-test-cases/conf/routes # samples-and-tests/just-test-cases/test/FunctionalTestTest.java

Update qudini/play1 master

* master: [playframework#1244] doc(release): add documentation for 1.5.1 [#1896] fix play daemon stacktrace on [re]start-stop

Fixed issue with Static test

Fraserhardy · 2018-07-19T17:10:06Z

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

-    
-    @Test
+
+    @Test(expected = RenderStatic.class)


The changes to how exceptions are thrown back to tests has resulted in RenderStatic.class being thrown back up to the tests in this case.

This change passes the test but I'm not 100% convinced this is the correct approach. Could someone who has more experience in how the static files are handled take a look at this please.

@hsch this PR uses your original changes from an old PR I'm attempting to get merged. Perhaps you could advise? @xael-fry could you take a look too please.

@Fraserhardy @hsch Certainly, it's a bad idea to expect RenderStatic.class in this test. But it's not your fault.

There is two problems with this test:

it WAS incorrect before your change. It actually didn't test anything. If only you add a line assertContentEquals("test", response); to this test, it would fail.

Bad design of method play.mvc.Router.Route#matches(java.lang.String, java.lang.String, java.lang.String, java.lang.String): it has side effect. Instead of returning Map<String, String>, it sometimes throws RenderStatic. And PlayHandler handles it as a special case: catches RenderStatic and sends file content asynchronously to the browser. Disgusting. :(

What I suggest is making a similar "special case" in FunctionalTest:

catch (ExecutionException e) { RuntimeException originalException = unwrapOriginalException(e); if (originalException instanceof RenderStatic) { response.status = 200; response.direct = ((RenderStatic) originalException).file; } else { throw originalException; } }

and adding a method in FuncationalTest:

public static void assertIsStaticFile(Response response, String filePath) { assertIsOk(response); String file = (String) response.direct; assertEquals(filePath, file); }

Finally, this test should be rewritten:

@Test public void testGettingStaticFile() { Response response = GET("/public/session.test?req=1"); assertIsStaticFile(response, "public/session.test"); }

asolntsev · 2018-07-27T22:58:35Z

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

-        // assertStatus(500, response);
-        assertTrue(Binary.errorInputStreamClosed);
+        try {
+            GET("/binary/getErrorBinary");


@Fraserhardy It doesn't seem to be a correct test.
You should restore assertStatus(500, response); instead of expecting Exception.class.

asolntsev · 2018-07-27T23:09:21Z

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


 public class TransactionalJPATest extends FunctionalTest {

-    @Test
+    @Test(expected = TransactionRequiredException.class)
    public void testImport() {
        Response response = GET("/Transactional/readOnlyTest");


Why did you remove few lines from the previous test? They seem to be reasonable.
Probably you should move them to a separate test method?

assertIsOk(response); response = GET("/Transactional/echoHowManyPosts"); assertIsOk(response); assertEquals("There are 0 posts", getContent(response));

asolntsev · 2018-07-28T00:02:27Z

framework/src/play/test/FunctionalTest.java

+              && (executionCause instanceof JavaExecutionException || executionCause instanceof UnexpectedException)) {
+            final Throwable originalCause = executionCause.getCause();
+            if (originalCause != null && originalCause instanceof RuntimeException) {
+                throw (RuntimeException) originalCause;


This is bug. Change throw (RuntimeException) originalCause; to return (RuntimeException) originalCause;

asolntsev · 2018-07-28T00:10:47Z

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

-    
-    @Test
+
+    @Test(expected = RenderStatic.class)


@Fraserhardy @hsch Certainly, it's a bad idea to expect RenderStatic.class in this test. But it's not your fault.

There is two problems with this test:

it WAS incorrect before your change. It actually didn't test anything. If only you add a line assertContentEquals("test", response); to this test, it would fail.

Bad design of method play.mvc.Router.Route#matches(java.lang.String, java.lang.String, java.lang.String, java.lang.String): it has side effect. Instead of returning Map<String, String>, it sometimes throws RenderStatic. And PlayHandler handles it as a special case: catches RenderStatic and sends file content asynchronously to the browser. Disgusting. :(

What I suggest is making a similar "special case" in FunctionalTest:

catch (ExecutionException e) { RuntimeException originalException = unwrapOriginalException(e); if (originalException instanceof RenderStatic) { response.status = 200; response.direct = ((RenderStatic) originalException).file; } else { throw originalException; } }

and adding a method in FuncationalTest:

public static void assertIsStaticFile(Response response, String filePath) { assertIsOk(response); String file = (String) response.direct; assertEquals(filePath, file); }

Finally, this test should be rewritten:

@Test public void testGettingStaticFile() { Response response = GET("/public/session.test?req=1"); assertIsStaticFile(response, "public/session.test"); }

…st RenderStatic in functional tests P.S. it implements my comments for PR playframework#1246

P.S. it implements my comments for PR playframework#1246

…eleted in PR 1246 P.S. it implements my comments for PR playframework#1246

asolntsev · 2018-07-31T21:30:41Z

@Fraserhardy @hsch I merged your pull request and then implemented my comments in a separate PR. You can see the final version in master branch now.

Fraserhardy · 2018-08-02T21:42:19Z

@asolntsev Thanks for your help on this. Much appreciated.

Fraserhardy · 2018-08-03T15:36:18Z

@asolntsev Now this has been merged what steps are required to get this into a new version? I know we've just missed 1.5.1. I noticed there are a few vulnerable dependencies that have just been updated too so perhaps another release is due soon.

asolntsev · 2018-08-04T07:55:14Z

@Fraserhardy Unfortunately, it's not an automated process. Typically @xael-fry does the release. It includes some additional work like preparing a documentation etc.

@xael-fry Where do you plan to make a new release?

xael-fry · 2018-08-22T15:58:19Z

@asolntsev In september, but this one should not have been merged, modifications should have been combined in one commit.
I would have to clean it up

… deleted in PR 1246 P.S. it implements my comments for PR playframework#1246 * [#1849] improve design of BinaryTest P.S. it implements my comments for PR playframework#1246 * [#1849] add method assertIsStaticFile() and special case allowing to test RenderStatic in functional tests P.S. it implements my comments for PR playframework#1246

Hendrik Schnepel and others added 10 commits December 25, 2015 14:27

[#1849] Made an unrelated test a little bit more robust against timin…

139e2fa

…g issues

[#1849] Added test cases that show the unexpected behavior

29c7e3f

[#1849] Adapted BinaryTest to reflect corrected expectations

ea60d35

[#1849] Fixed that exceptions from controllers are being ignored

f5419be

[#1849] Give tests more fine grained control over what exceptions the…

0dfbbfb

…y expect

Merge branch 'master' into 1849_controllerException

27db29e

# Conflicts: # framework/src/play/test/FunctionalTest.java # samples-and-tests/just-test-cases/conf/routes # samples-and-tests/just-test-cases/test/FunctionalTestTest.java

Resolved merge conflict

b41ebc3

Merge pull request #1 from playframework/master

2637d90

Update qudini/play1 master

Merge branch 'master' into 1849_controllerException

97c3ebc

* master: [playframework#1244] doc(release): add documentation for 1.5.1 [#1896] fix play daemon stacktrace on [re]start-stop

Fixed issue with Transactional Tests

28b06cd

Fixed issue with Static test

Fraserhardy commented Jul 19, 2018

View reviewed changes

asolntsev requested changes Jul 28, 2018

View reviewed changes

asolntsev added a commit to codeborne/play that referenced this pull request Jul 31, 2018

#1849 add method assertIsStaticFile() and special case allowing to te…

77a2c40

…st RenderStatic in functional tests P.S. it implements my comments for PR playframework#1246

asolntsev added a commit to codeborne/play that referenced this pull request Jul 31, 2018

#1849 improve design of BinaryTest

c54f2e6

P.S. it implements my comments for PR playframework#1246

asolntsev added a commit to codeborne/play that referenced this pull request Jul 31, 2018

#1849 restore checks in TransactionalJPATest that were accidentally d…

29319dd

…eleted in PR 1246 P.S. it implements my comments for PR playframework#1246

asolntsev merged commit 2410b22 into playframework:master Jul 31, 2018

xael-fry added this to the 1.5.2 milestone Aug 22, 2018

xael-fry mentioned this pull request Aug 23, 2018

[#1849] Give tests more fine grained control over what exceptions they expect #1258

Merged

xael-fry mentioned this pull request Aug 23, 2018

[#1849] restore checks in TransactionalJPATest that were accidentally deleted in PR 1246 #1259

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Fraserhardy commented Jul 18, 2018 •

edited

Loading

Fraserhardy Jul 19, 2018 •

edited

Loading

asolntsev Jul 28, 2018 •

edited

Loading

asolntsev Jul 27, 2018

asolntsev Jul 27, 2018

asolntsev Jul 28, 2018

asolntsev Jul 28, 2018 •

edited

Loading

asolntsev commented Jul 31, 2018

Fraserhardy commented Aug 2, 2018

Fraserhardy commented Aug 3, 2018

asolntsev commented Aug 4, 2018

xael-fry commented Aug 22, 2018

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

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

Conversation

Fraserhardy commented Jul 18, 2018 • edited Loading

Fraserhardy Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

asolntsev Jul 28, 2018 • edited Loading

Choose a reason for hiding this comment

asolntsev Jul 27, 2018

Choose a reason for hiding this comment

asolntsev Jul 27, 2018

Choose a reason for hiding this comment

asolntsev Jul 28, 2018

Choose a reason for hiding this comment

asolntsev Jul 28, 2018 • edited Loading

Choose a reason for hiding this comment

asolntsev commented Jul 31, 2018

Fraserhardy commented Aug 2, 2018

Fraserhardy commented Aug 3, 2018

asolntsev commented Aug 4, 2018

xael-fry commented Aug 22, 2018

Fraserhardy commented Jul 18, 2018 •

edited

Loading

Fraserhardy Jul 19, 2018 •

edited

Loading

asolntsev Jul 28, 2018 •

edited

Loading

asolntsev Jul 28, 2018 •

edited

Loading