Hadoop Common
  1. Hadoop Common
  2. HADOOP-9112

test-patch should -1 for @Tests without a timeout

    Details

    • Type: Improvement Improvement
    • Status: Reopened
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: build
    • Labels:
      None
    • Target Version/s:

      Description

      With our current test running infrastructure, if a test with no timeout set runs too long, it triggers a surefire-wide timeout, which for some reason doesn't show up as a failed test in the test-patch output. Given that, we should require that all tests have a timeout set, and have test-patch enforce this with a simple check

      1. HADOOP-9112-7.patch
        0.5 kB
        Surenkumar Nihalani
      2. HADOOP-9112-6.patch
        2 kB
        Surenkumar Nihalani
      3. HADOOP-9112-5.patch
        2 kB
        Surenkumar Nihalani
      4. HADOOP-9112-4.patch
        2 kB
        Surenkumar Nihalani
      5. HADOOP-9112-3.patch
        1 kB
        Surenkumar Nihalani
      6. HADOOP-9112-2.patch
        1 kB
        Surenkumar Nihalani
      7. HADOOP-9112-1.patch
        1 kB
        Surenkumar Nihalani

        Issue Links

          Activity

          Hide
          Aaron T. Myers added a comment -

          I'd propose that test-patch "-0" the patch with a comment along the lines of "This patch introduces tests that do not appear to have a timeout. Please justify why these tests cannot time out."

          Show
          Aaron T. Myers added a comment - I'd propose that test-patch "-0" the patch with a comment along the lines of "This patch introduces tests that do not appear to have a timeout. Please justify why these tests cannot time out."
          Hide
          Todd Lipcon added a comment -

          Another option might be to require that all classes containing test methods be annotated with a JUnit rule which enforces default timeouts. I'm not enough of a JUnit expert to know the best way. Any ideas from folks more versed in Rules, etc?

          Show
          Todd Lipcon added a comment - Another option might be to require that all classes containing test methods be annotated with a JUnit rule which enforces default timeouts. I'm not enough of a JUnit expert to know the best way. Any ideas from folks more versed in Rules, etc?
          Hide
          Tsz Wo Nicholas Sze added a comment -

          > ..., which for some reason doesn't show up as a failed test in the test-patch output. ...

          Should we fix the test-patch output instead?

          Show
          Tsz Wo Nicholas Sze added a comment - > ..., which for some reason doesn't show up as a failed test in the test-patch output. ... Should we fix the test-patch output instead?
          Hide
          Todd Lipcon added a comment -

          I'm not sure if it's an issue with the test-patch grepping of the results, or if it's an underlying surefire issue. Of course that would be simplest if possible.

          Show
          Todd Lipcon added a comment - I'm not sure if it's an issue with the test-patch grepping of the results, or if it's an underlying surefire issue. Of course that would be simplest if possible.
          Hide
          Robert Joseph Evans added a comment -

          test-patch runs maven with -fn. This means that maven will always return an exit code of 0, so we use grep to find test failures but if a test exits early then no xml file is generated for that test. which means that grep cannot find it. If we did not have the -fn then if one test failed it could prevent other tests from running. We could change it so we capture the output of running the test and then check for

          [INFO] ------------------------------------------------------------------------
          [INFO] BUILD FAILURE
          [INFO] ------------------------------------------------------------------------
          

          That always appears after failures even when mvn returned a 0. Or we could try and find another way to have maven detect the error. -Dmaven.test.failure.ignore=true does similar things to -fn but only for test failures. Test errors like an unexpected exception being thrown, a test timing out, or someone in the test calling system.exit will result in not running all of the tests. I think adding in the BUILD FAILURE grep is preferable, but it will not tell us in an easy way which tests timed out.

          Show
          Robert Joseph Evans added a comment - test-patch runs maven with -fn. This means that maven will always return an exit code of 0, so we use grep to find test failures but if a test exits early then no xml file is generated for that test. which means that grep cannot find it. If we did not have the -fn then if one test failed it could prevent other tests from running. We could change it so we capture the output of running the test and then check for [INFO] ------------------------------------------------------------------------ [INFO] BUILD FAILURE [INFO] ------------------------------------------------------------------------ That always appears after failures even when mvn returned a 0. Or we could try and find another way to have maven detect the error. -Dmaven.test.failure.ignore=true does similar things to -fn but only for test failures. Test errors like an unexpected exception being thrown, a test timing out, or someone in the test calling system.exit will result in not running all of the tests. I think adding in the BUILD FAILURE grep is preferable, but it will not tell us in an easy way which tests timed out.
          Hide
          Aaron T. Myers added a comment -

          The output when a test times out is to have two lines right in a row which name a test, without outputting any "Tests run:..." line in between. Given that, in the past the way I've identified those tests which timed out or exited early is by running this command on the output:

          $ pcregrep -M 'Running org\.apache\.hadoop.*\n[^(Tests run:)]' <file name>

          Which will output any tests which were executed, but which surefire wasn't able to determine which test cases that ran.

          Show
          Aaron T. Myers added a comment - The output when a test times out is to have two lines right in a row which name a test, without outputting any "Tests run:..." line in between. Given that, in the past the way I've identified those tests which timed out or exited early is by running this command on the output: $ pcregrep -M ' Running org\.apache\.hadoop.*\n [^(Tests run:)] ' <file name> Which will output any tests which were executed, but which surefire wasn't able to determine which test cases that ran.
          Hide
          Surenkumar Nihalani added a comment -

          I think I can write a java program that accesses annotations through reflection and checks for existence of timeouts when passed a list of java files. That should do it, right?

          This is also an option:
          http://www.attivio.com/blog/56-java-development/379-enhancing-junit-with-default-timeouts-and-stack-traces.html

          Show
          Surenkumar Nihalani added a comment - I think I can write a java program that accesses annotations through reflection and checks for existence of timeouts when passed a list of java files. That should do it, right? This is also an option: http://www.attivio.com/blog/56-java-development/379-enhancing-junit-with-default-timeouts-and-stack-traces.html
          Hide
          Aaron T. Myers added a comment -

          Please be careful with using code from that blog post. From a quick look, it's not obvious what license it's under.

          Show
          Aaron T. Myers added a comment - Please be careful with using code from that blog post. From a quick look, it's not obvious what license it's under.
          Hide
          Surenkumar Nihalani added a comment -

          while adding default timeout sounds like a patch but not the solution. I was looking at reflection's API. It's good enough for us to check Timeout for each Test annotation (assuming the test follows JUnit 4 format) however for it to work we need a class object to start with. There are two ways to get that

          1. From an instance, like, instance.getClass()
          2. From the fully qualified name String, like: Class c = Class.forName("com.duke.MyLocaleServiceProvider");

          Getting an instance of the instance of the test object seems to be tough.
          Using the fully qualified name seems to be a good approach to do this. Now, from a given file path, there is no clean way to get to fully qualified name. I think grepping for "package *;" and "public class [w]+ " and concatenating results and passing it down to a java program as an argument should be do-able.

          After I have a reference to a class object, it's easy to check if all the methods have annotations of junit.Test instance have a timeout variable.

          Thoughts?

          Show
          Surenkumar Nihalani added a comment - while adding default timeout sounds like a patch but not the solution. I was looking at reflection's API. It's good enough for us to check Timeout for each Test annotation (assuming the test follows JUnit 4 format) however for it to work we need a class object to start with. There are two ways to get that From an instance, like, instance.getClass() From the fully qualified name String, like: Class c = Class.forName("com.duke.MyLocaleServiceProvider"); Getting an instance of the instance of the test object seems to be tough. Using the fully qualified name seems to be a good approach to do this. Now, from a given file path, there is no clean way to get to fully qualified name. I think grepping for "package *;" and "public class [w] + " and concatenating results and passing it down to a java program as an argument should be do-able. After I have a reference to a class object, it's easy to check if all the methods have annotations of junit.Test instance have a timeout variable. Thoughts?
          Hide
          Todd Lipcon added a comment -

          I think you're probably better off using the Java annotation processing tool (apt): http://docs.oracle.com/javase/6/docs/technotes/guides/apt/index.html

          Show
          Todd Lipcon added a comment - I think you're probably better off using the Java annotation processing tool (apt): http://docs.oracle.com/javase/6/docs/technotes/guides/apt/index.html
          Hide
          Surenkumar Nihalani added a comment -

          apt tool has been deprecated since jdk 7 and will removed in the next release. http://docs.oracle.com/javase/7/docs/technotes/guides/apt/GettingStarted.html

          Should I start with apt's API or jdk 7's?

          Show
          Surenkumar Nihalani added a comment - apt tool has been deprecated since jdk 7 and will removed in the next release. http://docs.oracle.com/javase/7/docs/technotes/guides/apt/GettingStarted.html Should I start with apt's API or jdk 7's?
          Hide
          Surenkumar Nihalani added a comment -

          Any thoughts?

          Show
          Surenkumar Nihalani added a comment - Any thoughts?
          Hide
          Robert Joseph Evans added a comment -

          There are a lot of people still using JDK 6. All of our build boxes are still using JDK 6. I would start with APT. Either that or have the build boxes move to JDK 7 and have us officially drop support for JDK 6 in Hadoop.

          Show
          Robert Joseph Evans added a comment - There are a lot of people still using JDK 6. All of our build boxes are still using JDK 6. I would start with APT. Either that or have the build boxes move to JDK 7 and have us officially drop support for JDK 6 in Hadoop.
          Hide
          Chris Nauroth added a comment -

          I wonder if it's simpler to start with a regex matching approach, try it for a while, and see if that turns out to be good enough in practice that we don't need to actually introspect the code with apt or similar. We could grep -c the patch file for addition of lines (presence of '+') that have "@Test" not followed by "(timeout=...)". If the count is non-zero, then Jenkins votes -1.

          Here is an example of a regex that matches "+@Test" and "+@Test()" but not "+@Test(timeout=123)".

          scala> val p = Pattern.compile("\\+@Test(?!\\(timeout|timeout=\\d+\\))")
          val p = Pattern.compile("\\+@Test(?!\\(timeout|timeout=\\d+\\))")
          p: java.util.regex.Pattern = \+@Test(?!\(timeout|timeout=\d+\))
          
          scala> val strings = List("+@Test", "+@Test()", "+@Test(timeout=123)")
          val strings = List("+@Test", "+@Test()", "+@Test(timeout=123)")
          strings: List[java.lang.String] = List(+@Test, +@Test(), +@Test(timeout=123))
          
          scala> strings foreach((s: String) => { println(s + " matches? " + p.matcher(s).find) })
          find) })
          +@Test matches? true
          +@Test() matches? true
          +@Test(timeout=123) matches? false
          

          I happened to have a Scala shell opened, so that's where I experimented. In practice, we'd need to pick something on Jenkins that has regular expression support for negative lookahead. I don't recall if stock grep supports that.

          The regex above is incomplete. It would need additional work to handle whitespace correctly, and I'm sure there are other edge cases I haven't thought of yet.

          Show
          Chris Nauroth added a comment - I wonder if it's simpler to start with a regex matching approach, try it for a while, and see if that turns out to be good enough in practice that we don't need to actually introspect the code with apt or similar. We could grep -c the patch file for addition of lines (presence of '+') that have "@Test" not followed by "(timeout=...)". If the count is non-zero, then Jenkins votes -1. Here is an example of a regex that matches "+@Test" and "+@Test()" but not "+@Test(timeout=123)". scala> val p = Pattern.compile( "\\+@Test(?!\\(timeout|timeout=\\d+\\))" ) val p = Pattern.compile( "\\+@Test(?!\\(timeout|timeout=\\d+\\))" ) p: java.util.regex.Pattern = \+@Test(?!\(timeout|timeout=\d+\)) scala> val strings = List( "+@Test" , "+@Test()" , "+@Test(timeout=123)" ) val strings = List( "+@Test" , "+@Test()" , "+@Test(timeout=123)" ) strings: List[java.lang. String ] = List(+@Test, +@Test(), +@Test(timeout=123)) scala> strings foreach((s: String ) => { println(s + " matches? " + p.matcher(s).find) }) find) }) +@Test matches? true +@Test() matches? true +@Test(timeout=123) matches? false I happened to have a Scala shell opened, so that's where I experimented. In practice, we'd need to pick something on Jenkins that has regular expression support for negative lookahead. I don't recall if stock grep supports that. The regex above is incomplete. It would need additional work to handle whitespace correctly, and I'm sure there are other edge cases I haven't thought of yet.
          Hide
          Surenkumar Nihalani added a comment -

          Chris Nauroth, I see this approach is definitely better than having to make compatibility decisions at this point where we haven't jumped to JDK 7 safely just yet.

          I plan to go ahead with this and a write a shell snippet tomorrow. If anyone has a better approach or any reasons not to move forward this one, let me know.

          Show
          Surenkumar Nihalani added a comment - Chris Nauroth , I see this approach is definitely better than having to make compatibility decisions at this point where we haven't jumped to JDK 7 safely just yet. I plan to go ahead with this and a write a shell snippet tomorrow. If anyone has a better approach or any reasons not to move forward this one, let me know.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12568725/HADOOP-9112-1.patch
          against trunk revision .

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

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

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

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

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

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

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

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2172//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2172//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/12568725/HADOOP-9112-1.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in . +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2172//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2172//console This message is automatically generated.
          Hide
          Surenkumar Nihalani added a comment -

          The patch isn't stable yet. The second case where I am trying to match "@Test" is a bit unstable.

          • It would pass cases like @Test (timeout=x) but it covers empty @Test cases.
          • This isn't new line safe yet.
          Show
          Surenkumar Nihalani added a comment - The patch isn't stable yet. The second case where I am trying to match "@Test" is a bit unstable. It would pass cases like @Test (timeout=x) but it covers empty @Test cases. This isn't new line safe yet.
          Hide
          Surenkumar Nihalani added a comment -

          Turns out, negative look ahead isn't supported by extended regular expression. I am going to use perl option in grep. However, on Mac, the grep doesn't support perl regex while GNU's grep does. Can anyone confirm that the Jenkins system uses GNU grep?

          If not, there is an option in grep to return lines that don't match a pattern and I can simulate it, but negative lookahead would be robust.

          Show
          Surenkumar Nihalani added a comment - Turns out, negative look ahead isn't supported by extended regular expression. I am going to use perl option in grep. However, on Mac, the grep doesn't support perl regex while GNU's grep does. Can anyone confirm that the Jenkins system uses GNU grep? If not, there is an option in grep to return lines that don't match a pattern and I can simulate it, but negative lookahead would be robust.
          Hide
          Surenkumar Nihalani added a comment -

          There are five possibilities for JUnit's test annonation as per the documentation:

          • @Test
          • @Test()
          • @Test(timeout=x)
          • @Test(exception=x.class)
          • @Test(timeout=x, exception=y.class)
          • @Test(exception=y.class, timeout=y)

          Out of these, the current regex detects first three and we should fine. However, I couldn't get it to be newline safe. If any knows how to, feel free to update the patch. Thanks.

          Show
          Surenkumar Nihalani added a comment - There are five possibilities for JUnit's test annonation as per the documentation: @Test @Test() @Test(timeout=x) @Test(exception=x.class) @Test(timeout=x, exception=y.class) @Test(exception=y.class, timeout=y) Out of these, the current regex detects first three and we should fine. However, I couldn't get it to be newline safe. If any knows how to, feel free to update the patch. Thanks.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12568927/HADOOP-9112-2.patch
          against trunk revision .

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

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

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

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

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

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

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

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2177//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2177//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/12568927/HADOOP-9112-2.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in . +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2177//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2177//console This message is automatically generated.
          Hide
          Surenkumar Nihalani added a comment -

          Corrected the newline problem. Patch is complete.

          Show
          Surenkumar Nihalani added a comment - Corrected the newline problem. Patch is complete.
          Hide
          Surenkumar Nihalani added a comment -

          Request for code review.

          Show
          Surenkumar Nihalani added a comment - Request for code review.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12568932/HADOOP-9112-3.patch
          against trunk revision .

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

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

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

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

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

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

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

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2178//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2178//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/12568932/HADOOP-9112-3.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in . +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2178//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2178//console This message is automatically generated.
          Hide
          Surenkumar Nihalani added a comment -

          Request for code review.

          Show
          Surenkumar Nihalani added a comment - Request for code review.
          Hide
          Robert Joseph Evans added a comment -

          A quick look seems reasonable, but I have not had time to dig deeply into the REGEXP yet. Could you replace the grep with $GREP and add in similar code to support $TR instead of calling tr directly?

          Also what operating systems/distros have you run this on? We want to be fairly conservative in adding in new dependencies, and want to be sure that it at least works out of the box on stock Ubuntu, RedHat, and MacOS X.

          Show
          Robert Joseph Evans added a comment - A quick look seems reasonable, but I have not had time to dig deeply into the REGEXP yet. Could you replace the grep with $GREP and add in similar code to support $TR instead of calling tr directly? Also what operating systems/distros have you run this on? We want to be fairly conservative in adding in new dependencies, and want to be sure that it at least works out of the box on stock Ubuntu, RedHat, and MacOS X.
          Hide
          Robert Joseph Evans added a comment -

          The regexp seems reasonable. There are still some corner cases where it may produce a false -1, but all that I can think of are invalid java and would probably require a lot more complex code to get it right. Once you fix the $GREP and $TR issue I am a +1 for this.

          Show
          Robert Joseph Evans added a comment - The regexp seems reasonable. There are still some corner cases where it may produce a false -1, but all that I can think of are invalid java and would probably require a lot more complex code to get it right. Once you fix the $GREP and $TR issue I am a +1 for this.
          Hide
          Surenkumar Nihalani added a comment -

          The reason I didn't bother with other corner cases is because buildWithPatch is called before checkTests, so the robust java compiler would make sure that java syntax language is correct before we get to check for @Test.

          Updated patch to parametrize tr.

          Show
          Surenkumar Nihalani added a comment - The reason I didn't bother with other corner cases is because buildWithPatch is called before checkTests, so the robust java compiler would make sure that java syntax language is correct before we get to check for @Test. Updated patch to parametrize tr.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12570039/HADOOP-9112-4.patch
          against trunk revision .

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

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

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

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

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

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

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

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2211//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2211//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/12570039/HADOOP-9112-4.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in . +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2211//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2211//console This message is automatically generated.
          Hide
          Robert Joseph Evans added a comment -

          Looks good. There is no parameter to replace TR from the command line like there is for GREP or the others. This is fairly minor, especially because tr should be on the path for just about everyone, and tr has not really changed in a long time.

          I am fine with checking it in as is, but it would probably be best to just add it in.

          Show
          Robert Joseph Evans added a comment - Looks good. There is no parameter to replace TR from the command line like there is for GREP or the others. This is fairly minor, especially because tr should be on the path for just about everyone, and tr has not really changed in a long time. I am fine with checking it in as is, but it would probably be best to just add it in.
          Hide
          Surenkumar Nihalani added a comment -

          Using awk instead of tr now. No need to add another dependency.

          Show
          Surenkumar Nihalani added a comment - Using awk instead of tr now. No need to add another dependency.
          Hide
          Surenkumar Nihalani added a comment -

          Initially, awk was deleting newlines. Now it's substituting newlines with space. So, anything just newline separated is a safe input to work with.

          Show
          Surenkumar Nihalani added a comment - Initially, awk was deleting newlines. Now it's substituting newlines with space. So, anything just newline separated is a safe input to work with.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12570143/HADOOP-9112-5.patch
          against trunk revision .

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

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

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

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

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

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

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

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2214//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2214//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/12570143/HADOOP-9112-5.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in . +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2214//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2214//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12570144/HADOOP-9112-6.patch
          against trunk revision .

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

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

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

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

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

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

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

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2215//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2215//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/12570144/HADOOP-9112-6.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in . +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2215//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2215//console This message is automatically generated.
          Hide
          Robert Joseph Evans added a comment -

          Looks good. Thanks for your patience on this. +1. I'll check it in.

          Show
          Robert Joseph Evans added a comment - Looks good. Thanks for your patience on this. +1. I'll check it in.
          Hide
          Robert Joseph Evans added a comment -

          Thanks Surenkumar,

          I checked this into trunk.

          Show
          Robert Joseph Evans added a comment - Thanks Surenkumar, I checked this into trunk.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-trunk-Commit #3367 (See https://builds.apache.org/job/Hadoop-trunk-Commit/3367/)
          HADOOP-9112. test-patch should -1 for @Tests without a timeout (Surenkumar Nihalani via bobby) (Revision 1448285)

          Result = SUCCESS
          bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1448285
          Files :

          • /hadoop/common/trunk/dev-support/test-patch.sh
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Show
          Hudson added a comment - Integrated in Hadoop-trunk-Commit #3367 (See https://builds.apache.org/job/Hadoop-trunk-Commit/3367/ ) HADOOP-9112 . test-patch should -1 for @Tests without a timeout (Surenkumar Nihalani via bobby) (Revision 1448285) Result = SUCCESS bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1448285 Files : /hadoop/common/trunk/dev-support/test-patch.sh /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Hide
          Surenkumar Nihalani added a comment -

          Hadoop QA seems to -1 w/o reason some places. Looks like, I was returning wrong return codes.

          Show
          Surenkumar Nihalani added a comment - Hadoop QA seems to -1 w/o reason some places. Looks like, I was returning wrong return codes.
          Hide
          Surenkumar Nihalani added a comment -

          minor bug - Wrong return codes.

          Show
          Surenkumar Nihalani added a comment - minor bug - Wrong return codes.
          Hide
          Steve Loughran added a comment -

          sorry, I've only just seen this. As the only person on *-dev whose ever written >1 JUnit test runner, I do encourage people to point me at these kind of JIRAs.

          Timing out tests without explicit timeout attributes are probably dealt with by having maven killing the JUnit running process. Due to the (historical, flawed) fact that the XML results stick the summary data up as attributes on the root XML node, XML report generators have to buffer up the entire file before writing. Killed process => no output. Text reporters shouldn't have this problem, a past XHTML reporter I did would stream HTML out and provide something useful, along with colour coding log4j outputs based on severity levels. OneHostHtmlListener . Fixing the Ant-originated XML and tooling would be the ideal outcome here, just hard, as you have to not just delve into Ant and Maven code, but into downstream tools like Jenkins.

          One problem with timeout= attributes is that they can be very brittle -my test running machine may be slower than yours. We need a standard recommended (long) test run time, which should be minutes, just to ensure that the Maven JUnit4 runner runs it in timeout mode. It doesn't matter what the timeout is as long as it is >0, less than the time it takes to complete on everyones boxes/VMs, and less than the absolute maven test run timeout.

          Once the @Test(timeout) property is set, themselves can raise TimeoutException, which is translated into a timeout -so permitting tests to implement their own (property-configurable) timeout logic.

          Show
          Steve Loughran added a comment - sorry, I've only just seen this. As the only person on *-dev whose ever written >1 JUnit test runner, I do encourage people to point me at these kind of JIRAs. Timing out tests without explicit timeout attributes are probably dealt with by having maven killing the JUnit running process. Due to the (historical, flawed) fact that the XML results stick the summary data up as attributes on the root XML node, XML report generators have to buffer up the entire file before writing. Killed process => no output. Text reporters shouldn't have this problem, a past XHTML reporter I did would stream HTML out and provide something useful, along with colour coding log4j outputs based on severity levels. OneHostHtmlListener . Fixing the Ant-originated XML and tooling would be the ideal outcome here, just hard, as you have to not just delve into Ant and Maven code, but into downstream tools like Jenkins. One problem with timeout= attributes is that they can be very brittle -my test running machine may be slower than yours. We need a standard recommended (long) test run time, which should be minutes, just to ensure that the Maven JUnit4 runner runs it in timeout mode. It doesn't matter what the timeout is as long as it is >0, less than the time it takes to complete on everyones boxes/VMs, and less than the absolute maven test run timeout. Once the @Test(timeout) property is set, themselves can raise TimeoutException , which is translated into a timeout -so permitting tests to implement their own (property-configurable) timeout logic.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Yarn-trunk #134 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/134/)
          HADOOP-9112. test-patch should -1 for @Tests without a timeout (Surenkumar Nihalani via bobby) (Revision 1448285)

          Result = SUCCESS
          bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1448285
          Files :

          • /hadoop/common/trunk/dev-support/test-patch.sh
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Show
          Hudson added a comment - Integrated in Hadoop-Yarn-trunk #134 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/134/ ) HADOOP-9112 . test-patch should -1 for @Tests without a timeout (Surenkumar Nihalani via bobby) (Revision 1448285) Result = SUCCESS bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1448285 Files : /hadoop/common/trunk/dev-support/test-patch.sh /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Hide
          Nicolas Liochon added a comment -

          An issue I have with timeouts is that we have to change them during debugging may be there is an option I don't knwow.

          Anyway, a test process can fail in the afterSuite (basically, when you're shutting down the cluster). And surefire may not kill it, and you won't know, and you will find it at the next build.

          In HBase, we do that before running the tests:

              1. kill any process remaining from another test, maybe even another project
                jps | grep surefirebooter | cut -d ' ' -f 1 | xargs kill -9 2>/dev/null

          And this after
          ZOMBIE_TESTS_COUNT=`jps | grep surefirebooter | wc -l`
          if [[ $ZOMBIE_TESTS_COUNT != 0 ]] ; then
          #It seems sometimes the tests are not dying immediately. Let's give them 30s
          echo "Suspicious java process found - waiting 30s to see if there are just slow to stop"
          sleep 30
          ZOMBIE_TESTS_COUNT=`jps | grep surefirebooter | wc -l`
          if [[ $ZOMBIE_TESTS_COUNT != 0 ]] ; then
          echo "There are $ZOMBIE_TESTS_COUNT zombie tests, they should have been killed by surefire but survived"
          echo "************ BEGIN zombies jstack extract"
          ZB_STACK=`jps | grep surefirebooter | cut -d ' ' -f 1 | xargs -n 1 jstack | grep ".test" | grep "\.java"`
          jps | grep surefirebooter | cut -d ' ' -f 1 | xargs -n 1 jstack
          echo "************ END zombies jstack extract"
          JIRA_COMMENT="$JIRA_COMMENT

          -1 core zombie tests. There are $

          {ZOMBIE_TESTS_COUNT}

          zombie test(s): $

          {ZB_STACK}

          "
          BAD=1
          jps | grep surefirebooter | cut -d ' ' -f 1 | xargs kill -9
          else
          echo "We're ok: there is no zombie test, but some tests took some time to stop"
          fi
          else
          echo "We're ok: there is no zombie test"
          fi

          See http://www.mail-archive.com/issues@hbase.apache.org/msg73169.html for the outcome (it's actually a hdfs zombie, this was before we started killing the zombies at the beginning of our tests). The whole stack is in the build logs.

          It has improved the precommit success ratio.

          It was my two cents

          Show
          Nicolas Liochon added a comment - An issue I have with timeouts is that we have to change them during debugging may be there is an option I don't knwow . Anyway, a test process can fail in the afterSuite (basically, when you're shutting down the cluster). And surefire may not kill it, and you won't know, and you will find it at the next build. In HBase, we do that before running the tests: kill any process remaining from another test, maybe even another project jps | grep surefirebooter | cut -d ' ' -f 1 | xargs kill -9 2>/dev/null And this after ZOMBIE_TESTS_COUNT=`jps | grep surefirebooter | wc -l` if [[ $ZOMBIE_TESTS_COUNT != 0 ]] ; then #It seems sometimes the tests are not dying immediately. Let's give them 30s echo "Suspicious java process found - waiting 30s to see if there are just slow to stop" sleep 30 ZOMBIE_TESTS_COUNT=`jps | grep surefirebooter | wc -l` if [[ $ZOMBIE_TESTS_COUNT != 0 ]] ; then echo "There are $ZOMBIE_TESTS_COUNT zombie tests, they should have been killed by surefire but survived" echo "************ BEGIN zombies jstack extract" ZB_STACK=`jps | grep surefirebooter | cut -d ' ' -f 1 | xargs -n 1 jstack | grep ".test" | grep "\.java"` jps | grep surefirebooter | cut -d ' ' -f 1 | xargs -n 1 jstack echo "************ END zombies jstack extract" JIRA_COMMENT="$JIRA_COMMENT -1 core zombie tests . There are $ {ZOMBIE_TESTS_COUNT} zombie test(s): $ {ZB_STACK} " BAD=1 jps | grep surefirebooter | cut -d ' ' -f 1 | xargs kill -9 else echo "We're ok: there is no zombie test, but some tests took some time to stop" fi else echo "We're ok: there is no zombie test" fi See http://www.mail-archive.com/issues@hbase.apache.org/msg73169.html for the outcome (it's actually a hdfs zombie, this was before we started killing the zombies at the beginning of our tests). The whole stack is in the build logs. It has improved the precommit success ratio. It was my two cents
          Hide
          Surenkumar Nihalani added a comment -

          Steve Loughran, having a recommended value works, I was thinking of having intermediate value substitution in test-patch. If the value is x, and the coefficient of my machine (>= 1), we could have it configurable so that it substitutes (long)(coefficient * valueOfTimeoutForThat@Test) This way if anyone faces timeout exceptions, we can keep on increasing the configurable coefficient till all of the tests pass as part of initial setup.

          Would that be too much overhead for configuration?

          Show
          Surenkumar Nihalani added a comment - Steve Loughran , having a recommended value works, I was thinking of having intermediate value substitution in test-patch. If the value is x, and the coefficient of my machine (>= 1), we could have it configurable so that it substitutes (long)(coefficient * valueOfTimeoutForThat@Test) This way if anyone faces timeout exceptions, we can keep on increasing the configurable coefficient till all of the tests pass as part of initial setup. Would that be too much overhead for configuration?
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #1323 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1323/)
          HADOOP-9112. test-patch should -1 for @Tests without a timeout (Surenkumar Nihalani via bobby) (Revision 1448285)

          Result = SUCCESS
          bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1448285
          Files :

          • /hadoop/common/trunk/dev-support/test-patch.sh
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1323 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1323/ ) HADOOP-9112 . test-patch should -1 for @Tests without a timeout (Surenkumar Nihalani via bobby) (Revision 1448285) Result = SUCCESS bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1448285 Files : /hadoop/common/trunk/dev-support/test-patch.sh /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #1351 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1351/)
          HADOOP-9112. test-patch should -1 for @Tests without a timeout (Surenkumar Nihalani via bobby) (Revision 1448285)

          Result = FAILURE
          bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1448285
          Files :

          • /hadoop/common/trunk/dev-support/test-patch.sh
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1351 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1351/ ) HADOOP-9112 . test-patch should -1 for @Tests without a timeout (Surenkumar Nihalani via bobby) (Revision 1448285) Result = FAILURE bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1448285 Files : /hadoop/common/trunk/dev-support/test-patch.sh /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Hide
          Robert Joseph Evans added a comment -

          Sorry I should have caught the return code being wrong. I just checked in the fixed return codes in version 7 of the patch.

          Show
          Robert Joseph Evans added a comment - Sorry I should have caught the return code being wrong. I just checked in the fixed return codes in version 7 of the patch.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-trunk-Commit #3374 (See https://builds.apache.org/job/Hadoop-trunk-Commit/3374/)
          amendment to HADOOP-9112 fix return codes (Surenkumar Nihalani via bobby) (Revision 1448745)

          Result = SUCCESS
          bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1448745
          Files :

          • /hadoop/common/trunk/dev-support/test-patch.sh
          Show
          Hudson added a comment - Integrated in Hadoop-trunk-Commit #3374 (See https://builds.apache.org/job/Hadoop-trunk-Commit/3374/ ) amendment to HADOOP-9112 fix return codes (Surenkumar Nihalani via bobby) (Revision 1448745) Result = SUCCESS bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1448745 Files : /hadoop/common/trunk/dev-support/test-patch.sh
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Yarn-trunk #135 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/135/)
          amendment to HADOOP-9112 fix return codes (Surenkumar Nihalani via bobby) (Revision 1448745)

          Result = SUCCESS
          bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1448745
          Files :

          • /hadoop/common/trunk/dev-support/test-patch.sh
          Show
          Hudson added a comment - Integrated in Hadoop-Yarn-trunk #135 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/135/ ) amendment to HADOOP-9112 fix return codes (Surenkumar Nihalani via bobby) (Revision 1448745) Result = SUCCESS bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1448745 Files : /hadoop/common/trunk/dev-support/test-patch.sh
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #1324 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1324/)
          amendment to HADOOP-9112 fix return codes (Surenkumar Nihalani via bobby) (Revision 1448745)

          Result = FAILURE
          bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1448745
          Files :

          • /hadoop/common/trunk/dev-support/test-patch.sh
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1324 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1324/ ) amendment to HADOOP-9112 fix return codes (Surenkumar Nihalani via bobby) (Revision 1448745) Result = FAILURE bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1448745 Files : /hadoop/common/trunk/dev-support/test-patch.sh
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #1352 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1352/)
          amendment to HADOOP-9112 fix return codes (Surenkumar Nihalani via bobby) (Revision 1448745)

          Result = SUCCESS
          bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1448745
          Files :

          • /hadoop/common/trunk/dev-support/test-patch.sh
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1352 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1352/ ) amendment to HADOOP-9112 fix return codes (Surenkumar Nihalani via bobby) (Revision 1448745) Result = SUCCESS bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1448745 Files : /hadoop/common/trunk/dev-support/test-patch.sh
          Hide
          Konstantin Boudnik added a comment -

          So, why solving a pure Maven issue with policy enforcement that still causing head-aches? What's wrong with surefire.timeout setting? Won't it achieve the same essentially without burning everyone with quite an artificial requirement to set a timeout on each test?

          Show
          Konstantin Boudnik added a comment - So, why solving a pure Maven issue with policy enforcement that still causing head-aches? What's wrong with surefire.timeout setting? Won't it achieve the same essentially without burning everyone with quite an artificial requirement to set a timeout on each test?
          Hide
          Steve Loughran added a comment -

          I've added a new JIRA, HADOOP-9330, which proposes the proper way to fix this.

          As it is, I consider embedded timeouts dangerous and think this change should not be reverted. Certainly it could have been publicised more.

          Show
          Steve Loughran added a comment - I've added a new JIRA, HADOOP-9330 , which proposes the proper way to fix this. As it is, I consider embedded timeouts dangerous and think this change should not be reverted. Certainly it could have been publicised more.
          Hide
          Luke Lu added a comment -

          We had a timeout problem. We added regex to enforce per test timeout. Now we have two problems.

          Seriously, IMO, we should use org.junit.rules.Timeout in a base test class and be done with it.

          Show
          Luke Lu added a comment - We had a timeout problem. We added regex to enforce per test timeout. Now we have two problems. Seriously, IMO, we should use org.junit.rules.Timeout in a base test class and be done with it.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          The script will -1 on an existing @Test (i.e. not added by the submitted patch). I think it is wrong.

          How about reverting this? If there is no objection, I will revert this tomorrow.

          Show
          Tsz Wo Nicholas Sze added a comment - The script will -1 on an existing @Test (i.e. not added by the submitted patch). I think it is wrong. How about reverting this? If there is no objection, I will revert this tomorrow.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          Reverted r1448285 and r1448745.

          Show
          Tsz Wo Nicholas Sze added a comment - Reverted r1448285 and r1448745.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-trunk-Commit #3518 (See https://builds.apache.org/job/Hadoop-trunk-Commit/3518/)
          svn merge -c -1448745 and -1448285 for reverting HADOOP-9112: test-patch should -1 for @Tests without a timeout (Revision 1460520)

          Result = SUCCESS
          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1460520
          Files :

          • /hadoop/common/trunk/dev-support/test-patch.sh
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Show
          Hudson added a comment - Integrated in Hadoop-trunk-Commit #3518 (See https://builds.apache.org/job/Hadoop-trunk-Commit/3518/ ) svn merge -c -1448745 and -1448285 for reverting HADOOP-9112 : test-patch should -1 for @Tests without a timeout (Revision 1460520) Result = SUCCESS szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1460520 Files : /hadoop/common/trunk/dev-support/test-patch.sh /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Yarn-trunk #166 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/166/)
          svn merge -c -1448745 and -1448285 for reverting HADOOP-9112: test-patch should -1 for @Tests without a timeout (Revision 1460520)

          Result = SUCCESS
          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1460520
          Files :

          • /hadoop/common/trunk/dev-support/test-patch.sh
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Show
          Hudson added a comment - Integrated in Hadoop-Yarn-trunk #166 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/166/ ) svn merge -c -1448745 and -1448285 for reverting HADOOP-9112 : test-patch should -1 for @Tests without a timeout (Revision 1460520) Result = SUCCESS szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1460520 Files : /hadoop/common/trunk/dev-support/test-patch.sh /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #1355 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1355/)
          svn merge -c -1448745 and -1448285 for reverting HADOOP-9112: test-patch should -1 for @Tests without a timeout (Revision 1460520)

          Result = FAILURE
          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1460520
          Files :

          • /hadoop/common/trunk/dev-support/test-patch.sh
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1355 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1355/ ) svn merge -c -1448745 and -1448285 for reverting HADOOP-9112 : test-patch should -1 for @Tests without a timeout (Revision 1460520) Result = FAILURE szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1460520 Files : /hadoop/common/trunk/dev-support/test-patch.sh /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #1383 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1383/)
          svn merge -c -1448745 and -1448285 for reverting HADOOP-9112: test-patch should -1 for @Tests without a timeout (Revision 1460520)

          Result = FAILURE
          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1460520
          Files :

          • /hadoop/common/trunk/dev-support/test-patch.sh
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1383 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1383/ ) svn merge -c -1448745 and -1448285 for reverting HADOOP-9112 : test-patch should -1 for @Tests without a timeout (Revision 1460520) Result = FAILURE szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1460520 Files : /hadoop/common/trunk/dev-support/test-patch.sh /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt

            People

            • Assignee:
              Surenkumar Nihalani
              Reporter:
              Todd Lipcon
            • Votes:
              0 Vote for this issue
              Watchers:
              19 Start watching this issue

              Dates

              • Created:
                Updated:

                Development