scwhittle commented on code in PR #30693:
URL: https://github.com/apache/beam/pull/30693#discussion_r1579063822
##########
runners/google-cloud-dataflow-java/worker/src/test/java/org/apache/beam/runners/dataflow/worker/util/common/worker/MapTaskExecutorTest.java:
##########
@@ -153,7 +154,6 @@ public void testExecuteMapTaskExecutor() throws Exception {
inOrder.verify(o1).finish();
inOrder.verify(o2).finish();
inOrder.verify(o3).finish();
- inOrder.verify(stateTracker).reset();
Review Comment:
I was thinking to just add verifications on the stateTracker methods here
(like you were doing with reset). So I was thinking of something like this
inOrder.verify(stateTracker).activate()
inOrder.verify(o3).start();
inOrder.verify(o2).start();
inOrder.verify(o1).start();
inOrder.verify(o1).finish();
inOrder.verify(o2).finish();
inOrder.verify(o3).finish();
inOrder.verify(stateTracker).deactivate();
However I realize now that executor.execute() is just calling
stateTracker.activate() which in the real object returns a closable that is
deactivated but with the pure mock returns an arbitrary closable. You could set
up an expectation that stateTracker.activate() returns a closable that calls
stateTracker.deactivate() but instead I think it would be better to just use a
spy of a real object.
ExecutionStateTracker stateTracker =
Mockito.spy(ExecutionStateTracker.newForTest());
Then the tracker will be real and return the real closable that will call
deactivate. The spy just lets you verify it is called.
It looks like activate() is public, deactivate is private, you might have to
make it public with visiblefortesting annotation for it to work.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
