Vojtech Szocs has posted comments on this change.
Change subject: userportal, webadmin: Added caching
......................................................................
Patch Set 11: (18 inline comments)
Thanks Allon for your review, I've replied to all inline comments, here are
some additional comments:
> 1. GwtDynamicHostPageServletTest is a problematic name for an abstract class
> (see inline). This is also the only reason for my -1.
Agreed, renamed to AbstractGwtDynamicHostPageServletTest to indicate this is
just an abstract base class for concrete test classes.
> 2. General: all the assertXYZ methods should use the overloaded method which
> provides a description string for the failure
Yes, however I think assertions aren't that complex, e.g. assertThat(result,
is(Integer.class)). Complex assertions should be split into simple ones, which
are self-documenting.
> 3. General: all members of the test cases should be private. Mockito handles
> this fine, and no reason to over-expose members.
Agreed, however I think OOP encapsulation principle implemented via access
modifiers isn't really relevant for fields within test classes.
> 4. General: this patch contains several cleanups that seem unrelated to it -
> perhaps they should be extracted to another patch?
Some are reverted, but some are retained because of changes in root pom.xml
(they are logically related to changes in root pom).
....................................................
File backend/manager/modules/utils/pom.xml
Line 15
Line 16
Line 17
Line 18
Line 19
The reason is to define dependency versions via <dependencyManagement> in root
pom and use them consistently in sub-modules.
Yes, this could be done separately, but I prefer to do this now since the root
pom change in this patch already sets up the corresponding dependency version.
....................................................
File
frontend/webadmin/modules/frontend-overlay/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/GwtCachingFilter.java
Line 143: protected String getYesterdayHttpDate(Calendar calendar) {
Line 144: // Subtract a day, so it is in the past.
Line 145: calendar.add(Calendar.DAY_OF_MONTH, -1);
Line 146: // Format in the correct format.
Line 147: SimpleDateFormat dateFormat = new
SimpleDateFormat(HTTP_DATE_FORMAT, Locale.US);
Yes, makes sense, will update the patch.
Line 148: dateFormat.setTimeZone(TimeZone.getTimeZone(GMT));
Line 149: return dateFormat.format(calendar.getTime());
Line 150: }
Line 151:
....................................................
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;
OK, will update the patch.
However, I think OOP encapsulation principle implemented via access modifiers
isn't really relevant for fields within test classes.
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> {
I see your point.
Will rename this class to AbstractGwtDynamicHostPageServletTest, to indicate
this is just an abstract base class for concrete test classes.
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$
Because it's used in WebAdminHostPageServletTest
testGetMd5Digest_WithExtraObjects_WithoutUserInfoObject() method.
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;
Some of them are used in concrete test classes, so some of them need to be
protected. The rest will be private.
However, I think OOP encapsulation principle implemented via access modifiers
isn't really relevant for fields within test classes.
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;
OK, will update the patch.
However, I think OOP encapsulation principle implemented via access modifiers
isn't really relevant for fields within test classes.
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:
Will revert this change.
....................................................
File frontend/webadmin/modules/userportal-gwtp/pom.xml
Line 163
Line 164
Line 165
Line 166
Line 167
You are right, will revert this change, can be done in a later patch.
Line 168
Line 169
Line 170
Line 171
Line 172
Same as above.
Line 182
Line 183
Line 184
Line 185
Line 186
Same as above.
Line 187
Line 188
Line 189
Line 190
Line 191
Same as above.
....................................................
File frontend/webadmin/modules/webadmin/pom.xml
Line 163
Line 164
Line 165
Line 166
Line 167
You are right, will revert this change, can be done in a later patch.
Line 168
Line 169
Line 170
Line 171
Line 172
Same as above.
Line 182
Line 183
Line 184
Line 185
Line 186
Same as above.
Line 187
Line 188
Line 189
Line 190
Line 191
Same as above.
....................................................
File frontend/webadmin/modules/webadmin/src/main/webapp/WEB-INF/web.xml
Line 5:
Line 6: <filter-mapping>
Line 7: <filter-name>GwtCachingFilter</filter-name>
Line 8: <url-pattern>/webadmin/*</url-pattern>
Line 9: </filter-mapping>
Yes, the general idea behind this is web fragment which defines common
filters/servlets, and concrete WARs which put selected filters/servlets into
use via mappings.
Using servlet 3.0 annotations is not possible with common filters/servlets,
since URL pattern attribute is required with such annotations. We would have to
override such URL via web.xml + blacklist unwanted servlets via
<security-constraint> - both of which aren't good ways to go.
Line 10:
Line 11: <servlet-mapping>
Line 12: <servlet-name>WebAdminHostPageServlet</servlet-name>
Line 13: <url-pattern>/webadmin/WebAdmin.html</url-pattern>
Line 12: <servlet-name>WebAdminHostPageServlet</servlet-name>
Line 13: <url-pattern>/webadmin/WebAdmin.html</url-pattern>
Line 14: </servlet-mapping>
Line 15:
Line 16: <servlet-mapping>
Please see my comment above.
Line 17: <servlet-name>GenericApiServlet</servlet-name>
Line 18:
<url-pattern>/webadmin/GenericApiGWTService</url-pattern>
Line 19: </servlet-mapping>
Line 20:
--
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
