jinmeiliao commented on pull request #7395:
URL: https://github.com/apache/geode/pull/7395#issuecomment-1051379049
> > 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
jinmeiliao commented on pull request #7395:
URL: https://github.com/apache/geode/pull/7395#issuecomment-1051322125
> 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`
jinmeiliao commented on pull request #7395:
URL: https://github.com/apache/geode/pull/7395#issuecomment-1051129631
> 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.
>
jinmeiliao commented on pull request #7395:
URL: https://github.com/apache/geode/pull/7395#issuecomment-1051122960
> So make the test create a derived class
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?
jinmeiliao commented on pull request #7395:
URL: https://github.com/apache/geode/pull/7395#issuecomment-1051077952
> 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
jinmeiliao commented on pull request #7395:
URL: https://github.com/apache/geode/pull/7395#issuecomment-1051069871
> 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`
jinmeiliao commented on pull request #7395:
URL: https://github.com/apache/geode/pull/7395#issuecomment-1050374418
> The new `ImportDataFunctionTest` does not test `ImportDataFunction` at
all. It tests `CliFunction`. So move this test from `ImportDataFunctionTest` to
`CliFunctionTest`.
