Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-921

Convert TestDFSClientRetries::testNotYetReplicatedErrors to Mockito

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.21.0
    • Component/s: test
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      When TestDFSClientRetries::testNotYetReplicatedErrors was written, Mockito was not available and the NameNode was mocked by manually extending ClientProtocol and implementing all the methods, most with empty bodies. Now that we have Mockito, this code can be removed and replaced with an actual mock.

      1. HDFS-921.patch
        9 kB
        Jakob Homan
      2. HDFS-921-2.patch
        12 kB
        Jakob Homan

        Activity

        Hide
        Jakob Homan added a comment -

        Attaching patch that removes the old, manually mocked Namenode and provides equivalent functionality via Mockito.

        Show
        Jakob Homan added a comment - Attaching patch that removes the old, manually mocked Namenode and provides equivalent functionality via Mockito.
        Hide
        Jakob Homan added a comment -

        submitting patch.

        Show
        Jakob Homan added a comment - submitting patch.
        Hide
        Konstantin Boudnik added a comment -

        +1 patch looks good. It does eliminate about 140 lines of code and, which a more important, reduces the execution time of the test from about 160 secs down to 70 odd! Also, the code is a way more clean now. Great.

        One nit though (non-mandatory): I'd group mockito.* related imports together, but it is a matter of personal taste.

        Show
        Konstantin Boudnik added a comment - +1 patch looks good. It does eliminate about 140 lines of code and, which a more important, reduces the execution time of the test from about 160 secs down to 70 odd! Also, the code is a way more clean now. Great. One nit though (non-mandatory): I'd group mockito.* related imports together, but it is a matter of personal taste.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12431401/HDFS-921.patch
        against trunk revision 902330.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 3 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/205/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/205/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/205/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/205/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12431401/HDFS-921.patch against trunk revision 902330. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/205/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/205/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/205/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/205/console This message is automatically generated.
        Hide
        Jakob Homan added a comment -

        Patch went stale. Updated version. Uses new parameter to addBlock. Otherwise unchanged.
        Cos: I'm a lazy, lazy man. I let Eclipse handle all my imports...

        Show
        Jakob Homan added a comment - Patch went stale. Updated version. Uses new parameter to addBlock. Otherwise unchanged. Cos: I'm a lazy, lazy man. I let Eclipse handle all my imports...
        Hide
        Jakob Homan added a comment -

        Triggering Hudson on updated patch.

        Show
        Jakob Homan added a comment - Triggering Hudson on updated patch.
        Hide
        Jakob Homan added a comment -

        Hudson ran but didn't post:

             [exec] -1 overall.  Here are the results of testing the latest attachment 
             [exec]   http://issues.apache.org/jira/secure/attachment/12442795/HDFS-921-2.patch
             [exec]   against trunk revision 937855.
             [exec] 
             [exec]     +1 @author.  The patch does not contain any @author tags.
             [exec] 
             [exec]     +1 tests included.  The patch appears to include 3 new or modified tests.
             [exec] 
             [exec]     +1 javadoc.  The javadoc tool did not generate any warning messages.
             [exec] 
             [exec]     +1 javac.  The applied patch does not increase the total number of javac compiler warnings.
             [exec] 
             [exec]     +1 findbugs.  The patch does not introduce any new Findbugs warnings.
             [exec] 
             [exec]     +1 release audit.  The applied patch does not increase the total number of release audit warnings.
             [exec] 
             [exec]     +1 core tests.  The patch passed core unit tests.
             [exec] 
             [exec]     -1 contrib tests.  The patch failed contrib unit tests.
        

        Contrib test failure is unrelated.

        Show
        Jakob Homan added a comment - Hudson ran but didn't post: [exec] -1 overall. Here are the results of testing the latest attachment [exec] http://issues.apache.org/jira/secure/attachment/12442795/HDFS-921-2.patch [exec] against trunk revision 937855. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 3 new or modified tests. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. [exec] [exec] +1 core tests. The patch passed core unit tests. [exec] [exec] -1 contrib tests. The patch failed contrib unit tests. Contrib test failure is unrelated.
        Hide
        Jakob Homan added a comment -

        I've committed this. Resolving as fixed.

        Show
        Jakob Homan added a comment - I've committed this. Resolving as fixed.
        Hide
        Konstantin Boudnik added a comment -

        Handling imports automatically is a good thing: stale imports are being eliminated and all that.
        Actually, I was about to suggest to move this test into src/test/unit because now it looks like a real unit test. Shall I open a separate JIRA for that? What do you think?

        Show
        Konstantin Boudnik added a comment - Handling imports automatically is a good thing: stale imports are being eliminated and all that. Actually, I was about to suggest to move this test into src/test/unit because now it looks like a real unit test. Shall I open a separate JIRA for that? What do you think?
        Hide
        Jakob Homan added a comment -

        Actually, I was about to suggest to move this test into src/test/unit because now it looks like a real unit test. Shall I open a separate JIRA for that? What do you think?

        The src/test/unit directory is vexing. We should definitely have a JIRA for moving those tests that can be truly called unit tests to it; I'm just afraid it'll be a lonely little directory tree at the moment. If we did, TestDFSClientRetries actually probably wouldn't qualify because a new test that got added recently uses MiniDFSCluster and takes more than a minute to run... It's worth a JIRA though.

        Show
        Jakob Homan added a comment - Actually, I was about to suggest to move this test into src/test/unit because now it looks like a real unit test. Shall I open a separate JIRA for that? What do you think? The src/test/unit directory is vexing. We should definitely have a JIRA for moving those tests that can be truly called unit tests to it; I'm just afraid it'll be a lonely little directory tree at the moment. If we did, TestDFSClientRetries actually probably wouldn't qualify because a new test that got added recently uses MiniDFSCluster and takes more than a minute to run... It's worth a JIRA though.
        Hide
        Konstantin Boudnik added a comment -

        If we did, TestDFSClientRetries actually probably wouldn't qualify because a new test that got added recently uses MiniDFSCluster and takes more than a minute to run.

        Yeah, you are right - I have revisited the patch and I can see it now. As for the 'loneliness': there are two more unit tests already

        Show
        Konstantin Boudnik added a comment - If we did, TestDFSClientRetries actually probably wouldn't qualify because a new test that got added recently uses MiniDFSCluster and takes more than a minute to run. Yeah, you are right - I have revisited the patch and I can see it now. As for the 'loneliness': there are two more unit tests already

          People

          • Assignee:
            Jakob Homan
            Reporter:
            Jakob Homan
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development