You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by "kirklund (GitHub)" <gi...@apache.org> on 2018/12/11 00:46:09 UTC
[GitHub] [geode] kirklund opened pull request #2982: GEODE-6143:
Remove PowerMock from ExecuteFunction tests
This PR consists of two commits. First commit refactors FunctionService so that it is unit testable. Second commit removes PowerMock from the ExecuteFunction unit tests.
```
commit edf7d3c65892e0c3b05fdf25c4c2bfb1cd2bf23a (HEAD -> GEODE-6176-FunctionService-PowerMock, kirklund-fork/GEODE-6176-FunctionService-PowerMock)
Author: Kirk Lund <kl...@apache.org>
Date: Mon Dec 10 16:42:50 2018 -0800
GEODE-6143: Remove PowerMock from ExecuteFunction tests
Remove PowerMock from:
* ExecuteFunctionTest
* ExecuteFunction65Test
* ExecuteFunction66Test
Add test for ExecuteFunction70Test by subclasses ExecuteFunction66Test.
```
```
commit 0992c246577ba5a4c8b42d2c06974c1efcb1de60
Author: Kirk Lund <kl...@apache.org>
Date: Mon Dec 10 13:44:24 2018 -0800
GEODE-6176: Make FunctionService testable with internal delegates
Introduce new FunctionExecutionService interfaces:
* FunctionExecutionService (User API)
* InternalFunctionExecutionService
Implement new interfaces with a traditional class to be instantiated:
* FunctionExecutionServiceImpl
Collapse FunctionServiceManager into FunctionExecutionServiceImpl
Change static API classes to delegate to private instance of
FunctionExecutionServiceImpl:
* FunctionService (User API) -- only has private internal changes
* InternalFunctionService -- now extends FunctionService
This should allow unit tests to avoid using PowerMock.
```
[ Full content available at: https://github.com/apache/geode/pull/2982 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on issue #2982: GEODE-6143:
Remove PowerMock from ExecuteFunction tests
Posted by "kirklund (GitHub)" <gi...@apache.org>.
Adding FunctionExecutionService interface to User API package caused AssemblyContentsIntegrationTest to fail. I'm going to move it internal since I was questioning whether or not I should expose it or not.
[ Full content available at: https://github.com/apache/geode/pull/2982 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund closed pull request #2982: GEODE-6143:
Remove PowerMock from ExecuteFunction tests
Posted by "kirklund (GitHub)" <gi...@apache.org>.
[ pull request closed by kirklund ]
[ Full content available at: https://github.com/apache/geode/pull/2982 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2982:
GEODE-6143: Remove PowerMock from ExecuteFunction tests
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
Why not just use this constructor in the production code as well, and inject the dependencies there? Do we need two constructors (one of which is a test hook)?
[ Full content available at: https://github.com/apache/geode/pull/2982 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2982:
GEODE-6143: Remove PowerMock from ExecuteFunction tests
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
I know the behavior theoretically didn't change here (we are just making the code more testable), but I think we should add tests for this new class. I didn't see any tests in either commit, but I apologize if I just somehow missed them.
[ Full content available at: https://github.com/apache/geode/pull/2982 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2982:
GEODE-6143: Remove PowerMock from ExecuteFunction tests
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
As you mentioned, we are adding a new interface here to make the code testable and avoid using the static FunctionService methods. Are any additional steps required when we add new public API like this? Do we need to add documentation? You mentioned we _could_ make this interface internal. My only concern here is if i were a Geode user and I discovered both FunctionService and FunctionExecutionService in my IDE, it wouldn't be clear to me which to use. Maybe we should make this interface internal until we can fully remove the FunctionService interface in Geode 2.0?
[ Full content available at: https://github.com/apache/geode/pull/2982 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2982:
GEODE-6143: Remove PowerMock from ExecuteFunction tests
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
Same comment as above. Should this be calling `onServers()`?
[ Full content available at: https://github.com/apache/geode/pull/2982 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on issue #2982: GEODE-6143:
Remove PowerMock from ExecuteFunction tests
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
I meant to put an overview comment on my review, but the requesting of changes is mainly around adding tests for FunctionServiceExecutionImpl and what looks like maybe a few incorrect method calls (`onServer()` vs `onServers()`). Also a few leftover TODOs.
[ Full content available at: https://github.com/apache/geode/pull/2982 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2982:
GEODE-6143: Remove PowerMock from ExecuteFunction tests
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
Same comment as above. It would be nice to avoid a test only constructor if possible.
[ Full content available at: https://github.com/apache/geode/pull/2982 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2982:
GEODE-6143: Remove PowerMock from ExecuteFunction tests
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
Leftover TODO
[ Full content available at: https://github.com/apache/geode/pull/2982 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2982:
GEODE-6143: Remove PowerMock from ExecuteFunction tests
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
Not sure if this is another "test only" constructor, but if so, it is missing the annotation. It is hard to tell from the diff in GitHub.
[ Full content available at: https://github.com/apache/geode/pull/2982 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2982:
GEODE-6143: Remove PowerMock from ExecuteFunction tests
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
Should this be calling `onServers()`?
[ Full content available at: https://github.com/apache/geode/pull/2982 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org