> On Aug. 10, 2015, 8:34 a.m., Dian Fu wrote: > > test/src/test/java/org/apache/sqoop/integration/server/InvalidRESTCallsTest.java, > > line 63 > > <https://reviews.apache.org/r/37261/diff/1/?file=1035287#file1035287line63> > > > > I think it would be better if we can move method "assertResponseCode" > > out of class Validator.
I've put validation methods inside the Validator class because they are specific to the validator's instance - they are depending on the internal state of the object (HttpURL Connection for example and in the future on error/input streams). My goal here was to allow as simple implementation of the Validator class as possible, so that the testcases themselves are simple to read and follow. So when you look at line 98 for example, I have only: assertResponseCode(405); And the method is smart enough to go to the read the current response code internally from the instance. My earlier iterations of the code was: assertEquals(connection.getResponseCode(), 405); Which at least for me is much harder to read, especially if we will need to repeat this in every single test :-/ Having said that, would you agree to keep the method in the Validator object? > On Aug. 10, 2015, 8:34 a.m., Dian Fu wrote: > > test/src/test/java/org/apache/sqoop/integration/server/InvalidRESTCallsTest.java, > > line 134 > > <https://reviews.apache.org/r/37261/diff/1/?file=1035287#file1035287line134> > > > > Should be "wr.writeBytes(byteData)" instead of > > "wr.writeBytes(desc.data);" It's me being a bit lazy. The method writeBytes() actually doesn't accept byte[] but a String. Hence using desc.data instead. I'll do it properly and use write(byte[], len, offset), so that it's clear. - Jarek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37261/#review94710 ----------------------------------------------------------- On Aug. 8, 2015, 9:48 p.m., Jarek Cecho wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37261/ > ----------------------------------------------------------- > > (Updated Aug. 8, 2015, 9:48 p.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-2469 > https://issues.apache.org/jira/browse/SQOOP-2469 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > Added infrastructure to run arbitrary REST requests with two small tests. > Interestingly enough doing POST to /version leads to tomcat response rather > then Sqoop response which we will want to fix in future patch - I want to > keep this one simple. > > > Diffs > ----- > > > test/src/test/java/org/apache/sqoop/integration/server/InvalidRESTCallsTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/37261/diff/ > > > Testing > ------- > > The test case is passing. > > > Thanks, > > Jarek Cecho > >
