You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Colin Ma <ju...@intel.com> on 2015/09/23 09:26:55 UTC
Re: Review Request 38592: SQOOP-2553: Sqoop2: Jetty: Update testcases
using Jetty instead of Tomcat
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38592/
-----------------------------------------------------------
(Updated Sept. 23, 2015, 7:26 a.m.)
Review request for Sqoop.
Summary (updated)
-----------------
SQOOP-2553: Sqoop2: Jetty: Update testcases using Jetty instead of Tomcat
Repository: sqoop-sqoop2
Description
-------
Update testcases using Jetty instead of Tomcat
Diffs (updated)
-----
test/pom.xml 3e11f59
test/src/main/java/org/apache/sqoop/test/infrastructure/providers/SqoopInfrastructureProvider.java 5b4f595
test/src/main/java/org/apache/sqoop/test/minicluster/JettySqoopMiniCluster.java PRE-CREATION
test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java 4452558
test/src/main/java/org/apache/sqoop/test/testcases/JettyTestCase.java PRE-CREATION
test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java 666749b
test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgradeTest.java 1d5b692
Diff: https://reviews.apache.org/r/38592/diff/
Testing
-------
Thanks,
Colin Ma
Re: Review Request 38592: SQOOP-2553: Sqoop2: Jetty: Update testcases
using Jetty instead of Tomcat
Posted by Colin Ma <ju...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38592/
-----------------------------------------------------------
(Updated Sept. 25, 2015, 7:13 a.m.)
Review request for Sqoop.
Repository: sqoop-sqoop2
Description
-------
Update testcases using Jetty instead of Tomcat
Diffs (updated)
-----
test/pom.xml 3e11f59
test/src/main/java/org/apache/sqoop/test/infrastructure/providers/SqoopInfrastructureProvider.java 5b4f595
test/src/main/java/org/apache/sqoop/test/minicluster/JettySqoopMiniCluster.java PRE-CREATION
test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java 4452558
test/src/main/java/org/apache/sqoop/test/testcases/JettyTestCase.java PRE-CREATION
test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java 666749b
test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgradeTest.java 1d5b692
Diff: https://reviews.apache.org/r/38592/diff/
Testing
-------
Thanks,
Colin Ma
Re: Review Request 38592: SQOOP-2553: Sqoop2: Jetty: Update testcases
using Jetty instead of Tomcat
Posted by Colin Ma <ju...@intel.com>.
> On Sept. 24, 2015, 9:01 p.m., Jarek Cecho wrote:
> > test/src/main/java/org/apache/sqoop/test/minicluster/JettySqoopMiniCluster.java, line 63
> > <https://reviews.apache.org/r/38592/diff/1-2/?file=1079643#file1079643line63>
> >
> > Returning empty string will just result in exception being thrown later. What about throwing RuntimeException here with message "server wasn't started" or something like that?
Good point, I'll fix this.
- Colin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38592/#review100464
-----------------------------------------------------------
On Sept. 25, 2015, 7:13 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38592/
> -----------------------------------------------------------
>
> (Updated Sept. 25, 2015, 7:13 a.m.)
>
>
> Review request for Sqoop.
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Update testcases using Jetty instead of Tomcat
>
>
> Diffs
> -----
>
> test/pom.xml 3e11f59
> test/src/main/java/org/apache/sqoop/test/infrastructure/providers/SqoopInfrastructureProvider.java 5b4f595
> test/src/main/java/org/apache/sqoop/test/minicluster/JettySqoopMiniCluster.java PRE-CREATION
> test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java 4452558
> test/src/main/java/org/apache/sqoop/test/testcases/JettyTestCase.java PRE-CREATION
> test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java 666749b
> test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgradeTest.java 1d5b692
>
> Diff: https://reviews.apache.org/r/38592/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 38592: SQOOP-2553: Sqoop2: Jetty: Update testcases
using Jetty instead of Tomcat
Posted by Jarek Cecho <ja...@apache.org>.
> On Sept. 24, 2015, 9:01 p.m., Jarek Cecho wrote:
> > test/src/main/java/org/apache/sqoop/test/minicluster/JettySqoopMiniCluster.java, line 51
> > <https://reviews.apache.org/r/38592/diff/1-2/?file=1079643#file1079643line51>
> >
> > If we are starting the server with "startServer" instead of "starServerForTest", shouldn't we also call stopServer instead of "stopServerForTest"?
Drop my note, I've realized that we have no "stop()" method in that class while looking at the other review.
- Jarek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38592/#review100464
-----------------------------------------------------------
On Sept. 23, 2015, 7:26 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38592/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2015, 7:26 a.m.)
>
>
> Review request for Sqoop.
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Update testcases using Jetty instead of Tomcat
>
>
> Diffs
> -----
>
> test/pom.xml 3e11f59
> test/src/main/java/org/apache/sqoop/test/infrastructure/providers/SqoopInfrastructureProvider.java 5b4f595
> test/src/main/java/org/apache/sqoop/test/minicluster/JettySqoopMiniCluster.java PRE-CREATION
> test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java 4452558
> test/src/main/java/org/apache/sqoop/test/testcases/JettyTestCase.java PRE-CREATION
> test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java 666749b
> test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgradeTest.java 1d5b692
>
> Diff: https://reviews.apache.org/r/38592/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 38592: SQOOP-2553: Sqoop2: Jetty: Update testcases
using Jetty instead of Tomcat
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38592/#review100464
-----------------------------------------------------------
test/src/main/java/org/apache/sqoop/test/minicluster/JettySqoopMiniCluster.java (line 50)
<https://reviews.apache.org/r/38592/#comment157667>
If we are starting the server with "startServer" instead of "starServerForTest", shouldn't we also call stopServer instead of "stopServerForTest"?
test/src/main/java/org/apache/sqoop/test/minicluster/JettySqoopMiniCluster.java (line 62)
<https://reviews.apache.org/r/38592/#comment157666>
Returning empty string will just result in exception being thrown later. What about throwing RuntimeException here with message "server wasn't started" or something like that?
- Jarek Cecho
On Sept. 23, 2015, 7:26 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38592/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2015, 7:26 a.m.)
>
>
> Review request for Sqoop.
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Update testcases using Jetty instead of Tomcat
>
>
> Diffs
> -----
>
> test/pom.xml 3e11f59
> test/src/main/java/org/apache/sqoop/test/infrastructure/providers/SqoopInfrastructureProvider.java 5b4f595
> test/src/main/java/org/apache/sqoop/test/minicluster/JettySqoopMiniCluster.java PRE-CREATION
> test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java 4452558
> test/src/main/java/org/apache/sqoop/test/testcases/JettyTestCase.java PRE-CREATION
> test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java 666749b
> test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgradeTest.java 1d5b692
>
> Diff: https://reviews.apache.org/r/38592/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>