> On Jan. 26, 2017, 4:11 p.m., Kirk Lund wrote: > > geode-web-api/src/test/java/org/apache/geode/rest/internal/web/controllers/AbstractBaseControllerJUnitTest.java, > > line 58 > > <https://reviews.apache.org/r/55988/diff/2/?file=1617107#file1617107line58> > > > > You're missing fail("message"); in your try-block just before the > > catch-statement. > > > > I recommend using AssertJ's assertThatThrownBy instead. > > > > import static org.assertj.core.api.Assertions.assertThatThrownBy; > > > > assertThatThrownBy(() -> SecurityService.getObjectOfType("", > > String.class)) > > .isInstanceOf(GemFireSecurityException.class); > > > > assertThatThrownBy(serverConnection::getUniqueId) > > .isExactlyInstanceOf(AuthenticationRequiredException.class) > > > > .hasMessage(HandShake_NO_SECURITY_CREDENTIALS_ARE_PROVIDED.getRawText()); > > Kevin Duling wrote: > If I understand what you're saying, your first recommendation is > something like: > > ``` > try { > throwGemFireRestException(message); > fail("Failed to throw a GemFireRestException!"); > } catch (GemfireRestException re) { > String json = abstractBaseController.convertErrorAsJson(re); > ErrorMessage errorMessage = mapper.readValue(json, > ErrorMessage.class); > assertEquals(message, errorMessage.message); > } > ``` > `fail("Failed to throw a GemFireRestException!")` would be essentially > unreachable as `throwGemFireRestException(message)` does not do anything > except throw the exeption. The compiler wouldn't complain, but the line > would be as unnecessary as a `return` statement. The try block within > `throwGemFireRestException` could possibily throw something else, such as an > `OutOfMemoryException`, so the catch should be changed to catch `Exception` > and not specifically a `NullPointerException`. However, as > `testConvertErrorAsJsonT()` throws Exception anyway, any exception bubbling > up would fail the test. > > > I will look in to AssertJ next. I'm not very familiar with it. From > your example and glance I took of > http://blog.codeleak.pl/2015/04/junit-testing-exceptions-with-java-8.html, it > appears to be a far better way of accomplishing this.
I think there's some confusion here. I'm not trying to test if an exception is thrown. I'm trying to test whether or not valid JSON is created from an exception. I've updated the test to skip generating a thrown exception and simply create them to pass in to the method. - Kevin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55988/#review163205 ----------------------------------------------------------- On Jan. 26, 2017, 3:30 p.m., Kevin Duling wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55988/ > ----------------------------------------------------------- > > (Updated Jan. 26, 2017, 3:30 p.m.) > > > Review request for geode, Jinmei Liao, Jared Stewart, and Kirk Lund. > > > Bugs: GEODE-2294 > https://issues.apache.org/jira/browse/GEODE-2294 > > > Repository: geode > > > Description > ------- > > When an error is thrown in the controller, it is supposed to be wrapped in to > a JSON message to return to the client. The problem is a full stacktrace is > also being sent, exceeding the maximum number of bytes allowed in a single > http message. Messages are not flagged as multi-part. And who wants an > entire stacktrace in a response message? Instead, the message should return > the error, but the stacktrace should only be in the server's log. > > > Diffs > ----- > > geode-web-api/build.gradle f74b86fb2cb44ee2c6fe7dc498a6bd11c45f8b6c > > geode-web-api/src/main/java/org/apache/geode/rest/internal/web/controllers/AbstractBaseController.java > b9d2bf4dec5b8d091207b6a98ff30d11ba7cc822 > > geode-web-api/src/test/java/org/apache/geode/rest/internal/web/controllers/AbstractBaseControllerJUnitTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/55988/diff/ > > > Testing > ------- > > precheckin running > This changes also fixed a REST test that previously was returning a > mysterious 500 error. Now it's returning a proper 404 error. > > > Thanks, > > Kevin Duling > >
