Hadoop Common
  1. Hadoop Common
  2. HADOOP-5917

Testpatch isn't catching newly introduced javac warnings

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: test
    • Labels:
      None

      Description

      Testpatch doesn't seem to be catching newly introduced javac warnings, as detailed in the results of the experiment below.

        Issue Links

          Activity

          Hide
          Tsz Wo Nicholas Sze added a comment -

          test-patch.sh indeed has more problems. See HADOOP-6124.

          Show
          Tsz Wo Nicholas Sze added a comment - test-patch.sh indeed has more problems. See HADOOP-6124 .
          Hide
          Jakob Homan added a comment -

          Two options would be either remove the maxwarns restriction, which would generate quite a lot of warnings now due to the mapred/mapreduce deprecation issue, or to explicitly ignore the deprecation warnings in lint. Another option, which Nicholas mentioned, is to wait for the project split and see if there's a new testpatch script that will be more accurate against hdfs. This seems reasonable to me.

          Show
          Jakob Homan added a comment - Two options would be either remove the maxwarns restriction, which would generate quite a lot of warnings now due to the mapred/mapreduce deprecation issue, or to explicitly ignore the deprecation warnings in lint. Another option, which Nicholas mentioned, is to wait for the project split and see if there's a new testpatch script that will be more accurate against hdfs. This seems reasonable to me.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          It seems that test-patch only checks the first 1000 warnings since the javac args include "-Xmaxwarns 1000".

          Show
          Tsz Wo Nicholas Sze added a comment - It seems that test-patch only checks the first 1000 warnings since the javac args include "-Xmaxwarns 1000".
          Hide
          Jakob Homan added a comment -

          As part of the recent cleanup, I fixed all of the warnings in the TestDataTransferProtcol (HADOOP-5822). This has been applied to trunk. To test if testpatch is working correctly, I re-introduced a minor javac warning, a redundant cast:

          --- src/test/hdfs/org/apache/hadoop/hdfs/TestDataTransferProtocol.java
          +++ src/test/hdfs/org/apache/hadoop/hdfs/TestDataTransferProtocol.java
          @@ -194,7 +194,7 @@ public class TestDataTransferProtocol extends TestCase {
               sendBuf.reset();
               recvBuf.reset();
               sendOut.writeShort((short)DataTransferProtocol.DATA_TRANSFER_VERSION);
          -    sendOut.writeByte(DataTransferProtocol.OP_WRITE_BLOCK);
          +    sendOut.writeByte((byte)DataTransferProtocol.OP_WRITE_BLOCK);
               sendOut.writeLong(newBlockId);
               sendOut.writeLong(0);          // generation stamp
               sendOut.writeInt(0);           // targets in pipeline 
          

          This indeed is flagged as a warning when running with the lint parameter:

              [javac] /Users/jhoman/work/git/hadoop/src/test/hdfs/org/apache/hadoop/hdfs/TestDataTransferProtocol.java:197: warning: [cast] redundant cast to byte
              [javac]     sendOut.writeByte((byte)DataTransferProtocol.OP_WRITE_BLOCK);
          

          However, testpatch gives this patch a clean bill of health:

               [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 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 Eclipse classpath. The patch retains Eclipse classpath integrity.
               [exec] 
               [exec]     +1 release audit.  The applied patch does not increase the total number of release audit warnings.
          

          Perhaps the section of test-patch that calculates the difference in warnings got silently broken? Fixing this will hopefully prevent another round of spring cleaning next year.

          Show
          Jakob Homan added a comment - As part of the recent cleanup, I fixed all of the warnings in the TestDataTransferProtcol ( HADOOP-5822 ). This has been applied to trunk. To test if testpatch is working correctly, I re-introduced a minor javac warning, a redundant cast: --- src/test/hdfs/org/apache/hadoop/hdfs/TestDataTransferProtocol.java +++ src/test/hdfs/org/apache/hadoop/hdfs/TestDataTransferProtocol.java @@ -194,7 +194,7 @@ public class TestDataTransferProtocol extends TestCase { sendBuf.reset(); recvBuf.reset(); sendOut.writeShort(( short )DataTransferProtocol.DATA_TRANSFER_VERSION); - sendOut.writeByte(DataTransferProtocol.OP_WRITE_BLOCK); + sendOut.writeByte(( byte )DataTransferProtocol.OP_WRITE_BLOCK); sendOut.writeLong(newBlockId); sendOut.writeLong(0); // generation stamp sendOut.writeInt(0); // targets in pipeline This indeed is flagged as a warning when running with the lint parameter: [javac] /Users/jhoman/work/git/hadoop/src/test/hdfs/org/apache/hadoop/hdfs/TestDataTransferProtocol.java:197: warning: [cast] redundant cast to byte [javac] sendOut.writeByte((byte)DataTransferProtocol.OP_WRITE_BLOCK); However, testpatch gives this patch a clean bill of health: [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 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 Eclipse classpath. The patch retains Eclipse classpath integrity. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. Perhaps the section of test-patch that calculates the difference in warnings got silently broken? Fixing this will hopefully prevent another round of spring cleaning next year.

            People

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

              Dates

              • Created:
                Updated:

                Development