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