-
Re: Review Request: Modernize a chunk of the testsCheolsoo Park 2012-10-28, 04:13
-----------------------------------------------------------
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?)
