JDO
  1. JDO
  2. JDO-372

ConcurrentPersistenceManagersSameClasses - Failed on second run

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: JDO 2 rc1
    • Fix Version/s: JDO 2 final (2.0)
    • Component/s: tck
    • Labels:
      None

      Description

      Thanks to deleteTearDownClasses / deleteTearDownInstances, most test cases can run on either a clean database or on an existing database. Unfortunately, it seems that org.apache.jdo.tck.ConcurrentPersistenceManagersSameClasses is different. Old objects are not deleted from the 2nd database that this test case uses, so findPoint may return more than one result object, and the first result that is checked might be an old object from a previous run. Therefore, the test case passes the first run and may fail on any additional run on the same database files.

      1. JDO-372.patch
        1 kB
        Craig L Russell
      2. JDO-372.patch
        0.7 kB
        Craig L Russell
      3. JDO-372-mbo.patch
        2 kB
        Michael Bouschen

        Activity

        Hide
        Craig L Russell added a comment -

        Please review this patch. The patch deletes the three instances created in pm2.

        Show
        Craig L Russell added a comment - Please review this patch. The patch deletes the three instances created in pm2.
        Hide
        Ilan Kirsh added a comment -

        I had to delete old database files manually one time to make this patch work because the cleanup is done only on success (It is preferred to clean pm2 at the beginning of the test rather than at the end, as done with pm1...). Anyway, now that the test case passes, I can run it multiple times without problems. Thanks.

        Show
        Ilan Kirsh added a comment - I had to delete old database files manually one time to make this patch work because the cleanup is done only on success (It is preferred to clean pm2 at the beginning of the test rather than at the end, as done with pm1...). Anyway, now that the test case passes, I can run it multiple times without problems. Thanks.
        Hide
        Craig L Russell added a comment -

        Please review this patch.

        I noticed that this test will fail if the implementation does not support binary compatibility, since the classes will not be compatible with the reference implementation if enhanced without regard for binary compatibility.

        Show
        Craig L Russell added a comment - Please review this patch. I noticed that this test will fail if the implementation does not support binary compatibility, since the classes will not be compatible with the reference implementation if enhanced without regard for binary compatibility.
        Hide
        Ilan Kirsh added a comment -

        I don't understand the last comment. I am working now on ObjectDB 2.0 that unlike ObjectDB 1.0 will not support binary compatibility, but the test case passes now.
        Anyway, I remember that in some cases (maybe in this specific test), I had to use a clean database also to pass the JDOTCK 1.0 with ObjectDB 1.0 that did support binary compatibility.

        Show
        Ilan Kirsh added a comment - I don't understand the last comment. I am working now on ObjectDB 2.0 that unlike ObjectDB 1.0 will not support binary compatibility, but the test case passes now. Anyway, I remember that in some cases (maybe in this specific test), I had to use a clean database also to pass the JDOTCK 1.0 with ObjectDB 1.0 that did support binary compatibility.
        Hide
        Michael Bouschen added a comment -

        The test uses the same classes with another PersistenceManager (pm2) of the jdo refererence implementation (JPOX). It does not make sense to execute the test if the JDO implementation under test does not support binary compatibility, because then there is gurantee that the enhanced classes work with the jdo refererence implementation.

        One comment about the patch: I propose to delete the pm2 instances in the finally block. Then they get removed even if the test case fails. What do you think?

        Show
        Michael Bouschen added a comment - The test uses the same classes with another PersistenceManager (pm2) of the jdo refererence implementation (JPOX). It does not make sense to execute the test if the JDO implementation under test does not support binary compatibility, because then there is gurantee that the enhanced classes work with the jdo refererence implementation. One comment about the patch: I propose to delete the pm2 instances in the finally block. Then they get removed even if the test case fails. What do you think?
        Hide
        Ilan Kirsh added a comment -

        Thank you for the clarification. I now understand this is useless if binary compatibility is not supported. Maybe you can add a check before starting the test that binary compatibility is supported? Similar check is also needed in the org.apache.jdo.tck.enhancement tests. Deleting in finally could be good, but I think that deletePersistentAll of all the PCRect and PCPoint at the beginning of the test might be even better. Anyway, it doesn't make much different.

        Show
        Ilan Kirsh added a comment - Thank you for the clarification. I now understand this is useless if binary compatibility is not supported. Maybe you can add a check before starting the test that binary compatibility is supported? Similar check is also needed in the org.apache.jdo.tck.enhancement tests. Deleting in finally could be good, but I think that deletePersistentAll of all the PCRect and PCPoint at the beginning of the test might be even better. Anyway, it doesn't make much different.
        Hide
        Michael Bouschen added a comment -

        I extended Craig's patch and added two things:

        • Exit the test method if the binary compatibility option is not supported.
        • Remove pm2 instances in the finally block of the test method.
          Please have a look.

        Ilan,
        you are right, the enhancement tests need to check the binary compatibiliyt option, too. But since they are on the TCK exclude list, they are currently not executed.

        Show
        Michael Bouschen added a comment - I extended Craig's patch and added two things: Exit the test method if the binary compatibility option is not supported. Remove pm2 instances in the finally block of the test method. Please have a look. Ilan, you are right, the enhancement tests need to check the binary compatibiliyt option, too. But since they are on the TCK exclude list, they are currently not executed.
        Hide
        Ilan Kirsh added a comment -

        Both new patches (independently) look good. I just hope I followed all the changes exactly because I copied them manually. I am running the tests in the Eclipse IDE so I didn't notice the exclude list - where can I see it?

        Show
        Ilan Kirsh added a comment - Both new patches (independently) look good. I just hope I followed all the changes exactly because I copied them manually. I am running the tests in the Eclipse IDE so I didn't notice the exclude list - where can I see it?
        Hide
        Michael Bouschen added a comment -

        Thanks for the review.

        You find the exclude list in tck20/src/conf/exclude.list. It is automatically taken into account when running the tck from maven (e.g maven runtck.jdori or maven runtck.iut).

        Show
        Michael Bouschen added a comment - Thanks for the review. You find the exclude list in tck20/src/conf/exclude.list. It is automatically taken into account when running the tck from maven (e.g maven runtck.jdori or maven runtck.iut).
        Hide
        Craig L Russell added a comment -

        Hi Michael,

        The patch looks good.

        Show
        Craig L Russell added a comment - Hi Michael, The patch looks good.
        Hide
        Michael Bouschen added a comment -

        Checked in the patch in the 2.0 branch and the trunk (see revision 396521).

        Show
        Michael Bouschen added a comment - Checked in the patch in the 2.0 branch and the trunk (see revision 396521).

          People

          • Assignee:
            Michael Bouschen
            Reporter:
            Ilan Kirsh
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development