[GitHub] [geode] jinmeiliao edited a comment on pull request #7395: GEODE-10084: CliFunction should handle all throwable

[GitBox <gi...@apache.org>]

jinmeiliao edited a comment on pull request #7395:
URL: https://github.com/apache/geode/pull/7395#issuecomment-1051122960


   > That was I did originally, I thought you don't want that. So should I use a spy or create a new derived class or just as is?
   
   I did want a derived class. Here's way to do that without mocking or spying the thing being tested:
   ```
       function = new CliFunction<Object[]>() {
         @Override
         public CliFunctionResult executeFunction(FunctionContext<Object[]> context) {
           throw new InternalGemFireError("test");
         }
       };
   ```
   
   Either that or create an inner class called something like `ThrowingCliFunction` that's implemented the same way.
   
   Something I just noticed: It would be a good idea to assert that the result contains the exception that was thrown. Like this:
   ```
       ArgumentCaptor<CliFunctionResult> captor = ArgumentCaptor.forClass(CliFunctionResult.class);
       verify(resultSender).lastResult(captor.capture());
       CliFunctionResult result = captor.getValue();
       assertThat(result.getResultObject())
           .isSame(InternalGemFireError.class); // Or maybe isSame() or isEqualTo() the actual instance that was thrown
   ```
   
   If the only reason to change `ImportDataFunction` was to use it to test `CliFunction`, that's no longer necessary. (Though it would be a shame to throw those new tests away.)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org