Allon Mureinik has posted comments on this change.
Change subject: userportal, webadmin: Added caching
......................................................................
Patch Set 11: I would prefer that you didn't submit this
(16 inline comments)
I did not review the frontend changes (not my forte), but I do have some
comments:
1. GwtDynamicHostPageServletTest is a problematic name for an abstract class
(see inline). This is also the only reason for my -1. The other comments are
style comments you can either take or not,
2. General: all the assertXYZ methods should use the overloaded method which
provides a description string for the failure,
3. General: all members of the test cases should be private. Mockito handles
this fine, and no reason to over-expose members.
4. General: this patch contains several cleanups that seem unrelated to it -
perhaps they should be extracted to another patch?
....................................................
File backend/manager/modules/utils/pom.xml
Line 15
Line 16
Line 17
Line 18
Line 19
Why is this necessary for this patch? can't this be done separately?
....................................................
File
frontend/webadmin/modules/frontend-overlay/src/test/java/org/ovirt/engine/ui/frontend/server/gwt/GwtCachingFilterTest.java
Line 31: @RunWith(MockitoJUnitRunner.class)
Line 32: public class GwtCachingFilterTest {
Line 33:
Line 34: @Mock
Line 35: HttpServletRequest mockRequest;
All these members should be private
Line 36:
Line 37: @Mock
Line 38: HttpServletResponse mockResponse;
Line 39:
....................................................
File
frontend/webadmin/modules/frontend-overlay/src/test/java/org/ovirt/engine/ui/frontend/server/gwt/GwtDynamicHostPageServletTest.java
Line 37: import org.ovirt.engine.core.common.queries.VdcQueryType;
Line 38: import org.ovirt.engine.core.common.users.VdcUser;
Line 39: import org.ovirt.engine.core.compat.Guid;
Line 40:
Line 41: public abstract class GwtDynamicHostPageServletTest<T extends
GwtDynamicHostPageServlet> {
Please rename this class.
Some JUnit runners aren't as smart as they ought to be, and assume that if a
class ends with the word "Test" its a test case - and then fail running it
because its abstract.
GwtDynamicHostPageServletTestCase would be fine, IMHO.
Line 42:
Line 43: protected static final String SELECTOR_SCRIPT =
"myapp.nocache.js"; //$NON-NLS-1$
Line 44:
Line 45: @Mock
Line 39: import org.ovirt.engine.core.compat.Guid;
Line 40:
Line 41: public abstract class GwtDynamicHostPageServletTest<T extends
GwtDynamicHostPageServlet> {
Line 42:
Line 43: protected static final String SELECTOR_SCRIPT =
"myapp.nocache.js"; //$NON-NLS-1$
why not private?
Line 44:
Line 45: @Mock
Line 46: HttpServletRequest mockRequest;
Line 47:
Line 42:
Line 43: protected static final String SELECTOR_SCRIPT =
"myapp.nocache.js"; //$NON-NLS-1$
Line 44:
Line 45: @Mock
Line 46: HttpServletRequest mockRequest;
All these members should be private
Line 47:
Line 48: @Mock
Line 49: HttpServletResponse mockResponse;
Line 50:
....................................................
File
frontend/webadmin/modules/frontend-overlay/src/test/java/org/ovirt/engine/ui/frontend/server/gwt/WebAdminHostPageServletTest.java
Line 37:
Line 38: private static final String APPLICATION_MODE = "{ \"value\":
\"123\" }"; //$NON-NLS-1$
Line 39:
Line 40: @Mock
Line 41: ObjectNode mockApplicationModeObject;
All these data members should be private.
Line 42:
Line 43: // Cannot use @Mock since ArrayNode is final
Line 44: ArrayNode mockPluginDefinitionsArray;
Line 45:
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/gwtservices/GenericApiGWTService.java
Line 36:
Line 37: public VdcReturnValueBase logOff(VdcUser userToLogoff);
Line 38:
Line 39: public VdcReturnValueBase Login(String user, String password,
String domain);
Line 40:
why?
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/AbstractGWTServiceImpl.java
Line 1
Line 2
Line 3
Line 4
Line 5
Like this cleanup, but is it related to this patch?
....................................................
File frontend/webadmin/modules/userportal-gwtp/pom.xml
Line 163
Line 164
Line 165
Line 166
Line 167
why is this cleanup related to the patch?
Line 168
Line 169
Line 170
Line 171
Line 172
why is this cleanup related to the patch?
Line 182
Line 183
Line 184
Line 185
Line 186
why is this cleanup related to the patch?
Line 187
Line 188
Line 189
Line 190
Line 191
why is this cleanup related to the patch?
....................................................
File frontend/webadmin/modules/webadmin/pom.xml
Line 163
Line 164
Line 165
Line 166
Line 167
why is this cleanup related to the patch?
Line 168
Line 169
Line 170
Line 171
Line 172
why is this cleanup related to the patch?
Line 182
Line 183
Line 184
Line 185
Line 186
why is this cleanup related to the patch?
Line 187
Line 188
Line 189
Line 190
Line 191
why is this cleanup related to the patch?
--
To view, visit http://gerrit.ovirt.org/10449
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5d8e02ae542a4aa37bd421bde5582c0f3e9820ad
Gerrit-PatchSet: 11
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alexander Wels <[email protected]>
Gerrit-Reviewer: Alexander Wels <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Einav Cohen <[email protected]>
Gerrit-Reviewer: Gilad Chaplik <[email protected]>
Gerrit-Reviewer: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Vojtech Szocs <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
