Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: 2.0.0-M2
    • Fix Version/s: 2.0.0-M2
    • Component/s: None
    • Labels:
      None

      Description

      In openjpa-persistence-jdbc/pom.xml there are a lot of excluded testcases. Instead of hard coding these in pom.xml we should use the new annotation that Pinaki introduced in the "parent" JIRA issue.

      The list of excluded tests can be found in pom.xml, here's a subsection of them

      <!-- exclude new tests that aren't passing yet -->
      <exclude>org/apache/openjpa/persistence/jpql/clauses/TestEJBQueryInterface.java</exclude>
      <exclude>org/apache/openjpa/persistence/kernel/TestInitialValueFetching.java</exclude>
      <exclude>org/apache/openjpa/persistence/kernel/TestOpenResultsCommit.java</exclude>
      <exclude>org/apache/openjpa/persistence/query/TestQuotedNumbersInFilters2.java</exclude>

      1. OPENJPA-770.patch
        210 kB
        Tim McConnell
      2. OPENJPA-770-2.patch
        138 kB
        Tim McConnell
      3. OPENJPA-770-3.patch
        139 kB
        Tim McConnell

        Activity

        Hide
        Tim McConnell added a comment -

        Hi, I've attached a patch for this JIRA for review, comments, and hopefully inclusion. Please consider these points when reviewing the patch:

        1. The @AllowFailure annotation was added (almost exclusively at the method level) to the testcases that were previously excluded in the pom.xml.

        2. Some of the testcases that were previously excluded in the pom.xml are now working, in which case I did not alter them

        3. Some testcases were repaired for other reasons. Most notably, testcases were repaired so as not to fail the build (e.g., NullPointerExceptions were fixed, and testcases with no tests were repaired).

        4. Finally, I re-ordered all the imports in the classes that I touched, but I decided not to include those changes in the patch so as not to confuse the reviewer. I can easily attach that patch though if it would not be frowned upon.

        Show
        Tim McConnell added a comment - Hi, I've attached a patch for this JIRA for review, comments, and hopefully inclusion. Please consider these points when reviewing the patch: 1. The @AllowFailure annotation was added (almost exclusively at the method level) to the testcases that were previously excluded in the pom.xml. 2. Some of the testcases that were previously excluded in the pom.xml are now working, in which case I did not alter them 3. Some testcases were repaired for other reasons. Most notably, testcases were repaired so as not to fail the build (e.g., NullPointerExceptions were fixed, and testcases with no tests were repaired). 4. Finally, I re-ordered all the imports in the classes that I touched, but I decided not to include those changes in the patch so as not to confuse the reviewer. I can easily attach that patch though if it would not be frowned upon.
        Hide
        Pinaki Poddar added a comment -

        A small utility is available to find all tests that are marked with @AllowFailure. It is general purpose to detect other kinds of things but its main() is hardcoded for its original purpose.
        The following small script should show how this works:

        1. Go to root of OpenJPA directory
        2. Execute the following
        @setlocal
        set SERP_JAR=c:\Documents and Settings\Administrator\.m2\repository\net\sourceforge\serp\serp\1.13.1\serp-1.13.1.jar
        SET CLASSPATH=openjpa-persistence-jdbc\target\test-classes;%SERP_JAR%
        java org.apache.openjpa.persistence.test.ClassSelector
        @endlocal

        3. It prints something like this on console:

        Found 3 classes under C:\work\openjpa-1.3.0 that
        extends org.apache.openjpa.persistence.test.SingleEMTestCase
        or org.apache.openjpa.persistence.test.SingleEMFTestCase
        or org.apache.openjpa.persistence.kernel.BaseKernelTest
        or org.apache.openjpa.persistence.query.BaseQueryTest
        or org.apache.openjpa.persistence.jdbc.kernel.BaseJDBCTest
        or org.apache.openjpa.persistence.common.utils.AbstractTestCase
        and annotatated with org.apache.openjpa.persistence.test.AllowFailure

        org.apache.openjpa.persistence.jdbc.annotations.TestSequenceGenerator
        org.apache.openjpa.persistence.jdbc.query.TestQueryParameterBinding
        org.apache.openjpa.persistence.kernel.TestInitialValueFetching

        Show
        Pinaki Poddar added a comment - A small utility is available to find all tests that are marked with @AllowFailure. It is general purpose to detect other kinds of things but its main() is hardcoded for its original purpose. The following small script should show how this works: 1. Go to root of OpenJPA directory 2. Execute the following @setlocal set SERP_JAR=c:\Documents and Settings\Administrator\.m2\repository\net\sourceforge\serp\serp\1.13.1\serp-1.13.1.jar SET CLASSPATH=openjpa-persistence-jdbc\target\test-classes;%SERP_JAR% java org.apache.openjpa.persistence.test.ClassSelector @endlocal 3. It prints something like this on console: Found 3 classes under C:\work\openjpa-1.3.0 that extends org.apache.openjpa.persistence.test.SingleEMTestCase or org.apache.openjpa.persistence.test.SingleEMFTestCase or org.apache.openjpa.persistence.kernel.BaseKernelTest or org.apache.openjpa.persistence.query.BaseQueryTest or org.apache.openjpa.persistence.jdbc.kernel.BaseJDBCTest or org.apache.openjpa.persistence.common.utils.AbstractTestCase and annotatated with org.apache.openjpa.persistence.test.AllowFailure org.apache.openjpa.persistence.jdbc.annotations.TestSequenceGenerator org.apache.openjpa.persistence.jdbc.query.TestQueryParameterBinding org.apache.openjpa.persistence.kernel.TestInitialValueFetching
        Hide
        Albert Lee added a comment -

        Tim, I took a look at the patch, It does what is asked for in the original request.

        To keep the changes cleaner and to a minimum, I wonder a class level @AllowFailure can be applied to some of these tests instead of annotating in every method. Also consider the use of @AllowFailure(value=false) to negate test selection and/or exclusions.

        Thanks for the patch.
        Albert Lee.

        Show
        Albert Lee added a comment - Tim, I took a look at the patch, It does what is asked for in the original request. To keep the changes cleaner and to a minimum, I wonder a class level @AllowFailure can be applied to some of these tests instead of annotating in every method. Also consider the use of @AllowFailure(value=false) to negate test selection and/or exclusions. Thanks for the patch. Albert Lee.
        Hide
        Tim McConnell added a comment -

        Hi Albert, thanks much for reviewing. I've attached another patch which includes your recommendations.

        Show
        Tim McConnell added a comment - Hi Albert, thanks much for reviewing. I've attached another patch which includes your recommendations.
        Hide
        Tim McConnell added a comment -

        Patch #3 attached to fix problem no tests found in TestAbstractMappedAppIdSuper

        Show
        Tim McConnell added a comment - Patch #3 attached to fix problem no tests found in TestAbstractMappedAppIdSuper
        Hide
        Albert Lee added a comment -

        Tim, Thanks for providing the new patch.
        ---------------------
        Other developers,

        The new patch is ready but I have some hesitation in committing this changes. The reasons are:

        1) The # of test grows from 1300s to 2200s tests and 149 more test classes. (Not a good reason for not committing)
        2) The concern is the run duration for the mvn "test" target has grown approximately from 35 minutes to over 80 minutes on my Thinkpad.
        3) If the new tests provide value in validating openjpa functions, that is great but these tests are just ran, mostly failed but ignored, then I am not too thrill in committing this change.

        Please reply and vote if you think it is worth to make the changes:
        +1 - commit
        0 - neutral
        -1 - reject

        Thanks,
        Albert Lee.

        Show
        Albert Lee added a comment - Tim, Thanks for providing the new patch. --------------------- Other developers, The new patch is ready but I have some hesitation in committing this changes. The reasons are: 1) The # of test grows from 1300s to 2200s tests and 149 more test classes. (Not a good reason for not committing) 2) The concern is the run duration for the mvn "test" target has grown approximately from 35 minutes to over 80 minutes on my Thinkpad. 3) If the new tests provide value in validating openjpa functions, that is great but these tests are just ran, mostly failed but ignored, then I am not too thrill in committing this change. Please reply and vote if you think it is worth to make the changes: +1 - commit 0 - neutral -1 - reject Thanks, Albert Lee.
        Hide
        Michael Dick added a comment -

        I didn't realize the test exclusions would add that much time. I vote for leaving well enough alone - spending an additional 40 -50 minutes per build with no tangible benefit doesn't sound good to me.

        I appreciate the patch, and I would like to get rid of the long list of exclusions, but not at that price.

        Show
        Michael Dick added a comment - I didn't realize the test exclusions would add that much time. I vote for leaving well enough alone - spending an additional 40 -50 minutes per build with no tangible benefit doesn't sound good to me. I appreciate the patch, and I would like to get rid of the long list of exclusions, but not at that price.
        Hide
        Donald Woods added a comment -

        So why do we have so many junit tests that we aren't using? Maybe its time to delete them? What good are they providing if they are never executed?

        Show
        Donald Woods added a comment - So why do we have so many junit tests that we aren't using? Maybe its time to delete them? What good are they providing if they are never executed?
        Hide
        Albert Lee added a comment -

        3 votes (Mike D, Jeremy B and Albert L) NOT to commit the patch due to test run duration cost if higher than acceptable.

        Show
        Albert Lee added a comment - 3 votes (Mike D, Jeremy B and Albert L) NOT to commit the patch due to test run duration cost if higher than acceptable.

          People

          • Assignee:
            Tim McConnell
            Reporter:
            Michael Dick
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 3h
              3h
              Remaining:
              Remaining Estimate - 3h
              3h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development