|
Home > Archive > Apache JDO Project > September 2005 > JDO-131 patches for review
You are viewing an archived Text-only version of the thread.
To view this thread in it's original format and/or if you want to reply to
this thread please [click here]
| Author |
JDO-131 patches for review
|
|
| karan malhi 2005-09-11, 2:45 am |
| Hi,
I am submitting the patch files for review for JIRA issue 131.
--
Karan Singh
| |
| Michael Bouschen 2005-09-11, 7:45 am |
| Hi Karan Singh,
nice work!
I would like to propose three small changes:
- All tck property names start with jdo.tck, so how about renaming
cleanup.data to jdo.tck.cleanupdata?
- I propose to specify a default (true) for the new property in
JDO_Test. We could also call equalsIgnoreCase in the variable
initialization, then it is tested only once:
protected static boolean cleanupData =
System.getProperty("jdo.tck.cleanupdata",
"true").equalsIgnoreCase("true");
- It looks like your IDE uses tabs for indentation (see the line calling
equlasIgnoreCase). Using blanks would have the advantage that it looks
the same in all the editors and IDEs.
What do you think?
Regards Michael
> Hi,
>
> I am submitting the patch files for review for JIRA issue 131.
>
>------------------------------------------------------------------------
>
>Index: C:/ApacheJDO/trunk/tck20/test/java/org/apache/jdo/tck/JDO_Test.java
> ========================================
===========================
>--- C:/ApacheJDO/trunk/tck20/test/java/org/apache/jdo/tck/JDO_Test.java (revision 279926)
>+++ C:/ApacheJDO/trunk/tck20/test/java/org/apache/jdo/tck/JDO_Test.java (working copy)
>@@ -137,6 +137,11 @@
> /** Name of the file contaninig the properties for the PMF. */
> protected static String PMFProperties = System.getProperty("PMFProperties");
>
>+ /** String indicating whether to clean up data after tests or not. The value can be either
>+ * "true" or "false". If "false" then test will not clean up data from database
>+ */
>+ protected static String cleanupData = System.getProperty("cleanup.data");
>+
> /** The Properties object for the PersistenceManagerFactory. */
> protected static Properties PMFPropertiesObject;
>
>@@ -260,7 +265,9 @@
> pmf = null;
>
> try {
>- localTearDown();
>+ if (cleanupData.equalsIgnoreCase("true")) {
>+ localTearDown();
>+ }
> }
> catch (Throwable t) {
> setTearDownThrowable("localTearDown", t);
>
>
>------------------------------------------------------------------------
>
>Index: C:/ApacheJDO/trunk/tck20/maven.xml
> ========================================
===========================
>--- C:/ApacheJDO/trunk/tck20/maven.xml (revision 279926)
>+++ C:/ApacheJDO/trunk/tck20/maven.xml (working copy)
>@@ -361,6 +361,8 @@
> value="${jdo.tck.exclude}"/>
> <sysproperty key="jdo.tck.log.directory"
> value="${jdo.tck.log.directory}/${timestamp}"/>
>+ <sysproperty key="cleanup.data"
>+ value="${cleanup.data}"/>
> <jvmarg line="${database.runtck.sysproperties}"/>
> <jvmarg line="${jdo.runtck.sysproperties}"/>
> <arg line="${jdo.tck.classes}"/>
>
>
>------------------------------------------------------------------------
>
>Index: C:/ApacheJDO/trunk/tck20/project.properties
> ========================================
===========================
>--- C:/ApacheJDO/trunk/tck20/project.properties (revision 279926)
>+++ C:/ApacheJDO/trunk/tck20/project.properties (working copy)
>@@ -42,7 +42,8 @@
> maven.junit.dir = ${jdo.tck.testdir}
> maven.junit.sysproperties = PMFProperties
> PMFProperties = jdori.properties
>-
>+# Setting this property to false will turn off cleanup of data from database to inspect database contents after test run
>+cleanup.data = true
> # JDO TCK settings
> jdo.tck.dblist=derby
> jdo.tck.identitytypes=applicationidentity datastoreidentity
>
>
--
Michael Bouschen Tech@Spree Engineering GmbH
mailto:mbo.tech@spree.de http://www.tech.spree.de/
Tel.:++49/30/235 520-33 Buelowstr. 66
Fax.:++49/30/2175 2012 D-10783 Berlin
| |
| Karan Malhi 2005-09-11, 5:45 pm |
| Hi Michael,
Thanks a lot for the review. I agree will all the changes you have proposed..
I will incorporate your suggestions and resubmit the patch.
On 9/11/05, Michael Bouschen <mbo.tech@spree.de> wrote:
>
> Hi Karan Singh,
>
> nice work!
>
> I would like to propose three small changes:
> - All tck property names start with jdo.tck, so how about renaming
> cleanup.data to jdo.tck.cleanupdata?
> - I propose to specify a default (true) for the new property in
> JDO_Test. We could also call equalsIgnoreCase in the variable
> initialization, then it is tested only once:
> protected static boolean cleanupData =
> System.getProperty("jdo.tck.cleanupdata",
> "true").equalsIgnoreCase("true");
> - It looks like your IDE uses tabs for indentation (see the line calling
> equlasIgnoreCase). Using blanks would have the advantage that it looks
> the same in all the editors and IDEs.
>
> What do you think?
>
> Regards Michael
>
> C:/ApacheJDO/trunk/tck20/test/java/org/apache/jdo/tck/JDO_Test.java
> (revision 279926)
> (working copy)
> ("PMFProperties");
> value can be either
> database
> ");
> database to inspect database contents after test run
>
>
> --
> Michael Bouschen Tech@Spree Engineering GmbH
> mailto:mbo.tech@spree.de http://www.tech.spree.de/
> Tel.:++49/30/235 520-33 Buelowstr. 66
> Fax.:++49/30/2175 2012 D-10783 Berlin
>
>
--
Karan Malhi
| |
| karan malhi 2005-09-11, 5:45 pm |
| Hi Michael,
Do you think we would need the same property for the iut tests? The
patch which i submitted earlier does not add the sysproperty for iut
tests in maven.xml.
Michael Bouschen wrote:
> Hi Karan Singh,
>
> nice work!
>
> I would like to propose three small changes:
> - All tck property names start with jdo.tck, so how about renaming
> cleanup.data to jdo.tck.cleanupdata?
> - I propose to specify a default (true) for the new property in
> JDO_Test. We could also call equalsIgnoreCase in the variable
> initialization, then it is tested only once:
> protected static boolean cleanupData =
> System.getProperty("jdo.tck.cleanupdata",
> "true").equalsIgnoreCase("true");
> - It looks like your IDE uses tabs for indentation (see the line
> calling equlasIgnoreCase). Using blanks would have the advantage that
> it looks the same in all the editors and IDEs.
>
> What do you think?
>
> Regards Michael
>
>
>
--
Karan Singh
| |
| Michael Bouschen 2005-09-11, 5:45 pm |
| Hi Karan Singh,,
> Hi Michael,
>
> Do you think we would need the same property for the iut tests? The
> patch which i submitted earlier does not add the sysproperty for iut
> tests in maven.xml.
yes, I think we need the same property for the iut tests, so the goal
doRuntck.iut should add the sysproperty, too.
Good catch!
Regards Michael
>
> Michael Bouschen wrote:
>
>
--
Michael Bouschen Tech@Spree Engineering GmbH
mailto:mbo.tech@spree.de http://www.tech.spree.de/
Tel.:++49/30/235 520-33 Buelowstr. 66
Fax.:++49/30/2175 2012 D-10783 Berlin
| |
| Craig Russell 2005-09-12, 5:45 pm |
| | |
| Michael Bouschen 2005-09-12, 5:45 pm |
| Hi Craig,
> Hi,
>
> I wonder if the property should indicate that it's cleaning up "after"
> the test, something like:
>
> jdo.tck.cleanupafter or jdo.tck.cleanupaftertest
yes, that makes sense.
In case we want to vote, I vote for jdo.tck.cleanupaftertest (although
it is a little long :-)).
Regards Michael
>
> Craig
>
> On Sep 11, 2005, at 1:03 PM, Michael Bouschen wrote:
>
>
> Craig Russell
>
> Architect, Sun Java Enterprise System http://java.sun.com/products/jdo
>
> 408 276-5638 mailto:Craig.Russell@sun.com
>
> P.S. A good JDO? O, Gasp!
>
>
--
Michael Bouschen Tech@Spree Engineering GmbH
mailto:mbo.tech@spree.de http://www.tech.spree.de/
Tel.:++49/30/235 520-33 Buelowstr. 66
Fax.:++49/30/2175 2012 D-10783 Berlin
| |
| Karan Malhi 2005-09-12, 5:45 pm |
| Hi Michael,
I agree. I would go with the longer one too, it is clearer. However, i had
one question, are we naming the properties in camel case?
should i name the property jdo.tck.cleanupAfterTest or should it be all
lower case (jdo.tck.cleanupaftertest) ? Please let me know if we are
following any particular convention in naming properties
Thanks
On 9/12/05, Craig Russell <Craig.Russell@sun.com> wrote:
>
> Hi,
> I wonder if the property should indicate that it's cleaning up "after" the
> test, something like:
>
> jdo.tck.cleanupafter or jdo.tck.cleanupaftertest
>
> Craig
>
> On Sep 11, 2005, at 1:03 PM, Michael Bouschen wrote:
>
> Hi Karan Singh,
>
>
> Hi Michael,
>
> Do you think we would need the same property for the iut tests? The patch
> which i submitted earlier does not add the sysproperty for iut tests in
> maven.xml.
>
>
> yes, I think we need the same property for the iut tests, so the goal
> doRuntck.iut should add the sysproperty, too.
>
> Good catch!
>
> Regards Michael
>
>
>
> Michael Bouschen wrote:
>
>
> Hi Karan Singh,
>
> nice work!
>
> I would like to propose three small changes:
> - All tck property names start with jdo.tck, so how about renaming
> cleanup.data to jdo.tck.cleanupdata?
> - I propose to specify a default (true) for the new property in JDO_Test.
> We could also call equalsIgnoreCase in the variable initialization, then it
> is tested only once:
> protected static boolean cleanupData =
> System.getProperty("jdo.tck.cleanupdata",
> "true").equalsIgnoreCase("true");
> - It looks like your IDE uses tabs for indentation (see the line calling
> equlasIgnoreCase). Using blanks would have the advantage that it looks the
> same in all the editors and IDEs.
>
> What do you think?
>
> Regards Michael
>
>
> Hi,
>
> I am submitting the patch files for review for JIRA issue 131.
>
> ------------------------------------------------------------------------
>
> Index: C:/ApacheJDO/trunk/tck20/test/java/org/apache/jdo/tck/JDO_Test.java
> ========================================
===========================
> --- C:/ApacheJDO/trunk/tck20/test/java/org/apache/jdo/tck/JDO_Test.java (revision
> 279926)
> +++ C:/ApacheJDO/trunk/tck20/test/java/org/apache/jdo/tck/JDO_Test.java (working
> copy)
> @@ -137,6 +137,11 @@
> /** Name of the file contaninig the properties for the PMF. */
> protected static String PMFProperties = System.getProperty
> ("PMFProperties");
>
> + /** String indicating whether to clean up data after tests or not. The
> value can be either
> + * "true" or "false". If "false" then test will not clean up data from
> database
> + */
> + protected static String cleanupData = System.getProperty("cleanup.data
> ");
> + /** The Properties object for the PersistenceManagerFactory. */
> protected static Properties PMFPropertiesObject;
>
> @@ -260,7 +265,9 @@
> pmf = null;
> try {
> - localTearDown();
> + if (cleanupData.equalsIgnoreCase("true")) {
> + localTearDown();
> + }
> } catch (Throwable t) {
> setTearDownThrowable("localTearDown", t);
>
>
> ------------------------------------------------------------------------
>
> Index: C:/ApacheJDO/trunk/tck20/maven.xml
> ========================================
===========================
> --- C:/ApacheJDO/trunk/tck20/maven.xml (revision 279926)
> +++ C:/ApacheJDO/trunk/tck20/maven.xml (working copy)
> @@ -361,6 +361,8 @@
> value="${jdo.tck.exclude}"/>
> <sysproperty key="jdo.tck.log.directory"
> value="${jdo.tck.log.directory}/${timestamp}"/>
> + <sysproperty key="cleanup.data"
> + value="${cleanup.data}"/> <jvmarg line="${database.runtck.sysproperties
> }"/>
> <jvmarg line="${jdo.runtck.sysproperties}"/>
> <arg line="${jdo.tck.classes}"/>
>
>
> ------------------------------------------------------------------------
>
> Index: C:/ApacheJDO/trunk/tck20/project.properties
> ========================================
===========================
> --- C:/ApacheJDO/trunk/tck20/project.properties (revision 279926)
> +++ C:/ApacheJDO/trunk/tck20/project.properties (working copy)
> @@ -42,7 +42,8 @@
> maven.junit.dir = ${jdo.tck.testdir}
> maven.junit.sysproperties = PMFProperties
> PMFProperties = jdori.properties
> -
> +# Setting this property to false will turn off cleanup of data from
> database to inspect database contents after test run
> +cleanup.data = true
> # JDO TCK settings
> jdo.tck.dblist=derby
> jdo.tck.identitytypes=applicationidentity datastoreidentity
>
>
>
>
>
>
>
>
>
>
> --
> Michael Bouschen Tech@Spree Engineering GmbH
> mailto:mbo.tech@spree.de <mbo.tech@spree.de> http://www.tech.spree.de/
> Tel.:++49/30/235 520-33 Buelowstr. 66
> Fax.:++49/30/2175 2012 D-10783 Berlin
>
>
>
> Craig Russell
>
> Architect, Sun Java Enterprise System http://java.sun.com/products/jdo
>
> 408 276-5638 mailto:Craig.Russell@sun.com <Craig.Russell@sun.com>
>
> P.S. A good JDO? O, Gasp!
>
>
>
--
Karan Malhi
| |
| Michael Bouschen 2005-09-12, 5:45 pm |
| Hi Karan,
I think we should use all lowercase: jdo.tck.cleanupaftertest, just
because all the other property names are lower case.
Regards Michael
>Hi Michael,
>
>I agree. I would go with the longer one too, it is clearer. However, i had
>one question, are we naming the properties in camel case?
>
>should i name the property jdo.tck.cleanupAfterTest or should it be all
>lower case (jdo.tck.cleanupaftertest) ? Please let me know if we are
>following any particular convention in naming properties
>
>Thanks
>
>On 9/12/05, Craig Russell <Craig.Russell@sun.com> wrote:
>
>
>
>
>
>
--
Michael Bouschen Tech@Spree Engineering GmbH
mailto:mbo.tech@spree.de http://www.tech.spree.de/
Tel.:++49/30/235 520-33 Buelowstr. 66
Fax.:++49/30/2175 2012 D-10783 Berlin
| |
| Craig Russell 2005-09-12, 8:45 pm |
| | |
| Karan Malhi 2005-09-12, 8:45 pm |
| Hi ,
Should i also add this property to the help goal ? I guess, if
jdo.tck.cfglist and jdo.tck.identitytypes are specified in help, then we can
also specify this property in the help goal whereby we can give a short
description on this property
On 9/12/05, Craig Russell <Craig.Russell@sun.com> wrote:
>
> Hi Karan,
> You can see all of the property names in the maven file you have as part
> of this patch. They are all lower case.
>
> Craig
>
>
> On Sep 12, 2005, at 3:36 PM, Michael Bouschen wrote:
>
> Hi Karan,
>
> I think we should use all lowercase: jdo.tck.cleanupaftertest, just
> because all the other property names are lower case.
>
> Regards Michael
>
>
> Hi Michael,
>
> I agree. I would go with the longer one too, it is clearer. However, i had
> one question, are we naming the properties in camel case?
>
> should i name the property jdo.tck.cleanupAfterTest or should it be all
> lower case (jdo.tck.cleanupaftertest) ? Please let me know if we are
> following any particular convention in naming properties
>
> Thanks
>
> On 9/12/05, Craig Russell <Craig.Russell@sun.com> wrote:
>
>
>
> Hi,
> I wonder if the property should indicate that it's cleaning up "after" the
> test, something like:
>
> jdo.tck.cleanupafter or jdo.tck.cleanupaftertest
>
> Craig
>
> On Sep 11, 2005, at 1:03 PM, Michael Bouschen wrote:
>
> Hi Karan Singh,
>
>
> Hi Michael,
>
> Do you think we would need the same property for the iut tests? The patch
> which i submitted earlier does not add the sysproperty for iut tests in
> maven.xml.
>
>
> yes, I think we need the same property for the iut tests, so the goal
> doRuntck.iut should add the sysproperty, too.
>
> Good catch!
>
> Regards Michael
>
>
>
> Michael Bouschen wrote:
>
>
> Hi Karan Singh,
>
> nice work!
>
> I would like to propose three small changes:
> - All tck property names start with jdo.tck, so how about renaming
> cleanup.data to jdo.tck.cleanupdata?
> - I propose to specify a default (true) for the new property in JDO_Test.
> We could also call equalsIgnoreCase in the variable initialization, then it
> is tested only once:
> protected static boolean cleanupData =
> System.getProperty("jdo.tck.cleanupdata",
> "true").equalsIgnoreCase("true");
> - It looks like your IDE uses tabs for indentation (see the line calling
> equlasIgnoreCase). Using blanks would have the advantage that it looks the
> same in all the editors and IDEs.
>
> What do you think?
>
> Regards Michael
>
>
> Hi,
>
> I am submitting the patch files for review for JIRA issue 131.
>
> ------------------------------------------------------------------------
> Index: C:/ApacheJDO/trunk/tck20/test/java/org/apache/jdo/tck/JDO_Test.java
> ========================================
===========================
> --- C:/ApacheJDO/trunk/tck20/test/java/org/apache/jdo/tck/JDO_Test.java
> (revision 279926)
> +++ C:/ApacheJDO/trunk/tck20/test/java/org/apache/jdo/tck/JDO_Test.java
> (working copy)
> @@ -137,6 +137,11 @@
> /** Name of the file contaninig the properties for the PMF. */
> protected static String PMFProperties = System.getProperty
> ("PMFProperties");
>
> + /** String indicating whether to clean up data after tests or not. The
> value can be either
> + * "true" or "false". If "false" then test will not clean up data from
> database
> + */
> + protected static String cleanupData = System.getProperty("cleanup.data
> ");
> + /** The Properties object for the PersistenceManagerFactory. */
> protected static Properties PMFPropertiesObject;
>
> @@ -260,7 +265,9 @@
> pmf = null;
> try {
> - localTearDown();
> + if (cleanupData.equalsIgnoreCase("true")) {
> + localTearDown();
> + }
> } catch (Throwable t) {
> setTearDownThrowable("localTearDown", t);
>
>
> ------------------------------------------------------------------------
> Index: C:/ApacheJDO/trunk/tck20/maven.xml
> ========================================
===========================
> --- C:/ApacheJDO/trunk/tck20/maven.xml (revision 279926)
> +++ C:/ApacheJDO/trunk/tck20/maven.xml (working copy)
> @@ -361,6 +361,8 @@
> value="${jdo.tck.exclude}"/>
> <sysproperty key="jdo.tck.log.directory"
> value="${jdo.tck.log.directory}/${timestamp}"/>
> + <sysproperty key="cleanup.data"
> + value="${cleanup.data}"/> <jvmarg line="${database.runtck.sysproperties
> }"/>
> <jvmarg line="${jdo.runtck.sysproperties}"/>
> <arg line="${jdo.tck.classes}"/>
>
>
> ------------------------------------------------------------------------
> Index: C:/ApacheJDO/trunk/tck20/project.properties
> ========================================
===========================
> --- C:/ApacheJDO/trunk/tck20/project.properties (revision 279926)
> +++ C:/ApacheJDO/trunk/tck20/project.properties (working copy)
> @@ -42,7 +42,8 @@
> maven.junit.dir = ${jdo.tck.testdir}
> maven.junit.sysproperties = PMFProperties
> PMFProperties = jdori.properties
> -
> +# Setting this property to false will turn off cleanup of data from
> database to inspect database contents after test run
> +cleanup.data = true
> # JDO TCK settings
> jdo.tck.dblist=derby
> jdo.tck.identitytypes=applicationidentity datastoreidentity
>
>
>
>
>
>
>
>
>
>
> --
> Michael Bouschen Tech@Spree Engineering GmbH
> mailto:mbo.tech@spree.de <mbo.tech@spree.de> <mbo.tech@spree.de>
> http://www.tech.spree.de/
> Tel.:++49/30/235 520-33 Buelowstr. 66 Fax.:++49/30/2175 2012 D-10783
> Berlin
>
>
> Craig Russell
>
> Architect, Sun Java Enterprise System http://java.sun.com/products/jdo
>
> 408 276-5638 mailto:Craig.Russell@sun.com <Craig.Russell@sun.com> <
> Craig.Russell@sun.com>
>
> P.S. A good JDO? O, Gasp!
>
>
>
>
>
>
>
>
>
>
>
> --
> Michael Bouschen Tech@Spree Engineering GmbH
> mailto:mbo.tech@spree.de <mbo.tech@spree.de> http://www.tech.spree.de/
> Tel.:++49/30/235 520-33 Buelowstr. 66
> Fax.:++49/30/2175 2012 D-10783 Berlin
>
>
>
> Craig Russell
>
> Architect, Sun Java Enterprise System http://java.sun.com/products/jdo
>
> 408 276-5638 mailto:Craig.Russell@sun.com <Craig.Russell@sun.com>
>
> P.S. A good JDO? O, Gasp!
>
>
>
--
Karan Malhi
| |
| Michelle Caisse 2005-09-12, 8:45 pm |
| Yes, this is definitely a good idea!
-- Michelle
Karan Malhi wrote:
>Hi ,
>
>Should i also add this property to the help goal ? I guess, if
>jdo.tck.cfglist and jdo.tck.identitytypes are specified in help, then we can
>also specify this property in the help goal whereby we can give a short
>description on this property
>
>On 9/12/05, Craig Russell <Craig.Russell@sun.com> wrote:
>
>
>
>
>
>
| |
| Craig Russell 2005-09-12, 8:45 pm |
| |
|
|
|
|