You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Jarek Cecho <ja...@apache.org> on 2015/08/08 23:48:15 UTC
Review Request 37261: SQOOP-2469 Sqoop2: Provide base integration
test that
will run custom HTTP requests and validating the server response
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37261/
-----------------------------------------------------------
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
Re: Review Request 37261: SQOOP-2469 Sqoop2: Provide base integration
test
that will run custom HTTP requests and validating the server response
Posted by Dian Fu <di...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37261/#review95213
-----------------------------------------------------------
Ship it!
+1.
- Dian Fu
On Aug. 12, 2015, 8:20 p.m., Jarek Cecho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37261/
> -----------------------------------------------------------
>
> (Updated Aug. 12, 2015, 8:20 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
>
>
Re: Review Request 37261: SQOOP-2469 Sqoop2: Provide base integration
test
that will run custom HTTP requests and validating the server response
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37261/
-----------------------------------------------------------
(Updated Aug. 12, 2015, 8:20 p.m.)
Review request for Sqoop.
Changes
-------
Incorporated Dian's feedback.
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 (updated)
-----
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
Re: Review Request 37261: SQOOP-2469 Sqoop2: Provide base integration
test
that will run custom HTTP requests and validating the server response
Posted by Dian Fu <di...@gmail.com>.
> On Aug. 10, 2015, 8:34 a.m., Dian Fu wrote:
> >
Seems cool! Just a few minor comments.
- Dian
-----------------------------------------------------------
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
>
>
Re: Review Request 37261: SQOOP-2469 Sqoop2: Provide base integration
test
that will run custom HTTP requests and validating the server response
Posted by Jarek Cecho <ja...@apache.org>.
> 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
>
>
Re: Review Request 37261: SQOOP-2469 Sqoop2: Provide base integration
test
that will run custom HTTP requests and validating the server response
Posted by Dian Fu <di...@gmail.com>.
> 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.
>
> Jarek Cecho wrote:
> 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?
Make sense to me. :)
- Dian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37261/#review94710
-----------------------------------------------------------
On Aug. 12, 2015, 8:20 p.m., Jarek Cecho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37261/
> -----------------------------------------------------------
>
> (Updated Aug. 12, 2015, 8:20 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
>
>
Re: Review Request 37261: SQOOP-2469 Sqoop2: Provide base integration
test
that will run custom HTTP requests and validating the server response
Posted by Dian Fu <di...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37261/#review94710
-----------------------------------------------------------
test/src/test/java/org/apache/sqoop/integration/server/InvalidRESTCallsTest.java (line 45)
<https://reviews.apache.org/r/37261/#comment149326>
objec should be object.
test/src/test/java/org/apache/sqoop/integration/server/InvalidRESTCallsTest.java (line 63)
<https://reviews.apache.org/r/37261/#comment149327>
I think it would be better if we can move method "assertResponseCode" out of class Validator.
test/src/test/java/org/apache/sqoop/integration/server/InvalidRESTCallsTest.java (line 134)
<https://reviews.apache.org/r/37261/#comment149328>
Should be "wr.writeBytes(byteData)" instead of "wr.writeBytes(desc.data);"
- Dian Fu
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
>
>