-----------------------------------------------------------
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?)

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

(Updated Oct. 30, 2012, 1:10 a.m.)
Review request for pig and Julien Le Dem.
Changes
-------

I have taken into account comments, and this time I didn't use -w. It has whitespace changes. We'll see how that goes...
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?)
- Nothing should extend TestCase. Everything should be annotation driven.
- Properly use asserts. There was a lot of assertTrue(null==thing), so I replaced it with assertNull(thing), and so on.
- Get rid of MiniCluster use in a handful of cases.
This addresses bug PIG-3006.
    https://issues.apache.org/jira/browse/PIG-3006
Diffs (updated)
-----

  test/org/apache/pig/test/PigExecTestCase.java 32a502c
  test/org/apache/pig/test/TestAlgebraicEval.java 0bbd83d
  test/org/apache/pig/test/TestAlgebraicEvalLocal.java df4b76a
  test/org/apache/pig/test/TestBagFormat.java 09298d4
  test/org/apache/pig/test/TestBatchAliases.java 6e952c7
  test/org/apache/pig/test/TestCompressedFiles.java d54ffaa
  test/org/apache/pig/test/TestConversions.java 152ad5c
  test/org/apache/pig/test/TestDeleteOnFail.java 7070285
  test/org/apache/pig/test/TestFilterOpNumeric.java 730e808
  test/org/apache/pig/test/TestFilterOpString.java b65965f
  test/org/apache/pig/test/TestFilterSimplification.java ade97b6
  test/org/apache/pig/test/TestForEachNestedPlanLocal.java a78568e
  test/org/apache/pig/test/TestFuncSpec.java bc7144c
  test/org/apache/pig/test/TestInfixArithmetic.java cdf6948
  test/org/apache/pig/test/TestInputOutputFileValidator.java 67b2873
  test/org/apache/pig/test/TestInputOutputMiniClusterFileValidator.java caa62cb
  test/org/apache/pig/test/TestInstantiateFunc.java 31c37b1
  test/org/apache/pig/test/TestJoin.java a4f3aff
  test/org/apache/pig/test/TestKeyTypeDiscoveryVisitor.java 2bbeca1
  test/org/apache/pig/test/TestLargeFile.java 79590ce
  test/org/apache/pig/test/TestLocal.java 5680196
  test/org/apache/pig/test/TestLocal2.java eea7b2f
  test/org/apache/pig/test/TestMapReduce2.java 30574db
  test/org/apache/pig/test/TestNewPlanColumnPrune.java bed006e
  test/org/apache/pig/test/TestNewPlanListener.java 7701182
  test/org/apache/pig/test/TestNewPlanOperatorPlan.java 1f8fe56
  test/org/apache/pig/test/TestNewPlanPruneMapKeys.java d1cce22
  test/org/apache/pig/test/TestNewPlanRule.java 4a7ff0a
  test/org/apache/pig/test/TestNullConstant.java 3ae25d9
  test/org/apache/pig/test/TestOrderBy2.java 4ee4f26
  test/org/apache/pig/test/TestOrderBy3.java 2067d7a
  test/org/apache/pig/test/TestPOBinCond.java 20bd734
  test/org/apache/pig/test/TestPODistinct.java 60f9d73
  test/org/apache/pig/test/TestPOGenerate.java e0fd796
  test/org/apache/pig/test/TestPOMapLookUp.java 3ed0900
  test/org/apache/pig/test/TestPONegative.java 220c409
  test/org/apache/pig/test/TestPORegexp.java d6e15ac
  test/org/apache/pig/test/TestPOSort.java 600ee0c
  test/org/apache/pig/test/TestPOUserFunc.java 3a90d6c
  test/org/apache/pig/test/TestParamSubPreproc.java 1a52691
  test/org/apache/pig/test/TestParser.java 17dc42a
  test/org/apache/pig/test/TestPi.java f0883d1
  test/org/apache/pig/test/TestPigProgressReporting.java e4f76ec
  test/org/apache/pig/test/TestPigScriptParser.java 2acb1a8
  test/org/apache/pig/test/TestPigSplit.java af70e9d
  test/org/apache/pig/test/TestPinOptions.java a730ce7
  test/org/apache/pig/test/TestPruneColumn.java 03139a5
  test/org/apache/pig/test/TestRank1.java fbc6a7d
  test/org/apache/pig/test/TestRank2.java d4daf8b
  test/org/apache/pig/test/TestRank3.java 6dd2624
  test/org/apache/pig/test/TestRelationToExprProject.java 1411451
  test/org/apache/pig/test/TestSchemaUtil.java e1d1133
  test/org/apache/pig/test/TestStore.java 7f1c77b
  test/org/apache/pig/test/TestStoreOld.java 37ad3bf
  test/org/apache/pig/test/TestStreamingLocal.java b745074
  test/org/apache/pig/test/TestToolsPigServer.java e021b8c
  test/org/apache/pig/test/TestUDF.java f1b10f8
  test/org/apache/pig/test/TestUDFGroovy.java e5b8c8e
  test/org/apache/pig/test/TestUDFWithoutParameter.java 2527afa
  test/org/apache/pig/test/TestUTF8.java 42aab25

Diff: https://reviews.apache.org/r/7734/diff/
Testing

I ran every test affected and they pass, except for TestLargeFile which is failing independently (I made no changes to TestLargeFile that should affect whether it passed, it was small and cosmetic)
Thanks,

Jonathan Coveney