Pig
  1. Pig
  2. PIG-3006

Modernize a chunk of the tests

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.12.0
    • Component/s: None
    • Labels:
      None
    • Patch Info:
      Patch Available

      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 in?)
      • 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.

      I've run every test and they pass, EXCEPT TestLargeFile which is failing on trunk anyway.

      1. PIG-3006-4.patch
        750 kB
        Cheolsoo Park
      2. PIG-3006-3.patch
        712 kB
        Cheolsoo Park
      3. PIG-3006-2.patch
        740 kB
        Jonathan Coveney
      4. PIG-3006-1.patch
        369 kB
        Jonathan Coveney
      5. PIG-3006-0.patch
        369 kB
        Jonathan Coveney

        Issue Links

          Activity

          Hide
          Gianmarco De Francisci Morales added a comment -

          Great job guys!

          Show
          Gianmarco De Francisci Morales added a comment - Great job guys!
          Hide
          Jonathan Coveney added a comment -

          Awesome! Thanks, Cheolsoo. Now onto https://issues.apache.org/jira/browse/PIG-3016

          Show
          Jonathan Coveney added a comment - Awesome! Thanks, Cheolsoo. Now onto https://issues.apache.org/jira/browse/PIG-3016
          Hide
          Cheolsoo Park added a comment -

          I ran the full unit test suite with both hadoop 20 and 23 and didn't see any additional failures.

          Committed to trunk. Thanks Jonathan!

          Show
          Cheolsoo Park added a comment - I ran the full unit test suite with both hadoop 20 and 23 and didn't see any additional failures. Committed to trunk. Thanks Jonathan!
          Hide
          Jonathan Coveney added a comment -

          Cheolsoo,

          Absolutely feel free to proceed as such. It doesn't apply completely afaik b/c a lot of the windows patches clashed (Daniel alerted me to this but I said for them to go ahead and that we would fix it). So go ahead and fix and we're good to go.

          Show
          Jonathan Coveney added a comment - Cheolsoo, Absolutely feel free to proceed as such. It doesn't apply completely afaik b/c a lot of the windows patches clashed (Daniel alerted me to this but I said for them to go ahead and that we would fix it). So go ahead and fix and we're good to go.
          Hide
          Cheolsoo Park added a comment -

          I am running the unit test suite with this patch and like to commit this to trunk unless anyone is planning to.

          I noticed that the patch already doesn't apply nicely to trunk, so I am attaching a new patch that fixes conflicts. They're all due to white space changes. It should be OK for me to proceed, shouldn't it? Let me know.

          1 out of 7 hunks FAILED -- saving rejects to file test/org/apache/pig/test/TestInputOutputFileValidator.java.rej
          1 out of 8 hunks FAILED -- saving rejects to file test/org/apache/pig/test/TestLocal2.java.rej
          1 out of 4 hunks FAILED -- saving rejects to file test/org/apache/pig/test/TestNewPlanColumnPrune.java.rej
          1 out of 27 hunks FAILED -- saving rejects to file test/org/apache/pig/test/TestStore.java.rej
          8 out of 21 hunks FAILED -- saving rejects to file test/org/apache/pig/test/TestStreamingLocal.java.rej
          
          Show
          Cheolsoo Park added a comment - I am running the unit test suite with this patch and like to commit this to trunk unless anyone is planning to. I noticed that the patch already doesn't apply nicely to trunk, so I am attaching a new patch that fixes conflicts. They're all due to white space changes. It should be OK for me to proceed, shouldn't it? Let me know. 1 out of 7 hunks FAILED -- saving rejects to file test/org/apache/pig/test/TestInputOutputFileValidator.java.rej 1 out of 8 hunks FAILED -- saving rejects to file test/org/apache/pig/test/TestLocal2.java.rej 1 out of 4 hunks FAILED -- saving rejects to file test/org/apache/pig/test/TestNewPlanColumnPrune.java.rej 1 out of 27 hunks FAILED -- saving rejects to file test/org/apache/pig/test/TestStore.java.rej 8 out of 21 hunks FAILED -- saving rejects to file test/org/apache/pig/test/TestStreamingLocal.java.rej
          Hide
          Alan Gates added a comment -

          +1 from another pair of eyeballs.

          Thanks for taking on this important but tedious work.

          Show
          Alan Gates added a comment - +1 from another pair of eyeballs. Thanks for taking on this important but tedious work.
          Hide
          Jonathan Coveney added a comment -

          This patch takes into account Cheolsoo's RB comments. It ALSO includes whitespace changes. We will see how that goes!

          Show
          Jonathan Coveney added a comment - This patch takes into account Cheolsoo's RB comments. It ALSO includes whitespace changes. We will see how that goes!
          Hide
          Jonathan Coveney added a comment -

          Cheolsoo, I saw your comments in the RB, and they are good. Specially w.r.t whitespace, I was hoping you could express similar sentiment here: https://issues.apache.org/jira/browse/PIG-3008 The whitespace issue is of particular annoyance to me and I'd love to come up with a solution.

          Show
          Jonathan Coveney added a comment - Cheolsoo, I saw your comments in the RB, and they are good. Specially w.r.t whitespace, I was hoping you could express similar sentiment here: https://issues.apache.org/jira/browse/PIG-3008 The whitespace issue is of particular annoyance to me and I'd love to come up with a solution.
          Hide
          Gianmarco De Francisci Morales added a comment -

          I am fine with the changes, but it's a huge patch so I skimmed through it, I might have missed something.
          +0, let's wait for another pair of eyeballs.

          Show
          Gianmarco De Francisci Morales added a comment - I am fine with the changes, but it's a huge patch so I skimmed through it, I might have missed something. +0, let's wait for another pair of eyeballs.
          Hide
          Jonathan Coveney added a comment -

          Thanks for the comments Gianmarco. I cleaned things up even more.

          Show
          Jonathan Coveney added a comment - Thanks for the comments Gianmarco. I cleaned things up even more.
          Hide
          Gianmarco De Francisci Morales added a comment -

          I did a quick review and it looked good (I left a couple of comments), but I would feel better if one more committer reviews it.

          Show
          Gianmarco De Francisci Morales added a comment - I did a quick review and it looked good (I left a couple of comments), but I would feel better if one more committer reviews it.
          Hide
          Jonathan Coveney added a comment -

          Thanks Now I just need to cajole a committer into reviewing it

          Show
          Jonathan Coveney added a comment - Thanks Now I just need to cajole a committer into reviewing it
          Hide
          Gianmarco De Francisci Morales added a comment -

          Great job Jon! Our test suite definitely needed some cleanup.

          Show
          Gianmarco De Francisci Morales added a comment - Great job Jon! Our test suite definitely needed some cleanup.
          Hide
          Jonathan Coveney added a comment -
          Show
          Jonathan Coveney added a comment - Here is the RB: https://reviews.apache.org/r/7734/
          Hide
          Jonathan Coveney added a comment -

          I've attached a patch and will attach an RB.

          Show
          Jonathan Coveney added a comment - I've attached a patch and will attach an RB.

            People

            • Assignee:
              Jonathan Coveney
              Reporter:
              Jonathan Coveney
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development