Lior Vernia 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.isStartedProgress() - not the other stuff here like handleErrors() and running of the final action. 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 one isn't run? 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 that since the next action is only called from the first's callback, the original call to verify...() didn't include all those checks. There has to be a better way to write this. 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. 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. 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. 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? :) 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? 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 of code that shouldn't be reached? 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 extracted to a different method, e.g. assertStateConsistent(). I would run it not only if all actions succeeded, but also if there were failures, in which case I'd only care to verify that all succeeded actions share the same state object (why? for example getFlowState() assumes this is the case, even if there were failures). So maybe implement it in a public method and annotate it with @After. This would require you to somehow mark individual actions as succeeded - this can be done by either spying on the tested action objects or extending the actions using special test classes. Or whatever other way you come up with... 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 and moved up into AsyncUiActionTest? Actually related to a comment I put there. 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. 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. You're testing that the error dialog is shown yet you expect no errors? 2. Where do you actually verify that the mocked frontend tried to open an error dialog or something of the sort? It's quite possible that this does happen but I didn't research deep enough, just wanted to make sure it happens. 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 showErrorDialog to the UiVdcAction constructor instead of true. 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 question about showErrorDialog below. 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 despite of the failure? 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't the same behavior be expected whether the failure is on canDoAction() or upon execution? If so, you can pass a random boolean instead. 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 below. 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: 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
