Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Plain View
Pig >> mail # dev >> Re: Review Request: Modernize a chunk of the tests


Copy link to this message
-
Re: Review Request: Modernize a chunk of the tests

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7734/#review12859
-----------------------------------------------------------
Hi, Jonathan.

It looks really good! I made a few minor comments inline.

But my only complaint is about indentations. I know that we don't generate patches with white space changes, but I think that white space changes should be as part of the patch in this case because you are cleaning up a lot of old code. In particular, you are removing several try-catch blocks. But since indentations are not fixed, they look really odd.

Could you upload a new patch that includes white space changes to the RB? I don't think that it's too hard to review it since the RB has an option to hide them. Please let me know what you think.

Thanks!
test/org/apache/pig/test/TestNewPlanOperatorPlan.java
<https://reviews.apache.org/r/7734/#comment27620>

    Please change to @Test(expected = FrontendException).

test/org/apache/pig/test/TestPigScriptParser.java
<https://reviews.apache.org/r/7734/#comment27627>

    Please change to @Test(expected = FrontendException).

test/org/apache/pig/test/TestPigScriptParser.java
<https://reviews.apache.org/r/7734/#comment27628>

    Please change the order of args in assertEquals to expected and output.

test/org/apache/pig/test/TestPigScriptParser.java
<https://reviews.apache.org/r/7734/#comment27630>

    Please change the order of args in assertEquals to expected and output.

test/org/apache/pig/test/TestPigSplit.java
<https://reviews.apache.org/r/7734/#comment27634>

    Isn't this a test case? If so, please change the method name to testXX and add @Test to it.

test/org/apache/pig/test/TestPigSplit.java
<https://reviews.apache.org/r/7734/#comment27635>

    Please change this to assertFalse(iter.hasNext()).

test/org/apache/pig/test/TestPigSplit.java
<https://reviews.apache.org/r/7734/#comment27636>

    Please change this to assertFalse(iter.hasNext()).

test/org/apache/pig/test/TestPruneColumn.java
<https://reviews.apache.org/r/7734/#comment27637>

    Please change the order of args in assertEquals to expected and output.

test/org/apache/pig/test/TestPruneColumn.java
<https://reviews.apache.org/r/7734/#comment27638>

    Please change the order of args in assertEquals to expected and output.

test/org/apache/pig/test/TestPruneColumn.java
<https://reviews.apache.org/r/7734/#comment27639>

    Please change the order of args in assertEquals to expected and output.

test/org/apache/pig/test/TestPruneColumn.java
<https://reviews.apache.org/r/7734/#comment27640>

    Please change the order of args in assertEquals to expected and output.

test/org/apache/pig/test/TestPruneColumn.java
<https://reviews.apache.org/r/7734/#comment27643>

    Please change this to assertNull().

test/org/apache/pig/test/TestPruneColumn.java
<https://reviews.apache.org/r/7734/#comment27644>

    Please change this to assertNull().

test/org/apache/pig/test/TestPruneColumn.java
<https://reviews.apache.org/r/7734/#comment27645>

    Please change the order of args in assertEquals to expected to output.

test/org/apache/pig/test/TestRelationToExprProject.java
<https://reviews.apache.org/r/7734/#comment27646>

    Please delete this line?
- Cheolsoo Park
On Oct. 25, 2012, 6:05 p.m., Jonathan Coveney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7734/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2012, 6:05 p.m.)
>
>
> Review request for pig and Julien Le Dem.
>
>
> Description
> -------
>
> A lot of the tests use antiquated patterns. My goal was to refactor them in a couple ways:
>
> - get rid of the annotation specifying Junit 4. All should use JUnit 4 (question: where is the Junit 3 dependency even being pulled?)
+
Jonathan Coveney 2012-10-30, 01:10