test(JenkinsClient): add tests for exceptional cases on JenkinsClient #629
Conversation
| final Gson gson = new Gson(); | ||
| final JenkinsClient client = new JenkinsClient(gson); | ||
|
|
||
| InputStream invalidPayload = new ByteArrayInputStream("{ this is ] no json |[!".getBytes()); |
There was a problem hiding this comment.
I'm not sure, maybe it would be better to stub the resoponse of gson.fromJson(...) here to force throwing an exception... 🤔 that might, however, lead to succeeding tests but breaking code in production (assume the parser throws a different exception after an update).
There was a problem hiding this comment.
As long as it's a few objects that are easy to create (e.g. not something like MTE with a ton of start-up time and interconnected state), it is nice to be able to use real objects instead of mockito mocks.
Partly for the reason you mentioned: it can be hard to know when your mockito-defined behavior is no longer realistic.
and partly for some Java-specific worries I caught when working on old Mockito tests with warnings about illegal reflective access. Maybe they've fixed that? because Mockito is clearly still going strong and hasn't been destroyed by new releases of Java.
| final Gson gson = new Gson(); | ||
| final JenkinsClient client = new JenkinsClient(gson); | ||
|
|
||
| InputStream invalidPayload = new ByteArrayInputStream("{ this is ] no json |[!".getBytes()); |
There was a problem hiding this comment.
As long as it's a few objects that are easy to create (e.g. not something like MTE with a ton of start-up time and interconnected state), it is nice to be able to use real objects instead of mockito mocks.
Partly for the reason you mentioned: it can be hard to know when your mockito-defined behavior is no longer realistic.
and partly for some Java-specific worries I caught when working on old Mockito tests with warnings about illegal reflective access. Maybe they've fixed that? because Mockito is clearly still going strong and hasn't been destroyed by new releases of Java.
| } catch (JsonSyntaxException | JsonIOException e) { | ||
| logger.warn("Failed to read JSON from '{}'", url.toExternalForm(), e); | ||
| } catch (IOException e) { | ||
| e.printStackTrace(); | ||
| logger.warn("Failed to read from URL: {}", url.toExternalForm(), e); | ||
| } | ||
| return null; |
There was a problem hiding this comment.
This is the sort of thing where I'd be happy to let errors throw exceptions instead of returning nullable. Sure, it could help to catch gson's exception here and wrap it in something defined in our API, so the caller doesn't have to worry if it's a gson.JsonSyntaxException vs some other implementation, but having to do null-checking isn't better for our caller than having to catch an exception.
Not required for this PR though.
There was a problem hiding this comment.
I'd like to go more in the direction of monadic error handling, so instead of catch-and-log-to-nullable I'd like to wrap the error/message in a better error type and use that in further processing. This may be combined with new features such as pattern matching on instanceof.
Not sure how to proceed on that front, though. You can find a very simple Result class, or go for more full-fledged libraries such as https://www.vavr.io/ or jOOλ. 🤓
How much of that would we get "for free" when using Kotlin? ... or maybe exceptions aren't that bad at all, and I should befriend them a little more? 🧐
There was a problem hiding this comment.
Kotlin doesn't have Result built-in. If you want to go the full functional programming route with Kotlin, I think the thing to use is Λrrow.
I don't think exceptions are such a bad thing. The feeling I get is that most people don't think Java's checked exceptions are a feature worth keeping — e.g. Kotlin omits that idea entirely — but in general, throwing an exception when the method can't continue as planned, allowing that exception to bubble up through the stack, and having dedicated syntax for error-handling blocks (try/catch) seems okay to me.
(Clearly some language designers feel otherwise, as neither Go nor Rust have exceptions.)
Anyway, it sounds like neither of us are really fans of having null represent an error case. I haven't been won over by the idea of using Result in Java yet; it feels like it adds more noise to the happy-path (.getOrElse, etc) than try/catch does, but if we had a fully functional framework for use with JavaFX I could go along with that.
As this is a network operation, I also would want this to be an asynchronous function and would use something like ListenableFuture for that. But I hear programming languages have support for coroutines these days so maybe that's the cool thing to do?
The
JenkinsClientintroduced in #624 is not tested. This PR adds two tests for exceptional cases to ensure that the launcher handles these gracefully. One of the tests reveiled that the launcher would crash.