Details

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

      Description

      What do people think of making the tests run with java asserts enabled by default?

        Issue Links

          Activity

          Eli Collins created issue -
          Hide
          Nigel Daley added a comment -

          +1 on the idea.

          Show
          Nigel Daley added a comment - +1 on the idea.
          Hide
          Konstantin Shvachko added a comment -

          This would be very useful. I found several bugs running tests with asserts. Also in many cases asserts help tests fail early.

          Show
          Konstantin Shvachko added a comment - This would be very useful. I found several bugs running tests with asserts. Also in many cases asserts help tests fail early.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          > What do people think of making the tests run with java asserts enabled by default?
          +1

          Show
          Tsz Wo Nicholas Sze added a comment - > What do people think of making the tests run with java asserts enabled by default? +1
          Hide
          Todd Lipcon added a comment -

          Big +1. We should also open a ticket to add asserts in various places where we've shied away from error checking due to performance concerns.

          Note that we'll need to fix an assertion failure currently triggered by TestAccessTokenWithDFS before committing the -ea addition.

          Show
          Todd Lipcon added a comment - Big +1. We should also open a ticket to add asserts in various places where we've shied away from error checking due to performance concerns. Note that we'll need to fix an assertion failure currently triggered by TestAccessTokenWithDFS before committing the -ea addition.
          Eli Collins made changes -
          Field Original Value New Value
          Assignee Eli Collins [ eli ]
          Hide
          Eli Collins added a comment -

          Attached a patch for enabling java asserts by default for tests in common. Ran ant test runs w/o assertion failure on trunk@823756. Verified this same change on the hdfs tree running TestAccessTokenWithDFS causes the assert to fire. Will upload patches for hdfs and mapreduce. Lemme know if these need separate jiras, otherwise will post them here. Will also file jiras for any asserts that fail.

          Show
          Eli Collins added a comment - Attached a patch for enabling java asserts by default for tests in common. Ran ant test runs w/o assertion failure on trunk@823756. Verified this same change on the hdfs tree running TestAccessTokenWithDFS causes the assert to fire. Will upload patches for hdfs and mapreduce. Lemme know if these need separate jiras, otherwise will post them here. Will also file jiras for any asserts that fail.
          Eli Collins made changes -
          Attachment hadoop6309-common.patch [ 12421901 ]
          Hide
          Konstantin Boudnik added a comment -

          Yes, they better have separate JIRAs (being separate projects). And link them all together.

          Show
          Konstantin Boudnik added a comment - Yes, they better have separate JIRAs (being separate projects). And link them all together.
          Hide
          Eli Collins added a comment -

          Thanks Konstantin, filed HDFS-697 and MAPREDUCE-1092.

          Tested the patch attached to this jira by adding an assert false; to Configuration.java and made sure TestConfiguration failed as expected.

          Show
          Eli Collins added a comment - Thanks Konstantin, filed HDFS-697 and MAPREDUCE-1092 . Tested the patch attached to this jira by adding an assert false; to Configuration.java and made sure TestConfiguration failed as expected.
          Hide
          Eli Collins added a comment -

          Tests run w/o assertion failure on trunk.

          Show
          Eli Collins added a comment - Tests run w/o assertion failure on trunk.
          Eli Collins made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Tsz Wo Nicholas Sze added a comment -

          +1 patch looks good.

          Show
          Tsz Wo Nicholas Sze added a comment - +1 patch looks good.
          Tsz Wo Nicholas Sze made changes -
          Hadoop Flags [Reviewed]
          Component/s build [ 12311543 ]
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12421901/hadoop6309-common.patch
          against trunk revision 823756.

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

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +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/Hadoop-Patch-h4.grid.sp2.yahoo.net/83/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/83/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/83/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/83/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/12421901/hadoop6309-common.patch against trunk revision 823756. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +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/Hadoop-Patch-h4.grid.sp2.yahoo.net/83/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/83/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/83/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/83/console This message is automatically generated.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          How about we commit this to 0.21 and above? Any thought?

          Show
          Tsz Wo Nicholas Sze added a comment - How about we commit this to 0.21 and above? Any thought?
          Hide
          Konstantin Boudnik added a comment -

          You mean get it into the trunk and backport to 0.21? +1

          Show
          Konstantin Boudnik added a comment - You mean get it into the trunk and backport to 0.21? +1
          Hide
          Konstantin Boudnik added a comment -

          Also, for accounting purposes it'd by good to have a comment about -1 in the Hudson's report. It is clear why there's no additional tests in this patch, however later on it will require an extra effort to find out why the patch has been committed with negative rating.

          Show
          Konstantin Boudnik added a comment - Also, for accounting purposes it'd by good to have a comment about -1 in the Hudson's report. It is clear why there's no additional tests in this patch, however later on it will require an extra effort to find out why the patch has been committed with negative rating.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          > You mean get it into the trunk and backport to 0.21? +1

          Exactly.

          I will wait until tomorrow before committing this . See if anyone has comments.

          Show
          Tsz Wo Nicholas Sze added a comment - > You mean get it into the trunk and backport to 0.21? +1 Exactly. I will wait until tomorrow before committing this . See if anyone has comments.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          No new tests needed because the patch only changes build.xml.

          I have committed this to 0.21 and above. Thanks, Eli!

          Show
          Tsz Wo Nicholas Sze added a comment - No new tests needed because the patch only changes build.xml. I have committed this to 0.21 and above. Thanks, Eli!
          Tsz Wo Nicholas Sze made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Fix Version/s 0.21.0 [ 12313563 ]
          Fix Version/s 0.22.0 [ 12314296 ]
          Resolution Fixed [ 1 ]
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #60 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk-Commit/60/)
          . Change build.xml to run tests with java asserts. Contributed by Eli Collins

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #60 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk-Commit/60/ ) . Change build.xml to run tests with java asserts. Contributed by Eli Collins
          Tom White made changes -
          Fix Version/s 0.22.0 [ 12314296 ]
          Hide
          Eli Collins added a comment -

          Per HDFS-697 and MAPREDUCE-1092 should we just enable the asserts as part of the unit tests for now?

          Show
          Eli Collins added a comment - Per HDFS-697 and MAPREDUCE-1092 should we just enable the asserts as part of the unit tests for now?
          Hide
          Eli Collins added a comment -

          Actually that's what this change did, sorry for the noise.

          Show
          Eli Collins added a comment - Actually that's what this change did, sorry for the noise.
          Eli Collins made changes -
          Link This issue is related to HDFS-697 [ HDFS-697 ]
          Eli Collins made changes -
          Link This issue is related to MAPREDUCE-1092 [ MAPREDUCE-1092 ]
          Tom White made changes -
          Status Resolved [ 5 ] Closed [ 6 ]

            People

            • Assignee:
              Eli Collins
              Reporter:
              Eli Collins
            • Votes:
              0 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development