[GitHub] [geode] jinmeiliao edited a comment on pull request #7395: GEODE-10084: CliFunction should handle all throwable
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() {
@Override
public CliFunctionResult executeFunction(FunctionContext
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 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.)
--
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-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] jinmeiliao edited a comment on pull request #7395: GEODE-10084: CliFunction should handle all throwable
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() {
@Override
public CliFunctionResult executeFunction(FunctionContext
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 captor =
ArgumentCaptor.forClass(CliFunctionResult.class);
verify(resultSender).lastResult(captor.capture());
CliFunctionResult result = captor.getValue();
assertThat(result.getResultObject())
.isSame(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.)
--
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-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] jinmeiliao edited a comment on pull request #7395: GEODE-10084: CliFunction should handle all throwable
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() {
@Override
public CliFunctionResult executeFunction(FunctionContext
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 captor =
ArgumentCaptor.forClass(CliFunctionResult.class);
verify(resultSender).lastResult(captor.capture());
CliFunctionResult result = captor.getValue();
assertThat(result.getResultObject())
.isSame(InternalGemFireError.class); // 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.)
--
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-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] jinmeiliao edited a comment on pull request #7395: GEODE-10084: CliFunction should handle all throwable
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() {
@Override
public CliFunctionResult executeFunction(FunctionContext
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 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-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] jinmeiliao edited a comment on pull request #7395: GEODE-10084: CliFunction should handle all throwable
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() {
@Override
public CliFunctionResult executeFunction(FunctionContext
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 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-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
