Hadoop Common
  1. Hadoop Common
  2. HADOOP-6987

Use JUnit Rule to optionally fail test cases that run more than 10 seconds

    Details

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

      Description

      Using JUnit Rules annotations we can fail tests cases that take longer than 10 seconds (for instance) to run. This provides a regression check against test cases taking longer than they had previously due to unintended code changes, as well as provides a membership criteria for unit tests versus integration tests in HDFS and MR.

      1. HADOOP-6897.patch
        5 kB
        Jakob Homan
      2. HADOOP-6987-2.patch
        5 kB
        Jakob Homan

        Issue Links

          Activity

          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk #489 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk/489/)

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk #489 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk/489/ )
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #395 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk-Commit/395/)

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #395 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk-Commit/395/ )
          Hide
          Jakob Homan added a comment -

          I re-ran test-patch, which was clean. I've committed this. Resolving as fixed.

          Show
          Jakob Homan added a comment - I re-ran test-patch, which was clean. I've committed this. Resolving as fixed.
          Hide
          Konstantin Boudnik added a comment -

          +1 patch looks good.
          I have ran it with latest trunk and it seems to be doing the job.

          Show
          Konstantin Boudnik added a comment - +1 patch looks good. I have ran it with latest trunk and it seems to be doing the job.
          Hide
          Jakob Homan added a comment -

          Updated patch with Cos' suggestion. There's not really a util class to put the value in, so it's now a final field on the rule class. I'll update the unit test guidelines when the patch is committed. I've linked this JIRA to HADOOP-6399. Once this is committed, I'll open an HDFS JIRA to tag the tests currently in the unit directory.

          Show
          Jakob Homan added a comment - Updated patch with Cos' suggestion. There's not really a util class to put the value in, so it's now a final field on the rule class. I'll update the unit test guidelines when the patch is committed. I've linked this JIRA to HADOOP-6399 . Once this is committed, I'll open an HDFS JIRA to tag the tests currently in the unit directory.
          Hide
          Konstantin Boudnik added a comment -

          I had an offline conversation with Jakob about this and he expressed that the approach is about to enforce max time of a true unit test execution. Unit tests are clearly suppose to be very short.

          I still have a couple comments about this:

          • let's call class UnitTestTimeLimit rather then TenSecondsTimeoutPerTest to make it more generic
          • let's make timeout to be a constant and move it to a config or util class
          • let's update Unit test page
          • as we are going to establish a precedent for setting rules of engagement I'd suggest to update HADOOP-6399 with that rule
          Show
          Konstantin Boudnik added a comment - I had an offline conversation with Jakob about this and he expressed that the approach is about to enforce max time of a true unit test execution. Unit tests are clearly suppose to be very short. I still have a couple comments about this: let's call class UnitTestTimeLimit rather then TenSecondsTimeoutPerTest to make it more generic let's make timeout to be a constant and move it to a config or util class let's update Unit test page as we are going to establish a precedent for setting rules of engagement I'd suggest to update HADOOP-6399 with that rule
          Hide
          Konstantin Boudnik added a comment -

          Effectively, you offer to have a common class which has to be extended instead of having an explicit statement like
          @Rule public MethodRule globalTimeout = new Timeout(10 * 1000); in each test class which needs to have a special timeout clause. I vouch that having an explicit @Rule statement like above (if the same timeout applies for all class' cases) is clearer and doesn't require inheritance.

          I think it worth noting here about best practices of timeout settings as soon as we'll agreed on the approach.

          Show
          Konstantin Boudnik added a comment - Effectively, you offer to have a common class which has to be extended instead of having an explicit statement like @Rule public MethodRule globalTimeout = new Timeout(10 * 1000); in each test class which needs to have a special timeout clause. I vouch that having an explicit @Rule statement like above (if the same timeout applies for all class' cases) is clearer and doesn't require inheritance. I think it worth noting here about best practices of timeout settings as soon as we'll agreed on the approach.
          Hide
          Jakob Homan added a comment -

          Also, this approach doesn't rule out the other. It just adds a convenience class that can be included, or not, as needed. Test writers could certainly not use it and annotate individual test cases, as may be appropriate.

          Show
          Jakob Homan added a comment - Also, this approach doesn't rule out the other. It just adds a convenience class that can be included, or not, as needed. Test writers could certainly not use it and annotate individual test cases, as may be appropriate.
          Hide
          Jakob Homan added a comment -

          I don't like unnecessary inheritance in tests.

          I'm not wild about them either, but one works with what one's got.

          what if we'll have tests which shouldn't go over 10 secs, and others (i.e. true unit tests) which shouldn't exceed 2 secs

          The current layout in HDFS is to have separate directory trees for the integration and (currently sparsely populated) unit tests. This implies we wouldn't be commingling the two types in the same file, and this approach is predicated on that assumption. I'm ok with either approach, but if we're going to be combining types, we probably should collapse the directory trees sooner than later.

          If, on the other hand, we're separating unit and integration tests into different files, this approach has the advantage that one can apply the rule to an entire file with only one line (assuming the test is JUnit 4), rather than having to mark each and every test case with the same annotation. This approach thus has a smaller burden on the test writer and on any refactoring we may want to do to implement it.

          Also, it would be simple enough to write a script as part of test-patch to verify that all tests in /unit are so marked and thus eligible to be in the unit category (although, it also wouldn't be prohibitively difficult to write one to check each @Test annotation to include a timeout value).

          Show
          Jakob Homan added a comment - I don't like unnecessary inheritance in tests. I'm not wild about them either, but one works with what one's got. what if we'll have tests which shouldn't go over 10 secs, and others (i.e. true unit tests) which shouldn't exceed 2 secs The current layout in HDFS is to have separate directory trees for the integration and (currently sparsely populated) unit tests. This implies we wouldn't be commingling the two types in the same file, and this approach is predicated on that assumption. I'm ok with either approach, but if we're going to be combining types, we probably should collapse the directory trees sooner than later. If, on the other hand, we're separating unit and integration tests into different files, this approach has the advantage that one can apply the rule to an entire file with only one line (assuming the test is JUnit 4), rather than having to mark each and every test case with the same annotation. This approach thus has a smaller burden on the test writer and on any refactoring we may want to do to implement it. Also, it would be simple enough to write a script as part of test-patch to verify that all tests in /unit are so marked and thus eligible to be in the unit category (although, it also wouldn't be prohibitively difficult to write one to check each @Test annotation to include a timeout value).
          Hide
          Konstantin Boudnik added a comment -

          I don't like unnecessary inheritance in tests. This is, in fact, why I can't stand JUnit3
          The solution in the patch has one more disadvantage: what if we'll have tests which shouldn't go over 10 secs, and others (i.e. true unit tests) which shouldn't exceed 2 secs? Would offered solution force us to have different implementations for different timeouts?

          In this particular case I'd suggest to simply annotate test cases with expected timeout as in

          @Test(timeout=10000)
          public void testEscapeString() throws Exception {
          
          Show
          Konstantin Boudnik added a comment - I don't like unnecessary inheritance in tests. This is, in fact, why I can't stand JUnit3 The solution in the patch has one more disadvantage: what if we'll have tests which shouldn't go over 10 secs, and others (i.e. true unit tests) which shouldn't exceed 2 secs? Would offered solution force us to have different implementations for different timeouts? In this particular case I'd suggest to simply annotate test cases with expected timeout as in @Test(timeout=10000) public void testEscapeString() throws Exception {
          Hide
          Jakob Homan added a comment -

          Not getting my hopes up about Hudson. Tests all pass except known-bad TestSetFile (HADOOP-6989). Test-patch is good:

               [exec] +1 overall.  
               [exec] 
               [exec]     +1 @author.  The patch does not contain any @author tags.
               [exec] 
               [exec]     +1 tests included.  The patch appears to include 5 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 system tests framework.  The patch passed system tests framework compile.
          Show
          Jakob Homan added a comment - Not getting my hopes up about Hudson. Tests all pass except known-bad TestSetFile ( HADOOP-6989 ). Test-patch is good: [exec] +1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 5 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 system tests framework. The patch passed system tests framework compile.
          Hide
          Jakob Homan added a comment -

          Submitting patch.

          Show
          Jakob Homan added a comment - Submitting patch.
          Hide
          Jakob Homan added a comment -

          Patch creates a new base class, TenSecondsTimeoutPerTest, which tests can extend to 'mix in' (as much as Java allows) this behavior. Tests extending this class will automatically be failed if their cases run more than 10 seconds, like so:

          @Test public void willTimeOut() throws InterruptedException {
            Thread.sleep(12 * 1000);
          }
          

          results in

           Testcase: willTimeOut took 10.007 sec
             Caused an ERROR
           test timed out after 10000 milliseconds
           java.lang.Exception: test timed out after 10000 milliseconds
             at java.lang.Thread.sleep(Native Method)
             at org.apache.hadoop.util.TestStringUtils.willTimeOut(TestStringUtils.java:45)
          

          Tests cases need to be in JUnit 4 style to do this, and I've converted one from Core at random, to test its functionality. Most - though not all - all of the Core tests behave well, so this feature is intended more for HDFS and MR.

          Show
          Jakob Homan added a comment - Patch creates a new base class, TenSecondsTimeoutPerTest, which tests can extend to 'mix in' (as much as Java allows) this behavior. Tests extending this class will automatically be failed if their cases run more than 10 seconds, like so: @Test public void willTimeOut() throws InterruptedException { Thread.sleep(12 * 1000); } results in Testcase: willTimeOut took 10.007 sec Caused an ERROR test timed out after 10000 milliseconds java.lang.Exception: test timed out after 10000 milliseconds at java.lang.Thread.sleep(Native Method) at org.apache.hadoop.util.TestStringUtils.willTimeOut(TestStringUtils.java:45) Tests cases need to be in JUnit 4 style to do this, and I've converted one from Core at random, to test its functionality. Most - though not all - all of the Core tests behave well, so this feature is intended more for HDFS and MR.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development