In the spirit of retaining old reviews, will naming the attachment files accounting the review number (ex: fix-624-bug-1, fix-624-bug-2, ...) help? (Sorry, I am new to all this.)
On Thu, Oct 9, 2008 at 12:08 PM, John Hjelmstad <[EMAIL PROTECTED]> wrote: > Thanks for the reminder, Evan. Due to limitations with the code review > tool, I had to delete the old reviews as new patches came in. The last, > applied review was:http://codereview.appspot.com/7447/show > > --John > > > On Thu, Oct 9, 2008 at 12:02 PM, Evan Gilbert <[EMAIL PROTECTED]> wrote: > >> Not sure if it is purposeful, but this review is no longer available for >> viewing. Even if we upload a new patch or commit, this is still useful for >> people looking back at the patch review. >> >> Evan >> >> >> On Wed, Oct 8, 2008 at 1:14 PM, <[EMAIL PROTECTED]> wrote: >> >>> Thanks for the thorough review. New file attached to the issue. >>> >>> >>> 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: } >>> On 2008/10/07 23:44:13, johnfargo wrote: >>> >>>> space between methods and between members and ctor >>>> >>> >>> Done. >>> >>> 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"); >>> On 2008/10/07 23:44:13, johnfargo wrote: >>> >>>> 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); >>>> >>> >>> Done. Should be all fixed now. >>> >>> http://codereview.appspot.com/7441/diff/1/2#newcode90 >>> Line 90: int httpStatusCode) throws Exception { >>> On 2008/10/07 23:44:13, johnfargo wrote: >>> >>>> args on the first line, wrapped 4-indented after >>>> >>> >>> Done. >>> >>> http://codereview.appspot.com/7441/diff/1/2#newcode97 >>> Line 97: EasyMock.replay(request, response, handler, writer); >>> On 2008/10/07 23:44:13, johnfargo wrote: >>> >>>> stylistically we import static on these eg. >>>> import static ...EasyMock.expect; >>>> >>> >>> Done. >>> >>> http://codereview.appspot.com/7441/diff/1/2#newcode106 >>> Line 106: int httpStatusCode) throws Exception { >>> On 2008/10/07 23:44:13, johnfargo wrote: >>> >>>> args on the first line, wrapped 4-indented after >>>> >>> >>> Done. >>> >>> http://codereview.appspot.com/7441/diff/1/2#newcode137 >>> Line 137: String contentType, int httpStatusCode) throws IOException { >>> On 2008/10/07 23:44:13, johnfargo wrote: >>> >>>> This can be called createHttpResponse (and perhaps used for POST tests >>>> >>> later) >>> >>> Done. >>> >>> >>> http://codereview.appspot.com/7441 >>> >> >> >
