You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shindig.apache.org by jo...@gmail.com on 2008/10/08 01:44:13 UTC

Re: SHINDIG-624: JSONP support for /gadgets/metadata

Just a few more. Just a few style nits and a suggestion on Test
verification.


http://codereview.appspot.com/7441/diff/1/3
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/RpcServlet.java
(right):

http://codereview.appspot.com/7441/diff/1/3#newcode155
Line 155: }
space between methods and between members and ctor

http://codereview.appspot.com/7441/diff/1/2
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/RpcServletTest.java
(right):

http://codereview.appspot.com/7441/diff/1/2#newcode74
Line 74: assertDoGetWithoutHandler(null, "function");
It looks like these don't actually perform the test you want -- you'll
need to run EasyMock.verify(response) on the ServletResponse object
you're creating for each test.

You could probably unroll the test helper methods too, since they're not
repeatedly used too much. Thus testDoGetNormal would look like:
request = createMockRequest(req, callback);
response = createExpectedResponse(string, code);
expect(handler.stuffHappens);
servlet.doGet(req, response);
verify(response);

http://codereview.appspot.com/7441/diff/1/2#newcode90
Line 90: int httpStatusCode) throws Exception {
args on the first line, wrapped 4-indented after

http://codereview.appspot.com/7441/diff/1/2#newcode97
Line 97: EasyMock.replay(request, response, handler, writer);
stylistically we import static on these eg.
import static ...EasyMock.expect;

http://codereview.appspot.com/7441/diff/1/2#newcode106
Line 106: int httpStatusCode) throws Exception {
args on the first line, wrapped 4-indented after

http://codereview.appspot.com/7441/diff/1/2#newcode137
Line 137: String contentType, int httpStatusCode) throws IOException {
This can be called createHttpResponse (and perhaps used for POST tests
later)

http://codereview.appspot.com/7441