GEODE-10084: CliFunction should handle all throwable

[jinmeiliao]

No description provided.

[onichols-pivotal]

comment deleted

[demery-pivotal]

The new ImportDataFunctionTest does not test ImportDataFunction at all. It tests CliFunction. So move this test from ImportDataFunctionTest to CliFunctionTest. And change it so that instead of spying the unit under test, it creates a derived class where executeFunction() does nothing but throw.

Then add unit tests for the new and changed functionality in ImportDataFunction.

[jinmeiliao]

The new ImportDataFunctionTest does not test ImportDataFunction at all. It tests CliFunction. So move this test from ImportDataFunctionTest to CliFunctionTest. And change it so that instead of spying the unit under test, it creates a derived class where executeFunction() does nothing but throw.

Then add unit tests for the new and changed functionality in ImportDataFunction.

Oh, yeah, that's true. fixed. I don't have a test for ImporrtDataFunction since I don't know what kind of test would make sense for the little change I made besides the test I already wrote.

[demery-pivotal]

Thanks for moving that test to CliFunction. It fits much better there.

These two problems remain:

1. Do not spy the thing being tested. CliFunction is designed to be extended. So make the test create a derived class, and make its executeFunction method throw.
2. Write tests for ImportDataFunction.

Here are some test ideas.

One important change is that ImportDataFunction.executeFunction(…) returns a result, where the old execute(…) did not. Here are some things to test:

• If the region exists, is the result correct (status code, member name, message)?
• If the region doesn't exist, does the result correctly describe the problem (status code, member name, message)?

Here are some other things to test, to make sure they did not change:

• Does executeFunction invoke the callbacks from the snapshot service?
• Does executeFunction set parallel mode according to arg 3?
• Does executeFunction tell the snapshot service to load the snapshot with the right parameters?

[jinmeiliao]

Thanks for moving that test to CliFunction. It fits much better there.

These two problems remain:

1. Do not spy the thing being tested. CliFunction is designed to be extended. So make the test create a derived class, and make its executeFunction method throw.
2. Write tests for ImportDataFunction.

Here are some test ideas.

One important change is that ImportDataFunction.executeFunction(…) returns a result, where the old execute(…) did not. Here are some things to test:

• If the region exists, is the result correct (status code, member name, message)?
• If the region doesn't exist, does the result correctly describe the problem (status code, member name, message)?

Here are some other things to test, to make sure they did not change:

• Does executeFunction invoke the callbacks from the snapshot service?
• Does executeFunction set parallel mode according to arg 3?
• Does executeFunction tell the snapshot service to load the snapshot with the right parameters?

Thanks for the suggestion. More tests added. I didn't change the implementation of that function, only the error handling. so that part is tested.

[demery-pivotal]

Thank you for adding those tests. I like that you tested them through execute() rather than through executeFunction().

This problem remains, and in fact is getting worse:

• Do not spy the thing being tested.

[jinmeiliao]

Thank you for adding those tests. I like that you tested them through execute() rather than through executeFunction().

This problem remains, and in fact is getting worse:

• Do not spy the thing being tested.

Oh, you are right. I don't need to use spies.

[demery-pivotal]

Do not mock the thing being tested. CliFunction is designed to be extended. So make the test create a derived class, and make its executeFunction method throw.

I'm honestly astonished that the CliFunction test passes, given that it's testing a mock. My guess is that the mock is behaving like a spy.

[jinmeiliao]

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 in the test 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())
        .isSameAs(theThrownException); // Or maybe isEqualTo() the actual instance that was thrown, or isInstanceOf()

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

[jinmeiliao]

Do not mock the thing being tested. CliFunction is designed to be extended. So make the test create a derived class, and make its executeFunction method throw.

I'm honestly astonished that the CliFunction test passes, given that it's testing a mock. My guess is that the mock is behaving like a spy.

yeah, I changed back to use a spy.

[demery-pivotal]

Do not mock or spy the thing being tested.

CliFunction is designed to be extended. So make the test create a subclass, and make the subclass's executeFunction method throw. See below (or my previous comment) for an example of how to do this.

It would be a good idea to assert that the result contains the exception that was thrown by the subclass.

Here's a test that does all of those things. Notice that it tests CliFunction's error handling' without mocking or spying the thing being tested, and without altering an existing implementation subclass in order to test the CliFunction:

@Test  
public void executeShouldSendCliFunctionResultIfExecuteFunctionThrowsError() {  
  Error errorThrownByExecuteFunction = new InternalGemFireError("test");  
 CliFunction<Object[]> function = new CliFunction<Object[]>() {  
    @Override  
 public CliFunctionResult executeFunction(FunctionContext<Object[]> context) {  
      throw errorThrownByExecuteFunction;  
 }  
  };  
  
 function.execute(context);  
  
 ArgumentCaptor<CliFunctionResult> captor = ArgumentCaptor.forClass(CliFunctionResult.class);  
 verify(resultSender).lastResult(captor.capture());  
 CliFunctionResult result = captor.getValue();  
 assertThat(result)  
      .isNotNull();  
 assertThat(result.getResultObject())  
      .isSameAs(errorThrownByExecuteFunction);  
}

If the only reason to change ImportDataFunction was to use it to test CliFunction, then those changes are not necessary. (Though it would be a shame to throw those new tests away.)

[jinmeiliao]

Do not mock or spy the thing being tested.

CliFunction is designed to be extended. So make the test create a subclass, and make the subclass's executeFunction method throw. See below (or my previous comment) for an example of how to do this.

It would be a good idea to assert that the result contains the exception that was thrown by the subclass.

Here's a test that does all of those things. Notice that it tests CliFunction's error handling' without mocking or spying the thing being tested, and without altering an existing implementation subclass in order to test the CliFunction:

@Test  
public void executeShouldSendCliFunctionResultIfExecuteFunctionThrowsError() {  
  Error errorThrownByExecuteFunction = new InternalGemFireError("test");  
 CliFunction<Object[]> function = new CliFunction<Object[]>() {  
    @Override  
 public CliFunctionResult executeFunction(FunctionContext<Object[]> context) {  
      throw errorThrownByExecuteFunction;  
 }  
  };  
  
 function.execute(context);  
  
 ArgumentCaptor<CliFunctionResult> captor = ArgumentCaptor.forClass(CliFunctionResult.class);  
 verify(resultSender).lastResult(captor.capture());  
 CliFunctionResult result = captor.getValue();  
 assertThat(result)  
      .isNotNull();  
 assertThat(result.getResultObject())  
      .isSameAs(errorThrownByExecuteFunction);  
}

If the only reason to change ImportDataFunction was to use it to test CliFunction, then those changes are not necessary. (Though it would be a shame to throw those new tests away.)

I understand it as a theory. But in this particular case, isn't spy a better way or a more readable way to stub out the executeFunction method rather than having to implement an anonymous class to do exactly the same thing? I thought the purpose of spy or mock is to reduce the need for test classes.

[demery-pivotal]

I thought the purpose of spy or mock is to reduce the need for test classes.

The purpose of mocks (and, in very rare cases, spies) is to make it easier for the test to control and observe the collaborators of the thing you're testing.

It's true that code I offered does create some noise. To make it more readable, move the noise into a helper method, and name the helper method to make its purpose in the test clear. So the body could do something like this:

function = cliFunctionThatThrows(theError);

[jinmeiliao]

I thought the purpose of spy or mock is to reduce the need for test classes.

The purpose of mocks (and, in very rare cases, spies) is to make it easier for the test to control and observe the collaborators of the thing you're testing.

It's true that code I offered does create some noise. To make it more readable, move the noise into a helper method, and name the helper method to make its purpose in the test clear. So the body could do something like this:

function = cliFunctionThatThrows(theError);

I don't think I agree in this particular case, but I will quit making my case.