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

          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.
          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.
          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.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          +1 patch looks good.

          Show
          Tsz Wo Nicholas Sze added a comment - +1 patch looks good.
          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!
          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
          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.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development