Derby
  1. Derby
  2. DERBY-2458

Convert lang/unaryArithmeticDynamicParamter.java to junit

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 10.3.1.4
    • Fix Version/s: 10.3.1.4
    • Component/s: Test
    • Labels:
      None

      Description

      Place holder to convert lang/unaryArithmeticDynamicParameter.java to junit

      1. DERBY-2458_stat_ver1.txt
        0.2 kB
        Manjula Kutty
      2. DERBY-2458_stat_03_21.txt
        0.5 kB
        Manjula Kutty
      3. DERBY-2458_stat_03_19.txt
        0.3 kB
        Manjula Kutty
      4. DERBY-2458_diff_ver1.txt
        17 kB
        Manjula Kutty
      5. DERBY-2458_diff_03_21.txt
        0.6 kB
        Manjula Kutty
      6. DERBY-2458_diff_03_20.txt
        18 kB
        Manjula Kutty
      7. DERBY-2458_diff_03_20_ver2.txt
        17 kB
        Manjula Kutty
      8. DERBY-2458_diff_03_19.txt
        18 kB
        Manjula Kutty
      9. DERBY-2458_diff_03_16.txt
        0.9 kB
        Manjula Kutty
      10. DERBY-2458_diff_03_16_ver2.txt
        15 kB
        Manjula Kutty
      11. DERBY-2458__stat_03_20.txt
        0.2 kB
        Manjula Kutty

        Activity

        Hide
        Manjula Kutty added a comment -

        Attaching the first patch for this conversion

        Show
        Manjula Kutty added a comment - Attaching the first patch for this conversion
        Hide
        Manjula Kutty added a comment -

        Forgot to add the changes in the _Suite.java file. Please find the same and if everything looks fine please commit

        Show
        Manjula Kutty added a comment - Forgot to add the changes in the _Suite.java file. Please find the same and if everything looks fine please commit
        Hide
        Jean T. Anderson added a comment -

        I took a look at your patch and have several comments.

        I like your addition of a assertColumnTypes method to JDBC.java. And I really like your use of assertions. Your usage of JDBC.assertFullResultSet, assertCompileError, and other methods is a good reminder to me to check where I should be using more of what others have already defined.

        Here are a couple things I think could be changed:

        • setUp() needs a stmt.close()
        • Two test methods create a SQL function, but don't drop it. I didn't encounter problems when I ran your suite (both client and embedded), but I wonder if that might create a problem in some context.
        • Take a pass through the code and verify that the javadoc comments match the code; I know how easy it is for them to get out of sync while developing.
          For example, the javadoc comment for setUp() doesn't match what it does. You might consider simplifying the description to something like "Create and populate test tables.". That would reduce maintenance later if more tables get added, their names change, or more rows get inserted.
        Show
        Jean T. Anderson added a comment - I took a look at your patch and have several comments. I like your addition of a assertColumnTypes method to JDBC.java. And I really like your use of assertions. Your usage of JDBC.assertFullResultSet, assertCompileError, and other methods is a good reminder to me to check where I should be using more of what others have already defined. Here are a couple things I think could be changed: setUp() needs a stmt.close() Two test methods create a SQL function, but don't drop it. I didn't encounter problems when I ran your suite (both client and embedded), but I wonder if that might create a problem in some context. Take a pass through the code and verify that the javadoc comments match the code; I know how easy it is for them to get out of sync while developing. For example, the javadoc comment for setUp() doesn't match what it does. You might consider simplifying the description to something like "Create and populate test tables.". That would reduce maintenance later if more tables get added, their names change, or more rows get inserted.
        Hide
        Manjula Kutty added a comment -

        Thanks for the comments, Jean. I'm attaching the new patch which will take care of the closing issue and drop functions. Please review and if looks fine please commit

        Show
        Manjula Kutty added a comment - Thanks for the comments, Jean. I'm attaching the new patch which will take care of the closing issue and drop functions. Please review and if looks fine please commit
        Hide
        Daniel John Debrunner added a comment -

        Patch doesn't compile for me,
        [javac] C:_work\svn_clean4\trunk\java\testing\org\apache\derbyTesting\funct
        ionTests\tests\lang\UnaryArithmeticParameterTest.java:262: assertColumnTypes(jav
        a.sql.ResultSet,int[]) in org.apache.derbyTesting.junit.JDBC cannot be applied t
        o (java.sql.PreparedStatement,int[])
        [javac] JDBC.assertColumnTypes(ps,expectedTypes);
        [javac] ^

        Seems like some other changes are missing.

        The comments don't indicate why this test requires JDBC 3? I couldn't see anything obvious that would stop this running on JSR169.

        FYI - a single patch file is much more convienent, than a separate one for the change to the suite file.

        For code like this:
        + Object[][] expectedRows = new Object[][]new String("-2"),new String("2")},{new String("-2"),new String("2");
        + JDBC.assertFullResultSet(ps.executeQuery(), expectedRows, true);

        it can be cleaned up a couple of ways:

        • new String("-2") is kind of pointless, "-2" is a String object, just it can be used directly.
        • a utility assertFullResultSet method exists that takes strings, so the code could be:

        + String[][] expectedRows = new String[][]"-2", "2"},{-2","2";
        + JDBC.assertFullResultSet(ps.executeQuery(), expectedRows);

        It looks like the suite method runs the fixtures twice in embedded, once adding them directly and once through defaultSuite

        Show
        Daniel John Debrunner added a comment - Patch doesn't compile for me, [javac] C:_work\svn_clean4\trunk\java\testing\org\apache\derbyTesting\funct ionTests\tests\lang\UnaryArithmeticParameterTest.java:262: assertColumnTypes(jav a.sql.ResultSet,int[]) in org.apache.derbyTesting.junit.JDBC cannot be applied t o (java.sql.PreparedStatement,int[]) [javac] JDBC.assertColumnTypes(ps,expectedTypes); [javac] ^ Seems like some other changes are missing. The comments don't indicate why this test requires JDBC 3? I couldn't see anything obvious that would stop this running on JSR169. FYI - a single patch file is much more convienent, than a separate one for the change to the suite file. For code like this: + Object[][] expectedRows = new Object[][] new String("-2"),new String("2")},{new String("-2"),new String("2") ; + JDBC.assertFullResultSet(ps.executeQuery(), expectedRows, true); it can be cleaned up a couple of ways: new String("-2") is kind of pointless, "-2" is a String object, just it can be used directly. a utility assertFullResultSet method exists that takes strings, so the code could be: + String[][] expectedRows = new String[][] "-2", "2"},{-2","2" ; + JDBC.assertFullResultSet(ps.executeQuery(), expectedRows); It looks like the suite method runs the fixtures twice in embedded, once adding them directly and once through defaultSuite
        Hide
        Manjula Kutty added a comment -

        >>Patch doesn't compile for me,
        >> [javac] C:_work\svn_clean4\trunk\java\testing\org\apache\derbyTesting\funct
        >>nTests\tests\lang\UnaryArithmeticParameterTest.java:262: assertColumnTypes(jav
        >>a.sql.ResultSet,int[]) in org.apache.derbyTesting.junit.JDBC cannot be applied t
        >>o (java.sql.PreparedStatement,int[])
        >> [javac] JDBC.assertColumnTypes(ps,expectedTypes);
        >> [javac] ^

        >>Seems like some other changes are missing.
        Sorry that in the last patch I forgot to include the changes I made in the junit/JDBC.java

        >>The comments don't indicate why this test requires JDBC 3? I couldn't see anything obvious that would stop this running on JSR169.
        Added comments to to reason why we need JDBC3. Also if I do specify only about JDBC3, I thought the test will run in JSR 169 (I may be wrong here...Please clear me if I'm wrong).

        >>FYI - a single patch file is much more convienent, than a separate one for the change to the suite file.
        Sure I will be attaching the recent patch in a single set.

        >>For code like this:
        >>+ Object[][] expectedRows = new Object[][]new String("-2"),new String("2")},{new String("-2"),new String("2");
        >>+ JDBC.assertFullResultSet(ps.executeQuery(), expectedRows, true);

        >>it can be cleaned up a couple of ways:
        >> - new String("-2") is kind of pointless, "-2" is a String object, just it can be used directly.
        >> - a utility assertFullResultSet method exists that takes strings, so the code could be:

        >> + String[][] expectedRows = new String[][]"-2", "2"},{-2","2";
        >>+ JDBC.assertFullResultSet(ps.executeQuery(), expectedRows);

        Will be fxing this in the next patch

        >>It looks like the suite method runs the fixtures twice in embedded, once adding them directly and once through defaultSuite
        I thought I give
        return TestConfiguration.defaultSuite(UnaryArithmeticParameterTest.class);
        The test will run both embedded and n/w server mode. (Please correct me if I'm wrong)

        Show
        Manjula Kutty added a comment - >>Patch doesn't compile for me, >> [javac] C:_work\svn_clean4\trunk\java\testing\org\apache\derbyTesting\funct >>nTests\tests\lang\UnaryArithmeticParameterTest.java:262: assertColumnTypes(jav >>a.sql.ResultSet,int[]) in org.apache.derbyTesting.junit.JDBC cannot be applied t >>o (java.sql.PreparedStatement,int[]) >> [javac] JDBC.assertColumnTypes(ps,expectedTypes); >> [javac] ^ >>Seems like some other changes are missing. Sorry that in the last patch I forgot to include the changes I made in the junit/JDBC.java >>The comments don't indicate why this test requires JDBC 3? I couldn't see anything obvious that would stop this running on JSR169. Added comments to to reason why we need JDBC3. Also if I do specify only about JDBC3, I thought the test will run in JSR 169 (I may be wrong here...Please clear me if I'm wrong). >>FYI - a single patch file is much more convienent, than a separate one for the change to the suite file. Sure I will be attaching the recent patch in a single set. >>For code like this: >>+ Object[][] expectedRows = new Object[][] new String("-2"),new String("2")},{new String("-2"),new String("2") ; >>+ JDBC.assertFullResultSet(ps.executeQuery(), expectedRows, true); >>it can be cleaned up a couple of ways: >> - new String("-2") is kind of pointless, "-2" is a String object, just it can be used directly. >> - a utility assertFullResultSet method exists that takes strings, so the code could be: >> + String[][] expectedRows = new String[][] "-2", "2"},{-2","2" ; >>+ JDBC.assertFullResultSet(ps.executeQuery(), expectedRows); Will be fxing this in the next patch >>It looks like the suite method runs the fixtures twice in embedded, once adding them directly and once through defaultSuite I thought I give return TestConfiguration.defaultSuite(UnaryArithmeticParameterTest.class); The test will run both embedded and n/w server mode. (Please correct me if I'm wrong)
        Hide
        Manjula Kutty added a comment -

        Thanks for the review, Dan.

        Attaching new patch in which I tried to take care of Dan's comments

        Show
        Manjula Kutty added a comment - Thanks for the review, Dan. Attaching new patch in which I tried to take care of Dan's comments
        Hide
        Daniel John Debrunner added a comment -

        Patch generally looks good but it's good to have clear & logical naming. The new utility method you've added JDBC.assertColumnTypes is not checking column types, it's checking parameter types. That's the danger of copying other code, as I see it see has comments in its javadoc related to DatabaseMetaData which is not relevant here.

        And then I would be more explicit on the JDBC3 requirement, just stating the test requires ParameterMetaData I think would be clearer than "it relies on jdk14 jdbc metadata calls"

        Show
        Daniel John Debrunner added a comment - Patch generally looks good but it's good to have clear & logical naming. The new utility method you've added JDBC.assertColumnTypes is not checking column types, it's checking parameter types. That's the danger of copying other code, as I see it see has comments in its javadoc related to DatabaseMetaData which is not relevant here. And then I would be more explicit on the JDBC3 requirement, just stating the test requires ParameterMetaData I think would be clearer than "it relies on jdk14 jdbc metadata calls"
        Hide
        Jean T. Anderson added a comment -

        So I'm a little confused by the "Patch generally looks good but it's good to have clear & logical naming."

        Is this patch ready to commit? If so, I'll go ahead and do so,

        Or do you want the naming changed first? Would it be sufficient for me to change the JDBC.assertColumnTypes to JDBC.assertParameterTypes when it's committed?

        Show
        Jean T. Anderson added a comment - So I'm a little confused by the "Patch generally looks good but it's good to have clear & logical naming." Is this patch ready to commit? If so, I'll go ahead and do so, Or do you want the naming changed first? Would it be sufficient for me to change the JDBC.assertColumnTypes to JDBC.assertParameterTypes when it's committed?
        Hide
        Jean T. Anderson added a comment -

        Actually, I can't apply the latest patch (DERBY-2458_diff_03_19.txt), I get this error:

        patching file java/testing/org/apache/derbyTesting/functionTests/tests/lang/_Suite.java
        patch: **** malformed patch at line 377: Index: java/testing/org/apache/derbyTesting/junit/JDBC.java

        I'm not spotting what the problem is in the patch file.

        Manjula, I suggest that you sync up to the latest svn revision, take care of the remaining comments, and upload a final patch.

        Show
        Jean T. Anderson added a comment - Actually, I can't apply the latest patch ( DERBY-2458 _diff_03_19.txt), I get this error: patching file java/testing/org/apache/derbyTesting/functionTests/tests/lang/_Suite.java patch: **** malformed patch at line 377: Index: java/testing/org/apache/derbyTesting/junit/JDBC.java I'm not spotting what the problem is in the patch file. Manjula, I suggest that you sync up to the latest svn revision, take care of the remaining comments, and upload a final patch.
        Hide
        Manjula Kutty added a comment -

        Jean and Dan thanks for reviewing the patch. Your comments are really very helpful.

        Attaching the new patch (hopefully the final one). I took care of most of Dan's comments.

        Please review it and if everything looks good please commit

        Show
        Manjula Kutty added a comment - Jean and Dan thanks for reviewing the patch. Your comments are really very helpful. Attaching the new patch (hopefully the final one). I took care of most of Dan's comments. Please review it and if everything looks good please commit
        Hide
        Jean T. Anderson added a comment -

        Almost there ... unfortunately DERBY-2458_diff_03_20.txt gets a Hunk failure at 77 – the problem is with _Suite.java. Please be sure to do an 'svn up'. For a tricky patch, especially if your svn working copy has a lot going on and your patch applies to just some of those changes, it might make sense to do a fresh checkout and apply your patch there to make sure it will work.

        Show
        Jean T. Anderson added a comment - Almost there ... unfortunately DERBY-2458 _diff_03_20.txt gets a Hunk failure at 77 – the problem is with _Suite.java. Please be sure to do an 'svn up'. For a tricky patch, especially if your svn working copy has a lot going on and your patch applies to just some of those changes, it might make sense to do a fresh checkout and apply your patch there to make sure it will work.
        Hide
        Manjula Kutty added a comment -

        Attaching the new patch.

        Show
        Manjula Kutty added a comment - Attaching the new patch.
        Hide
        Jean T. Anderson added a comment -

        Committed patch DERBY-2458_diff_03_20_ver2.txt, revision 520709. Thanks, Manjula!

        Show
        Jean T. Anderson added a comment - Committed patch DERBY-2458 _diff_03_20_ver2.txt, revision 520709. Thanks, Manjula!
        Hide
        Jean T. Anderson added a comment -

        One step that remains is to delete the old test. A checklist is at http://wiki.apache.org/db-derby/ConvertOldTestToJunitTips#DeleteOldHarnessFiles

        Show
        Jean T. Anderson added a comment - One step that remains is to delete the old test. A checklist is at http://wiki.apache.org/db-derby/ConvertOldTestToJunitTips#DeleteOldHarnessFiles
        Hide
        Manjula Kutty added a comment -

        Thanks for the commit Jean. Here is the additional patch that will take care of the deletion on old test and update to the derbylang.runall file. Please review and if looks good please commit

        Show
        Manjula Kutty added a comment - Thanks for the commit Jean. Here is the additional patch that will take care of the deletion on old test and update to the derbylang.runall file. Please review and if looks good please commit
        Hide
        Jean T. Anderson added a comment -

        Committed patch DERBY-2458_diff_03_21.txt, revision 521036 with one additional change that removed unaryArithmeticDynamicParameter.java from suites/derbynetmats.runall. Thanks, Manjula!

        Show
        Jean T. Anderson added a comment - Committed patch DERBY-2458 _diff_03_21.txt, revision 521036 with one additional change that removed unaryArithmeticDynamicParameter.java from suites/derbynetmats.runall. Thanks, Manjula!
        Hide
        Manjula Kutty added a comment -

        Thanks for the commit , Jean

        Show
        Manjula Kutty added a comment - Thanks for the commit , Jean

          People

          • Assignee:
            Manjula Kutty
            Reporter:
            Manjula Kutty
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development