org.apache.jdo.tck.api.persistencemanagerfactory.GetPersistenceManager.test() and org.apache.jdo.tck.api.persistencemanagerfactory.GetPersistenceManagerForUser.test() don't close PMF correctly

• Key: JDO-394
• Type: Bug
• Status: Resolved
• Resolution: Fixed
• Priority: Minor
• Assignee: Michael Bouschen
• Reporter: Christian Ernst
• Component/s: tck2
• Affects Version/s: JDO 2 final
• Fix Version/s: JDO 2 maintenance release 1

Description

org.apache.jdo.tck.api.persistencemanagerfactory.GetPersistenceManager.test() and
org.apache.jdo.tck.api.persistencemanagerfactory.GetPersistenceManagerForUser.test()
don't close PMF correctly and this can cause other Testcases to fail

Following should be added to the finally block of each test() Method

if (pmf != null) pmf.close();

Comments

[Craig Russell]

Need to resolve these issues for maintenance release 1.

[Michael Bouschen]

Attached you find a patch (JDO-394.patch) adding pmf.close to the finally block and does some more code cleanup.

I found a similar issue with the following test classes from the same package: SetIgnoreCache, SetMultithreaded, SetNonTransactionalRead, SetNonTransactionalWrite, and SetRetainValues. The patch adds a finally block closing the pmf.

[Michael Bouschen]

Attached you find a new patch (JDO-394-2.patch) for review.

The new patch fixes a problem I overlooked before: the old code creates a new PMF as part of the test method, but the setUp method
already created a PMF. This patch fixes this problem. It also makes sure that an existing PMF is closed, that might have been left open from a previous test. The test cases testing PMF setter method cannot use the getPMF method as implemented in the test super class JDO_Test, because this returns a non-configurable PMF.

[Craig Russell]

Looks great. It's good to clean up these tests. Just a few comments.

1. The pattern:

             if (pmf.getOptimistic() != true) {
- fail(ASSERTION_FAILED,
- "Optimistic set to true, value returned by PMF is " +
- pmf.getOptimistic());

hides the value that was returned by the pmf, and gets a new value for the error message. I'd prefer something like this:

boolean optimistic = pmf.getOptimistic();
             if (optimistic != true) {
- fail(ASSERTION_FAILED,
- "Optimistic set to true, value returned by PMF is " +
- optimistic;

2. In src/java/org/apache/jdo/tck/api/persistencemanagerfactory/GetPersistenceManagerFactoryByPropertiesInstance.java you use the JDO_Test pmf instance but define your own pm and tx instances, which requires you to use your own finally block to close the pm. Couldn't you use the JDO_Test pm instance as well? Then the normal tearDown will close the pm.

3. This is really a nit, but I'd prefer the method getConfigurablePMF to be named getUnconfiguredPMF. From the name, I was expecting perhaps a PMF that was configured with all of the properties but was still configurable. Instead, it's simply an instance of PMF that had not been configured at all.

4. In src/java/org/apache/jdo/tck/api/persistencemanagerfactory/GetPersistenceManagerForUser.java:
        username = props.getProperty(CONNECTION_USERNAME_PROP);
        password = props.getProperty(CONNECTION_PASSWORD_PROP);
        props.remove(CONNECTION_USERNAME_PROP);
        props.remove(CONNECTION_PASSWORD_PROP);

This could be simplified, since remove returns the present value in the Map:
        username = props.remove(CONNECTION_USERNAME_PROP);
        password = props.remove(CONNECTION_PASSWORD_PROP);

Craig

[Michael Bouschen]

I changed the patch according to the comments and checked it in (see revision 429480).