Greg Sheremeta has posted comments on this change.
Change subject: userportal,webadmin: Frontend refactor[WIP]
......................................................................
Patch Set 5: I would prefer that you didn't submit this
(18 inline comments)
lots of nitpicks, but I do have 1 over-testing concern. Lmk what you think!
Great work.
....................................................
File
frontend/webadmin/modules/frontend/src/test/java/org/ovirt/engine/ui/frontend/FrontendActionTest.java
Line 308: * Run the following test case.
Line 309: * <ol>
Line 310: * <li>Run a single action</li>
Line 311: * <li>Return success.</li>
Line 312: * <li>Check to make sure the failure event is fired</li>
I think you mean "success"
Line 313: * <li>Check to make sure the callback is called</li>
Line 314: * </ol>
Line 315: */
Line 316: @Test
Line 401: * </ol>
Line 402: * Test just the handler method.
Line 403: */
Line 404: @Test
Line 405: public void
testHandleActionResult_isRaiseErrorModalPanel_actionMessageSizeSmallerEq1() {
I'm not sure what "actionMessageSizeSmallerEq1" means. Could that be explained
in the javadoc comment? Or perhaps rename.
Line 406: VdcFault testFault = new VdcFault();
Line 407:
when(mockEventsHandler.isRaiseErrorModalPanel(VdcActionType.AddDisk,
testFault)).thenReturn(true);
Line 408: Object testState = new Object();
Line 409: VdcActionParametersBase testParameters = new
VdcActionParametersBase();
Line 439: * </ol>
Line 440: * Test just the handler method.
Line 441: */
Line 442: @Test
Line 443: public void
testHandleActionResult_isRaiseErrorModalPanel_actionMessageSize1() {
suggest "withActionMessageSize1"
Line 444: VdcFault testFault = new VdcFault();
Line 445:
when(mockEventsHandler.isRaiseErrorModalPanel(VdcActionType.AddDisk,
testFault)).thenReturn(true);
Line 446: Object testState = new Object();
Line 447: VdcActionParametersBase testParameters = new
VdcActionParametersBase();
Line 478: * </ol>
Line 479: * Test just the handler method.
Line 480: */
Line 481: @Test
Line 482: public void
testHandleActionResult_isRaiseErrorModalPanel_actionMessageSizeGreater1() {
suggest "withActionMessageSizeGreaterThan1"
Line 483: VdcFault testFault = new VdcFault();
Line 484:
when(mockEventsHandler.isRaiseErrorModalPanel(VdcActionType.AddDisk,
testFault)).thenReturn(true);
Line 485: Object testState = new Object();
Line 486: VdcActionParametersBase testParameters = new
VdcActionParametersBase();
Line 515: * <li>Check to make sure the callback is called</li>
Line 516: * </ol>
Line 517: */
Line 518: @Test
Line 519: public void testRunMultipleActions_oneaction() {
nit: you use "1" instead of "one" in the above tests
Line 520: List<VdcActionType> actionTypes = new
ArrayList<VdcActionType>();
Line 521: actionTypes.add(VdcActionType.AddDisk);
Line 522: List<VdcActionParametersBase> testParameters = new
ArrayList<VdcActionParametersBase>();
Line 523: testParameters.add(new VdcActionParametersBase());
Line 565: returnValue.setSucceeded(true);
Line 566: callbackAction.getValue().onSuccess(returnValue);
Line 567: verify(mockActionCallback).executed(callbackParam.capture());
Line 568: assertEquals(callbackParam.getValue().getReturnValue(),
returnValue);
Line 569: assertEquals("List size should be 1", 1, actionTypes.size());
//$NON-NLS-1$
I see in RunMultipleActions that these 3 arrays get the first element removed,
and then it recurses. But is that popping mechanism important to test? I'm
concerned it might be over-testing. When you refactor, I could totally see you
changing that implementation detail.
>From the javadoc comment, the point of this test is just to make sure the 2
>callbacks are called. So I think I'd limit the test to just that.
I suppose this thought applies to all the RunMultiple tests.
Line 570: assertEquals("List size should be 1", 1,
testParameters.size()); //$NON-NLS-1$
Line 571: assertEquals("List size should be 1", 1, callbacks.size());
//$NON-NLS-1$
Line 572: //Second call to RunAction, the size of the parameters should
have decreased
Line 573:
verify(mockService).RunAction(eq(VdcActionType.AddBricksToGlusterVolume),
eq(testParameters.get(0)),
Line 586: /**
Line 587: * Run the following test case.
Line 588: * <ol>
Line 589: * <li>Run MultipleActions with multiple actions, first
success, and second failure.</li>
Line 590: * <li>Check to make sure the callback is called</li>
This comment is a little unclear w/r/t the difference in callback checks
between the first and second actions. Should it mention both callbacks?
Line 591: * </ol>
Line 592: */
Line 593: @Test
Line 594: public void
testRunMultipleActions_multipleaction_success_first_success_second_failure() {
Line 631: /**
Line 632: * Run the following test case.
Line 633: * <ol>
Line 634: * <li>Run MultipleActions with multiple actions, first
failure, and second success.</li>
Line 635: * <li>Check to make sure the callback is called</li>
the ^first^ callback
Line 636: * <li>Make sure the second is never called</li>
Line 637: * </ol>
Line 638: */
Line 639: @Test
Line 748: /**
Line 749: * Run the following test case.
Line 750: * <ol>
Line 751: * <li>Attempt to login</li>
Line 752: * <li>Have the login succeed</li>
er, have the login fail
Line 753: * <li>Check to make sure the callback is called</li>
Line 754: * <li>Check to make sure the events handler is called</li>
Line 755: * <li>Make sure the frontend failure handler is called</li>
Line 756: * </ol>
Line 751: * <li>Attempt to login</li>
Line 752: * <li>Have the login succeed</li>
Line 753: * <li>Check to make sure the callback is called</li>
Line 754: * <li>Check to make sure the events handler is called</li>
Line 755: * <li>Make sure the frontend failure handler is called</li>
is ^not^ called
Line 756: * </ol>
Line 757: */
Line 758: @Test
Line 759: public void testLoginAsync_login_failure() {
....................................................
File
frontend/webadmin/modules/frontend/src/test/java/org/ovirt/engine/ui/frontend/FrontendTest.java
Line 263: * Run the following test case.
Line 264: * <ol>
Line 265: * <li>Run a search query, with *win* as the parameter,
searching for VMs</li>
Line 266: * <li>The callback is NOT marked to handle failures</li>
Line 267: * <li>The failure is NOT, a not logged in failure</li>
I would hyphenate like so: "NOT a not-logged-in failure"
Line 268: * <li>Return success, but the success status is !succeeded
(business logic failure/not logged in)</li>
Line 269: * <li>Check to make sure the appropriate query start and query
complete events are fired</li>
Line 270: * </ol>
Line 271: */
Line 299: Frontend.RunQuery(VdcQueryType.Search, testParameters,
mockService, mockAsyncQuery);
Line 300: verify(mockService).RunQuery(eq(VdcQueryType.Search),
eq(testParameters), callback.capture());
Line 301: VdcQueryReturnValue mockReturnValue = new
VdcQueryReturnValue();
Line 302: mockReturnValue.setExceptionString("USER_IS_NOT_LOGGED_IN");
//$NON-NLS-1$
Line 303: //Return value set to failure
nit: // spaces after slashes
Line 304: mockReturnValue.setSucceeded(false);
Line 305: callback.getValue().onSuccess(mockReturnValue);
Line 306: //Make sure the not logged in event is called
Line 307: verify(mockFrontendNotLoggedInEvent).raise(Frontend.class,
EventArgs.Empty);
Line 382: VdcQueryReturnValue mockReturnValue = new
VdcQueryReturnValue();
Line 383: mockReturnValue.setReturnValue(mockResultModel);
Line 384: mockReturnValue.setExceptionString("USER_IS_NOT_LOGGED_IN");
//$NON-NLS-1$
Line 385: when(mockConverter.Convert(mockResultModel,
mockAsyncQuery)).thenReturn(mockConvertedModel);
Line 386: //Return value set to failure
i believe it's set to success
Line 387: mockReturnValue.setSucceeded(true);
Line 388: callback.getValue().onSuccess(mockReturnValue);
Line 389:
verify(mockAsyncQuery).setOriginalReturnValue(mockReturnValue);
Line 390: verify(mockAsyncCallback).onSuccess(mockModel,
mockConvertedModel);
Line 410: Frontend.RunQuery(VdcQueryType.Search, testParameters,
mockService, mockAsyncQuery);
Line 411: verify(mockService).RunQuery(eq(VdcQueryType.Search),
eq(testParameters), callback.capture());
Line 412: VdcQueryReturnValue mockReturnValue = new
VdcQueryReturnValue();
Line 413: mockReturnValue.setExceptionString("USER_IS_NOT_LOGGED_IN");
//$NON-NLS-1$
Line 414: //Return value set to failure
s/failure/success/
Line 415: mockReturnValue.setSucceeded(true);
Line 416: callback.getValue().onSuccess(mockReturnValue);
Line 417: verify(mockAsyncCallback).onSuccess(mockModel,
mockReturnValue);
Line 418: reset(mockAsyncCallback);
Line 439: mockService);
Line 440: verify(mockService).RunMultipleQueries(eq(queryTypeList),
eq(queryParamsList), callbackMultipleQueries.capture());
Line 441: StatusCodeException exception = new StatusCodeException(0, "0
status code"); //$NON-NLS-1$
Line 442: // Call the failure handler.
Line 443: callbackMultipleQueries.getValue().onFailure(exception);
should there be an assertion at the end here?
Line 444: }
Line 445:
Line 446: /**
Line 447: * Run the following test case.
Line 463: mockService);
Line 464: verify(mockService).RunMultipleQueries(eq(queryTypeList),
eq(queryParamsList), callbackMultipleQueries.capture());
Line 465: StatusCodeException exception = new StatusCodeException(0, "0
status code"); //$NON-NLS-1$
Line 466: // Call the failure handler.
Line 467: callbackMultipleQueries.getValue().onFailure(exception);
should there be an assertion at the end here?
Line 468: }
Line 469:
Line 470: /**
Line 471: * Run the following test case.
Line 469:
Line 470: /**
Line 471: * Run the following test case.
Line 472: * <ol>
Line 473: * <li>Run a multiple search query, with only multiple
requests, with *win* as the parameter, searching for VMs</li>
nit: one of the params is lin, not win
Line 474: * <li>Force a special failure with an HTTP status code =
404</li>
Line 475: * <li>Check to make sure the appropriate query start and query
complete events are fired</li>
Line 476: * </ol>
Line 477: */
Line 470: /**
Line 471: * Run the following test case.
Line 472: * <ol>
Line 473: * <li>Run a multiple search query, with only multiple
requests, with *win* as the parameter, searching for VMs</li>
Line 474: * <li>Force a special failure with an HTTP status code =
404</li>
s/special//
Line 475: * <li>Check to make sure the appropriate query start and query
complete events are fired</li>
Line 476: * </ol>
Line 477: */
Line 478: @Test
--
To view, visit http://gerrit.ovirt.org/15399
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3dae5a455ad16660b97cddafba53b53d3b4196f1
Gerrit-PatchSet: 5
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alexander Wels <[email protected]>
Gerrit-Reviewer: Alexander Wels <[email protected]>
Gerrit-Reviewer: Greg Sheremeta <[email protected]>
Gerrit-Reviewer: Vojtech Szocs <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
