> 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());
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.
- 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
>
>
