> On Nov. 19, 2015, 6:07 p.m., Abraham Fine wrote: > > test/src/test/java/org/apache/sqoop/integration/server/rest/ConnectorRestTest.java, > > line 28 > > <https://reviews.apache.org/r/40419/diff/2/?file=1128982#file1128982line28> > > > > could we use `type[] arrayName` instead of `type []arrayName`
I don't think that we have that defined anywhere, but I'm open to apply such change. > On Nov. 19, 2015, 6:07 p.m., Abraham Fine wrote: > > test/src/test/java/org/apache/sqoop/integration/server/rest/ConnectorRestTest.java, > > line 29 > > <https://reviews.apache.org/r/40419/diff/2/?file=1128982#file1128982line29> > > > > we could put the HTTP verbs in an enum to prevent typos For the purpose of the REST-related tests I want to keep String as I actually want to try to see how the server behaves when we send non-existing HTTP methods (future exercise). In the production (non-test) code we're already using HttpMethod interface [1] as an "enum". Links: 1: https://docs.oracle.com/javaee/6/api/javax/ws/rs/HttpMethod.html > On Nov. 19, 2015, 6:07 p.m., Abraham Fine wrote: > > test/src/test/java/org/apache/sqoop/integration/server/rest/LinkRestTest.java, > > line 27 > > <https://reviews.apache.org/r/40419/diff/2/?file=1128983#file1128983line27> > > > > we still need more test cases (ex handling creation of links) but as > > per our discussion that will be handled in another patch Correct, this JIRA is really just splitting existing tests into standalone test cases. I'm still due to add a lot of various tests. > On Nov. 19, 2015, 6:07 p.m., Abraham Fine wrote: > > test/src/test/java/org/apache/sqoop/integration/server/rest/LinkRestTest.java, > > line 40 > > <https://reviews.apache.org/r/40419/diff/2/?file=1128983#file1128983line40> > > > > likely outside the scope of this jira, but assertcontains does not seem > > all that rigorous to me. we should have tests to validate the structure of > > the returned json. I do not want to necesarily dive into entire JSON structure here, especially not on "correct" case as those tests are elsewhere. I want more to focus on error conditions. But I guess that we can improve the test a bit to ensure that teh first-link here is indeed part of structure that should be a valid connector. Let's do that in separate JIRA. > On Nov. 19, 2015, 6:07 p.m., Abraham Fine wrote: > > test/src/test/java/org/apache/sqoop/integration/server/rest/LinkRestTest.java, > > line 72 > > <https://reviews.apache.org/r/40419/diff/2/?file=1128983#file1128983line72> > > > > is there a way we could avoid repeating this chunk of code? Sadly no, the problem is that the provider have to be static and JAVA doesn't allow one to inherit not to override static methods. > On Nov. 19, 2015, 6:07 p.m., Abraham Fine wrote: > > test/src/test/java/org/apache/sqoop/integration/server/rest/RestTest.java, > > line 122 > > <https://reviews.apache.org/r/40419/diff/2/?file=1128984#file1128984line122> > > > > this should be in the `LinkRestTest` class right? Nope, the method is in RestTest class (parent method) and LinkTest only calls it if/when needed. This is intentional because the method will be needed by multiple children - even though that right now it's only LinkTest, I'll add JobTest in the future that will need it as well. > On Nov. 19, 2015, 6:07 p.m., Abraham Fine wrote: > > test/src/test/java/org/apache/sqoop/integration/server/rest/RestTest.java, > > line 137 > > <https://reviews.apache.org/r/40419/diff/2/?file=1128984#file1128984line137> > > > > can we generalize this so it does not need to know specifically what it > > needs to delete? Yes, I'll improve this fragment in next patch iteration. - Jarek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40419/#review107207 ----------------------------------------------------------- On Nov. 18, 2015, 1:14 a.m., Jarek Cecho wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40419/ > ----------------------------------------------------------- > > (Updated Nov. 18, 2015, 1:14 a.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-2698 > https://issues.apache.org/jira/browse/SQOOP-2698 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > Splitted the current state of the test as per the recommendation on JIRA. > > > Diffs > ----- > > > test/src/test/java/org/apache/sqoop/integration/server/InvalidRESTCallsTest.java > 40d8c88 > > test/src/test/java/org/apache/sqoop/integration/server/rest/ConnectorRestTest.java > PRE-CREATION > > test/src/test/java/org/apache/sqoop/integration/server/rest/LinkRestTest.java > PRE-CREATION > test/src/test/java/org/apache/sqoop/integration/server/rest/RestTest.java > PRE-CREATION > > test/src/test/java/org/apache/sqoop/integration/server/rest/VersionRestTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/40419/diff/ > > > Testing > ------- > > All new test cases seems to be passing. > > > Thanks, > > Jarek Cecho > >
