Alona Kaplan has posted comments on this change. Change subject: webadmin: fluent UiAction ......................................................................
Patch Set 25: (18 comments) https://gerrit.ovirt.org/#/c/36848/25/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/action/UiAction.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/action/UiAction.java: Line 173: return name; Line 174: } Line 175: Line 176: void tryToFinalize(boolean handleErrors) { Line 177: if (actionFlowState.isAllDone() && actionFlowState.isStartedProgress()) { > I think only model.stopProgress() should depend on whether actionFlowState. Done Line 178: model.stopProgress(); Line 179: Line 180: if (handleErrors) { Line 181: handleErrors(); https://gerrit.ovirt.org/#/c/36848/25/frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/action/AsyncUiActionTest.java File frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/action/AsyncUiActionTest.java: Line 85: assertFinishedSuccessfully(actions); Line 86: } Line 87: Line 88: @Test Line 89: public void runNextActionFlowWithLastActionFailureTest() { > How about a test where the first action fails, which makes sure the second There is a specific test for this use-case in each of the sub-classes. Since the sub-classes have different behavior in this case the test couldn't be common. Line 90: List<UiAction> actions = runNextActionFlowCommon(false); Line 91: assertFinishedWithFailures(actions, 1); Line 92: } Line 93: Line 96: ActionFlowState flowState = actions.get(0).getActionFlowState(); Line 97: Line 98: verifyRunActionAndExecuteCallbacksRandomly(true, flowState, 1); Line 99: List<C> callbacks = verfifyRunAction(2); Line 100: executeCallbacks(lastSuccess, flowState, callbacks.subList(callbacks.size() - 1, callbacks.size())); > This code REALLY isn't clear - it took me a very long time to figure out th I'm not sure why did it take you so long to figure out the behavior. 'verifyRunActionAndExecuteCallbacksRandomly(..)' as its name specifies, is a composition of two methods 'verfifyRunAction(.., exepectedNumOfRunActionExecutions)'- verifies the Frontend.runAction was exucted 'exepectedNumOfRunActionExecutions' times and return the callback of all the executed actions. 'executeCallbacks(.., callbacks)'- executes the callbacks that were passed to the method. The callbacks are executed in a random order to simulate the real life behavior of getting the responses from the backed in a random order. The method is generic and therefore unaware to the content of the callbacks it executes. So I don't really understand why did you except if to do the 'future' checks. As you see, in this case 'verifyRunActionAndExecuteCallbacksRandomly(..,1)'- verifies that only ONE runAction was executed and therefore it executes only its ONE callback. Since its callback executes another runAction. Line 99- verifies that up to this point 2 runActions had run. Line 100- executes just the callback of the last action (since the callback of the firs action was already executed). If you have a better suggestion of how to write this code I will be happy to hear. Meanwhile, I've added a new overload to verifyRunActionAndExecuteCallbacksRandomly that excepts 'numOfCallbacksFromTheEndToExecute' parameter. So now line 99 and 100 are combined to one method call. Also, added java docs to verifyRunActionAndExecuteCallbacksRandomly, verfifyRunAction and executeCallbacks. Line 101: Line 102: return actions; Line 103: } Line 104: Line 109: ActionFlowState flowState = actions.get(0).getActionFlowState(); Line 110: Line 111: verifyRunActionAndExecuteCallbacksRandomly(true, flowState, 2); Line 112: List<C> callbacks = verfifyRunAction(4); Line 113: executeCallbacks(true, flowState, callbacks.subList(callbacks.size() - 2, callbacks.size())); > Same comment as above. same answer Line 114: Line 115: assertFinishedSuccessfully(actions); Line 116: } Line 117: Line 117: Line 118: @Test Line 119: public void runMixedActionFlowWithFailureOnIndependentAction() { Line 120: // action1.and(action2).next(action3).and(action4) Line 121: // action1 has an error- all the flow will be executed > As above, a more interesting case is if action1 also has a next action. Please refer to the test in line 144, that is exactly what it verifies. Line 122: Line 123: List<UiAction> actions = runActionFlow(ActionType.parallel, ActionType.next, ActionType.parallel); Line 124: ActionFlowState flowState = actions.get(0).getActionFlowState(); Line 125: Line 131: // execute action2 callback Line 132: executeCallbacks(true, flowState, callbacks.subList(0, 1)); Line 133: Line 134: // verify action3 and action4 were also executed Line 135: callbacks = verfifyRunAction(4); > Same as before - it's very hard to follow what you're validating. It is a complex case. I hope my explanation in the previous comment helped you to understand. The flow is action1.and(action2).next(action3).and(action4), action1 has an error. In mean that- phase 1- action1 and action2 will be executed. phase 2- action3 and action4 will be executed from action2's callback. line 126- verifies phase 1, 2 Frontend.runAction were executed. One of action1 and one of action2. line 129- executes action1's callback, the return value that is passed to the callback is 'succeed=false' on the return value, since action1 fails (it should affect the flow since action1 doesn't have actions that depend on it). line 132- executes action2's callback with 'succeed=true' on the return value. line 135- since action2's callback was executed in line 129, now action3 and action4 should have also been executed. So now, the total number of executed 'Frontend.runAction' should be 4. line 138- executes the callbacks action3 and action4. Line 136: Line 137: // executed the callbacks of action3 and action4 Line 138: executeCallbacks(true, flowState, callbacks.subList(callbacks.size() - 2, callbacks.size())); Line 139: Line 164: protected abstract void verifyRunActionAndExecuteCallbacksRandomly(boolean success, Line 165: ActionFlowState flowState, Line 166: int exepectedNumOfRunActionExecutions); Line 167: Line 168: protected abstract List<C> verfifyRunAction(int exepectedNumOfRunActionExecutions); > verfify? :) Done https://gerrit.ovirt.org/#/c/36848/25/frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/action/SyncUiActionTest.java File frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/action/SyncUiActionTest.java: Line 88: public void shouldExecuteTest() { Line 89: UiAction action = new SyncUiAction(model, "test") { //$NON-NLS-1$ Line 90: @Override Line 91: protected void onActionExecute() { Line 92: assertTrue(false); > Maybe call Mockito's fail() instead? Done Line 93: } Line 94: Line 95: @Override Line 96: protected boolean shouldExecute() { Line 121: SimpleAction finalAction = new SimpleAction() { Line 122: Line 123: @Override Line 124: public void execute() { Line 125: // Do nothing > Similarly to the above case, maybe call fail() to also mark this as a piece Done Line 126: } Line 127: }; Line 128: action1.onAllExecutionsFinish(finalAction); Line 129: action2.onAllExecutionsFinish(finalAction); https://gerrit.ovirt.org/#/c/36848/25/frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/action/UiActionBaseTest.java File frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/action/UiActionBaseTest.java: Line 54: int numOfProgressInteractions = shouldCallProgressActions ? 1 : 0; Line 55: verify(model, times(numOfProgressInteractions)).startProgress(any(String.class)); Line 56: verify(model, times(numOfProgressInteractions)).stopProgress(); Line 57: Line 58: UiAction previousAction = null; > The test in this block is actually quite important, and I think should be e The state consistency check is ran on each assertAllDone. assertAllDone is ran when the flow is finished. Each flow (unless there is a bug/exception) should be finished (counter=0), no matter if there were failures or all succeeded. So, in my opinion, the check here is enough. Line 59: for (UiAction action : actions) { Line 60: if (previousAction != null) { Line 61: assertSame(previousAction.getActionFlowState(), action.getActionFlowState()); Line 62: } https://gerrit.ovirt.org/#/c/36848/25/frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/action/UiVdcActionTest.java File frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/action/UiVdcActionTest.java: Line 20: @RunWith(MockitoJUnitRunner.class) Line 21: public class UiVdcActionTest extends AsyncUiActionTest<IFrontendActionAsyncCallback> { Line 22: Line 23: @Test Line 24: public void runNextActionFlowWithFirstActionFailureTest() { > There's a similar test in UiVdcMultipleActionTest - can't they be unified a As I wrote in the answer to the previous comment. UiVdcAction and UiVdcMultipleAction have different behaviour in this case. UiVdcAction- Doesn't run the next action in case of a failure. UiVdcMultipleAction- runs the next action in case of a failure in one of the multiple actions. Line 25: List<UiAction> actions = runActionFlow(ActionType.next); Line 26: ActionFlowState flowState = actions.get(0).getActionFlowState(); Line 27: Line 28: verifyRunActionAndExecuteCallbacksRandomly(false, flowState, 1); Line 32: assertFinishedWithFailures(Collections.singletonList(actions.get(0)), 1); Line 33: } Line 34: Line 35: @Test Line 36: public void runMixedActionFlowWithFailureOnDependentAction() { > Same question. same answer. Line 37: // action1.and(action2).next(action3).and(action4) Line 38: // action1 and action 2 has failure -> action3 and action4 won't be executed Line 39: Line 40: List<UiAction> actions = runActionFlow(ActionType.parallel, ActionType.next, ActionType.parallel); Line 52: action.runAction(); Line 53: Line 54: verifyRunActionAndExecuteCallbacksRandomly(false, action.getActionFlowState(), 1, true); Line 55: Line 56: assertFinishedSuccessfully(Collections.singletonList(action)); > This is weird for two reasons: 1. If the error dialog is shown it means the errors are not collected. Therefore, at the end of the flow there are no errors, as if everything finished successfully. Change the method name to assertFinishedWithNoErros, I hope now it is clearer. 2. I don't. This is a test class for UiVdcAction. I'm not testing the functionality of other classes. Line 57: } Line 58: Line 59: @Override Line 60: protected UiVdcAction createAction() { Line 60: protected UiVdcAction createAction() { Line 61: return new UiVdcAction(ACTION_TYPE, new VdcActionParametersBase(), model); Line 62: } Line 63: Line 64: private UiVdcAction createAction(boolean showErrorDialog) { > You don't actually use the argument here - either rename the method or pass Done Line 65: return new UiVdcAction(ACTION_TYPE, new VdcActionParametersBase(), model, true); Line 66: } Line 67: Line 68: private void verifyRunActionAndExecuteCallbacksRandomly(boolean success, Line 97: } Line 98: Line 99: private List<IFrontendActionAsyncCallback> verfifyRunAction(int exepectedNumOfRunActionExecutions, Line 100: boolean showErrorDialog) { Line 101: verify(frontend, times(exepectedNumOfRunActionExecutions)).runAction(eq(ACTION_TYPE), > What is the benefit of using eq() over passing ACTION_TYPE itself? Same que it is the syntax, it doesn't pass compilation without the eq. Line 102: any(VdcActionParametersBase.class), Line 103: callbackCaptor.capture(), Line 104: eq(showErrorDialog)); Line 105: List<IFrontendActionAsyncCallback> callbacks = callbackCaptor.getAllValues(); https://gerrit.ovirt.org/#/c/36848/25/frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/action/UiVdcMultipleActionTest.java File frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/action/UiVdcMultipleActionTest.java: Line 48: assertFinishedWithFailures(actions.subList(0, 2), 2); Line 49: } Line 50: Line 51: @Test Line 52: public void waitForResultTest() { > Shouldn't this (or another test) verify that a subsequent action is run des Created a common test method this test and runNextActionFlowWithFirstActionFailureTest that receives waitForResult as a parameter. Line 53: UiAction action = createAction(true, true); Line 54: action.runAction(); Line 55: Line 56: verifyRunActionAndExecuteCallbacksRandomly(false, action.getActionFlowState(), 1, true); Line 59: } Line 60: Line 61: @Test Line 62: public void dontRunNextInCaseOfErrorTest() { Line 63: UiAction action1 = createAction(false, false); > Do you actually care whether the first parameter is false or true? Shouldn' Created a common method that receives waitForResult as a parameter. Splitted the test into two. Line 64: UiAction action2 = createAction(); Line 65: action1.then(action2); Line 66: action1.runAction(); Line 67: Line 139: } Line 140: Line 141: private List<IFrontendMultipleActionAsyncCallback> verfifyRunAction(int exepectedNumOfRunActionExecutions, Line 142: boolean waitForResult) { Line 143: verify(frontend, times(exepectedNumOfRunActionExecutions)).runMultipleAction(eq(ACTION_TYPE), > Same questions as in UiVdcAction concerning the usage of eq() here and belo same answer Line 144: anyListOf(VdcActionParametersBase.class), Line 145: callbackCaptor.capture(), Line 146: eq(false), Line 147: eq(waitForResult)); -- To view, visit https://gerrit.ovirt.org/36848 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I262282d00bbb16a65f36a2463d3ffe8dbf6594c6 Gerrit-PatchSet: 25 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Yevgeny Zaspitsky <yzasp...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
