You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Boglarka Egyed <eg...@gmail.com> on 2017/02/25 17:46:19 UTC
Re: Review Request 55466: Clean up expected exception logic in tests
- part III.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55466/
-----------------------------------------------------------
(Updated Feb. 25, 2017, 5:46 p.m.)
Review request for Sqoop, Attila Szabo, Anna Szonyi, and Liz Szilagyi.
Changes
-------
Correcting review findings
Bugs: SQOOP-3100
https://issues.apache.org/jira/browse/SQOOP-3100
Repository: sqoop-trunk
Description
-------
Normalizing test cases where we except an exception using JUnit's ExpectedException rule to make the code more clean and self-explanatory.
Diffs (updated)
-----
src/test/com/cloudera/sqoop/TestIncrementalImport.java 57f4433d8f50b37f102a8a8ada7196d9287b5557
src/test/com/cloudera/sqoop/lib/TestRecordParser.java 57bdb5fe057f74dcaef9e2e3d6e1d26d3af7833a
src/test/com/cloudera/sqoop/metastore/TestSavedJobs.java 1fb732439dd2c8f187af726935a335c993895bc5
src/test/com/cloudera/sqoop/orm/TestClassWriter.java a27353df4ed84a7dfe7612662f3170c00951d2cf
Diff: https://reviews.apache.org/r/55466/diff/
Testing
-------
ant test, ant clean test
Thanks,
Boglarka Egyed
Re: Review Request 55466: Clean up expected exception logic in tests
- part III.
Posted by Boglarka Egyed <eg...@gmail.com>.
> On Feb. 26, 2017, 6:02 a.m., Attila Szabo wrote:
> > Hey Bogi,
> >
> > The change looks good. I've only raised one concern:
> > The mechanism won't be indentical as before b/c we will lose the text messages were included in the fail calls.
> >
> > Would you please consider the usage of reportMissingExceptionWithMessage method?
> >
> > Also: IMHO it would make sense to open JIRAs for your previous cleanup changes to add back those information in the same way.
> >
> > Thanks,
> > Attila
Hey Attila,
Thanks for suggesting reportMissingExceptionWithMessage, I have included it in the logic. I also needed to upgrade to JUnit 4.12 (from 4.11) as this functionality exists from that version.
I'm opening a JIRA ticket for the previous changes to add these fail messages back.
Thanks,
Bogi
- Boglarka
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55466/#review166822
-----------------------------------------------------------
On Feb. 27, 2017, 8:01 p.m., Boglarka Egyed wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55466/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2017, 8:01 p.m.)
>
>
> Review request for Sqoop, Attila Szabo, Anna Szonyi, and Liz Szilagyi.
>
>
> Bugs: SQOOP-3100
> https://issues.apache.org/jira/browse/SQOOP-3100
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> Normalizing test cases where we except an exception using JUnit's ExpectedException rule to make the code more clean and self-explanatory.
>
>
> Diffs
> -----
>
> ivy/libraries.properties 05baa5cb46a841bbd686e950e575129820e7fcf2
> src/test/com/cloudera/sqoop/TestIncrementalImport.java 57f4433d8f50b37f102a8a8ada7196d9287b5557
> src/test/com/cloudera/sqoop/lib/TestRecordParser.java 57bdb5fe057f74dcaef9e2e3d6e1d26d3af7833a
> src/test/com/cloudera/sqoop/metastore/TestSavedJobs.java 1fb732439dd2c8f187af726935a335c993895bc5
> src/test/com/cloudera/sqoop/orm/TestClassWriter.java a27353df4ed84a7dfe7612662f3170c00951d2cf
>
> Diff: https://reviews.apache.org/r/55466/diff/
>
>
> Testing
> -------
>
> ant test, ant clean test
>
>
> Thanks,
>
> Boglarka Egyed
>
>
Re: Review Request 55466: Clean up expected exception logic in tests
- part III.
Posted by Attila Szabo <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55466/#review166822
-----------------------------------------------------------
Fix it, then Ship it!
Hey Bogi,
The change looks good. I've only raised one concern:
The mechanism won't be indentical as before b/c we will lose the text messages were included in the fail calls.
Would you please consider the usage of reportMissingExceptionWithMessage method?
Also: IMHO it would make sense to open JIRAs for your previous cleanup changes to add back those information in the same way.
Thanks,
Attila
src/test/com/cloudera/sqoop/TestIncrementalImport.java
<https://reviews.apache.org/r/55466/#comment238897>
Hey Bogi,
Would you please consider using the reportMissingExceptionWithMessage method of ExpectedException not to loose these kind of information in the new version either?
Thanks
- Attila Szabo
On Feb. 25, 2017, 5:46 p.m., Boglarka Egyed wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55466/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2017, 5:46 p.m.)
>
>
> Review request for Sqoop, Attila Szabo, Anna Szonyi, and Liz Szilagyi.
>
>
> Bugs: SQOOP-3100
> https://issues.apache.org/jira/browse/SQOOP-3100
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> Normalizing test cases where we except an exception using JUnit's ExpectedException rule to make the code more clean and self-explanatory.
>
>
> Diffs
> -----
>
> src/test/com/cloudera/sqoop/TestIncrementalImport.java 57f4433d8f50b37f102a8a8ada7196d9287b5557
> src/test/com/cloudera/sqoop/lib/TestRecordParser.java 57bdb5fe057f74dcaef9e2e3d6e1d26d3af7833a
> src/test/com/cloudera/sqoop/metastore/TestSavedJobs.java 1fb732439dd2c8f187af726935a335c993895bc5
> src/test/com/cloudera/sqoop/orm/TestClassWriter.java a27353df4ed84a7dfe7612662f3170c00951d2cf
>
> Diff: https://reviews.apache.org/r/55466/diff/
>
>
> Testing
> -------
>
> ant test, ant clean test
>
>
> Thanks,
>
> Boglarka Egyed
>
>
Re: Review Request 55466: Clean up expected exception logic in tests
- part III.
Posted by Szabolcs Vasas <va...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55466/#review166858
-----------------------------------------------------------
Ship it!
Ship It!
- Szabolcs Vasas
On Feb. 25, 2017, 5:46 p.m., Boglarka Egyed wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55466/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2017, 5:46 p.m.)
>
>
> Review request for Sqoop, Attila Szabo, Anna Szonyi, and Liz Szilagyi.
>
>
> Bugs: SQOOP-3100
> https://issues.apache.org/jira/browse/SQOOP-3100
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> Normalizing test cases where we except an exception using JUnit's ExpectedException rule to make the code more clean and self-explanatory.
>
>
> Diffs
> -----
>
> src/test/com/cloudera/sqoop/TestIncrementalImport.java 57f4433d8f50b37f102a8a8ada7196d9287b5557
> src/test/com/cloudera/sqoop/lib/TestRecordParser.java 57bdb5fe057f74dcaef9e2e3d6e1d26d3af7833a
> src/test/com/cloudera/sqoop/metastore/TestSavedJobs.java 1fb732439dd2c8f187af726935a335c993895bc5
> src/test/com/cloudera/sqoop/orm/TestClassWriter.java a27353df4ed84a7dfe7612662f3170c00951d2cf
>
> Diff: https://reviews.apache.org/r/55466/diff/
>
>
> Testing
> -------
>
> ant test, ant clean test
>
>
> Thanks,
>
> Boglarka Egyed
>
>
Re: Review Request 55466: Clean up expected exception logic in tests
- part III.
Posted by Attila Szabo <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55466/#review168886
-----------------------------------------------------------
Ship it!
Ship It!
- Attila Szabo
On Feb. 27, 2017, 8:01 p.m., Boglarka Egyed wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55466/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2017, 8:01 p.m.)
>
>
> Review request for Sqoop, Attila Szabo, Anna Szonyi, and Liz Szilagyi.
>
>
> Bugs: SQOOP-3100
> https://issues.apache.org/jira/browse/SQOOP-3100
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> Normalizing test cases where we except an exception using JUnit's ExpectedException rule to make the code more clean and self-explanatory.
>
>
> Diffs
> -----
>
> ivy/libraries.properties 05baa5cb46a841bbd686e950e575129820e7fcf2
> src/test/com/cloudera/sqoop/TestIncrementalImport.java 57f4433d8f50b37f102a8a8ada7196d9287b5557
> src/test/com/cloudera/sqoop/lib/TestRecordParser.java 57bdb5fe057f74dcaef9e2e3d6e1d26d3af7833a
> src/test/com/cloudera/sqoop/metastore/TestSavedJobs.java 1fb732439dd2c8f187af726935a335c993895bc5
> src/test/com/cloudera/sqoop/orm/TestClassWriter.java a27353df4ed84a7dfe7612662f3170c00951d2cf
>
>
> Diff: https://reviews.apache.org/r/55466/diff/3/
>
>
> Testing
> -------
>
> ant test, ant clean test
>
>
> Thanks,
>
> Boglarka Egyed
>
>
Re: Review Request 55466: Clean up expected exception logic in tests
- part III.
Posted by Boglarka Egyed <eg...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55466/
-----------------------------------------------------------
(Updated Feb. 27, 2017, 8:01 p.m.)
Review request for Sqoop, Attila Szabo, Anna Szonyi, and Liz Szilagyi.
Changes
-------
Upgrading to JUnit 4.12 and using report reportMissingExceptionWithMessage where needed
Bugs: SQOOP-3100
https://issues.apache.org/jira/browse/SQOOP-3100
Repository: sqoop-trunk
Description
-------
Normalizing test cases where we except an exception using JUnit's ExpectedException rule to make the code more clean and self-explanatory.
Diffs (updated)
-----
ivy/libraries.properties 05baa5cb46a841bbd686e950e575129820e7fcf2
src/test/com/cloudera/sqoop/TestIncrementalImport.java 57f4433d8f50b37f102a8a8ada7196d9287b5557
src/test/com/cloudera/sqoop/lib/TestRecordParser.java 57bdb5fe057f74dcaef9e2e3d6e1d26d3af7833a
src/test/com/cloudera/sqoop/metastore/TestSavedJobs.java 1fb732439dd2c8f187af726935a335c993895bc5
src/test/com/cloudera/sqoop/orm/TestClassWriter.java a27353df4ed84a7dfe7612662f3170c00951d2cf
Diff: https://reviews.apache.org/r/55466/diff/
Testing
-------
ant test, ant clean test
Thanks,
Boglarka Egyed