sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Veena Basavaraj" <vbasava...@cloudera.com>
Subject Re: Review Request 29773: External connector loading in Sqoop2
Date Fri, 16 Jan 2015 05:57:25 GMT


> On Jan. 15, 2015, 7:03 a.m., Qian Xu wrote:
> > core/src/test/java/org/apache/sqoop/connector/TestConnectorManagerUtils.java, line
59
> > <https://reviews.apache.org/r/29773/diff/8/?file=821969#file821969line59>
> >
> >     Are you expecting GenericJdbcConnector class can be instantiated? If it is true,
better add a assertNotNull to test. 
> >     
> >     NIT: Maybe a docstring before @Test will be descriptive for this test case.
> 
> Veena Basavaraj wrote:
>     the ClassUtils throws an exception if class nt found so that will fail the test.
DOnt think NotNull test is a must.
> 
> Qian Xu wrote:
>     What is the expectation of the test then??? Usually the following code without checking
its return value means you expect an exception!
>     
>         ClassUtils.loadClass("org.apache.sqoop.connector.jdbc.GenericJdbcConnector");
> 
> Veena Basavaraj wrote:
>     expectation is just if it found the class, if it did not find it then it will throw
Exception,
> 
> Qian Xu wrote:
>     The expected behavior of your test case is to ensure class can be loaded. And your
code relies on not seeing an implicit exception. Wouldn't the following code be more straightfoward?
>     
>         Class<AbstractConnector> actual = ClassUtils.loadClass("org.apache.sqoop.connector.jdbc.GenericJdbcConnector");
>         assertNotNull(actual);

i really care just that there is not CNF period, nothing more


- Veena


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29773/#review68249
-----------------------------------------------------------


On Jan. 15, 2015, 11:02 a.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29773/
> -----------------------------------------------------------
> 
> (Updated Jan. 15, 2015, 11:02 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-1821
>     https://issues.apache.org/jira/browse/SQOOP-1821
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> see jira for details
> 
> For committing/ testing:
> Please upload the test jars separately while applying patch and committing, since QA
bot does not like binary files.
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/sqoop/utils/ClassUtils.java 0be4d41 
>   common/src/test/java/org/apache/sqoop/utils/TestClassUtils.java 161a1fa 
>   core/src/main/java/org/apache/sqoop/connector/ConnectorHandler.java 1919b4b 
>   core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java b9d4d60 
>   core/src/main/java/org/apache/sqoop/connector/ConnectorManagerUtils.java c7193ee 
>   core/src/main/java/org/apache/sqoop/core/ConfigurationConstants.java f341108 
>   core/src/test/java/org/apache/sqoop/connector/TestConnectorManagerUtils.java PRE-CREATION

>   core/src/test/resources/test-connector.jar PRE-CREATION 
>   core/src/test/resources/test-non-connector.jar PRE-CREATION 
>   dist/src/main/server/conf/sqoop.properties e22e8b0 
> 
> Diff: https://reviews.apache.org/r/29773/diff/
> 
> 
> Testing
> -------
> 
> added unit tests as well
> 
> 
> Thanks,
> 
> Veena Basavaraj
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message