Test for behaviour of null passed to detachCopy(), makePersistent(), etc

• Key: JDO-432
• Type: Task
• Status: Resolved
• Resolution: Fixed
• Priority: Minor
• Assignee: Michelle Caisse
• Reporter: Andy Jefferson
• Component/s: tck2
• Affects Version/s: JDO 2 final
• Fix Version/s: JDO 2 maintenance release 1

Description

The JDO2 spec adequately defines (section 12.6.0 "Null Management") behaviour when null values are passed in to the PersistenceManager methods makePersistent, detachCopy, etc but there doesnt seem to be a test for these situations currently. JPOX 1.1.2 on calling detachCopy(null) actually throws an NPE, which is outside of spec ... now fixed.

Comments

[Michelle Caisse]

The attached patch supplies code for the first of a set of tests. This one tests makePersistent() and makePersistentAll(). It creates a new package org.apache.jdo.tck.api.persistencemanager.nullargs, a new superclass for null argument tests PersistenceManagerNullsTest.java and a new test class MakePersistentNullArgs.java. Please review and comment.

The RI fails on three of the five test methods by throwing an NPE rather than doing nothing with a null pc instance or a null element of an array or Collection of pc instances.

[Michelle Caisse]

This patch is complete for this issue. It contains a new superclass and a test class for each PersistanceManager API family under test, in a new package. I updated pm.conf to invoke these tests. Please review.

There are three failures in the 50 new test cases:

    [java] 1) testDetachCopyCollectionNullElement(org.apache.jdo.tck.api.persistencemanager.nullargs.DetachCopyNullArgs)junit.framework.AssertionFailedError: Assertion A12.6-5 failed:
    [java] detachCopyAll(Collection) on a null Collection element should do nothing. Instead we get: org.jpox.exceptions.ClassNotDetachableException: The class "org.apache.jdo.tck.pc.mylib.PCPoint" is
 not Detachable. This means that the MetaData for the class did not have the "detachable" attribute set to true.
    [java] at org.apache.jdo.tck.JDO_Test.fail(JDO_Test.java:638)
...
    [java] 2) testDetachCopyArrayNullElement(org.apache.jdo.tck.api.persistencemanager.nullargs.DetachCopyNullArgs)junit.framework.AssertionFailedError: Assertion A12.6-5 failed:
    [java] detachCopyAll(Array) on a null array element should do nothing. Instead we get: org.jpox.exceptions.ClassNotDetachableException: The class "org.apache.jdo.tck.pc.mylib.PCPoint" is not Detac
hable. This means that the MetaData for the class did not have the "detachable" attribute set to true.
...
    [java] 3) testMakeNontransactionalNullObject(org.apache.jdo.tck.api.persistencemanager.nullargs.MakeNontransactionalNullArgs)junit.framework.AssertionFailedError: Assertion A12.6-3 failed:
    [java] makeNontransactional(null)on a null object should do nothing. Instead we get: java.lang.NullPointerException
    [java] at org.apache.jdo.tck.JDO_Test.fail(JDO_Test.java:638)
    [java] at org.apache.jdo.tck.api.persistencemanager.nullargs.PersistenceManagerNullsTest.executeNullObjectParameter(PersistenceManagerNullsTest.java:222)
    [java] at org.apache.jdo.tck.api.persistencemanager.nullargs.MakeNontransactionalNullArgs.testMakeNontransactionalNullObject(MakeNontransactionalNullArgs.java:69)
    [java] at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    [java] at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
    [java] at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
    [java] at org.apache.jdo.tck.JDO_Test.runBare(JDO_Test.java:253)
    [java] at org.apache.jdo.tck.util.BatchTestRunner.doRun(BatchTestRunner.java:107)
    [java] at org.apache.jdo.tck.util.BatchTestRunner.start(BatchTestRunner.java:147)
    [java] at org.apache.jdo.tck.util.BatchTestRunner.main(BatchTestRunner.java:122)

[Craig Russell]

Looks good; just a few comments.

1. The Assertion Description section of javadoc exceeds 80 characters in most of the tests. Perhaps the description could be abstracted to the superclass and reduced to 80 character lines, and then the description in the subclass would reference the superclass and name the method, e.g. "Test null arguments for makePersistent."

2. Javadoc for checkReturn methods (both Collection and Object[ ]) is incorrect. It only checks that corresponding parameters are either null or not null.

+ /**
+ * Checks if expected and actual arguments are equal
+ * @return true if arguments are equal
+ * @param expected Collection
+ * @param actual Collection
+ */
+ protected boolean checkReturn(Collection expected, Collection actual) {
...
+ if ( (eElem == null && aElem != null)
+ || (aElem == null && eElem != null) )
+ return false;

3. The method makeTestInstancesPersistent should only make the instance persistent, and not use makePersistentAll methods, since these methods are supposed to be tested with these test cases.

4. Consider putting getPM() into the makeTestInstancesPersistent method instead of in localSetUp. Consider getting rid of makeTestInstancesPersistent entirely and putting all the logic into localSetUp, its only use.

[Michelle Caisse]

Re preceding comments:

1. The Assertion Description in all the TCK tests is an exact copy of the assertion text and generally exceeds 80 characters.

2., 3., 4. Done.

Thanks!

[Michelle Caisse]

Completed: At revision: 465134

[Andy Jefferson]

The 2 remaining issues are due to JPOX implementing part of an earlier version of the JDO2 spec. Some time ago it used to say that calling detachCopyXXX() on an object that was of a non-Detachable class would throw JDOUserException. JDO2-final says that it should return a transient and previously there was seemingly no TCK test for that condition until this test for nulls that actually also tests this too.

I'll leave it to you to decide if you want to just pass in a single "null" element so the test just tests the null part of the spec, and add a new test for the return of a transient when passing in a non-Detachable object.

Anyway, its monitorable via http://www.jpox.org/servlet/jira/browse/CORE-3037