Derby
  1. Derby
  2. DERBY-2658

Convert jdbcapi/parameterMetaDataJdbc30.java to JUnit

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.3.1.4
    • Fix Version/s: 10.3.3.0, 10.4.1.3
    • Component/s: Test
    • Labels:
      None

      Description

      Convert jdbcapi/parameterMetaDataJdbc30.java to JUnit.

      1. DERBY-2658v4.diff
        2 kB
        Ramin Moazeni
      2. DERBY-2658_061807.stat
        0.5 kB
        Ramin Moazeni
      3. DERBY-2658_061807.diff
        37 kB
        Ramin Moazeni
      4. DERBY-2658.diff-061307
        71 kB
        Ramin Moazeni
      5. DERBY-2658.stat-060807
        0.6 kB
        Ramin Moazeni
      6. DERBY-2658.diff-060807
        57 kB
        Ramin Moazeni
      7. DERBY-2658.diff
        38 kB
        Ramin Moazeni

        Issue Links

          Activity

          Hide
          Ramin Moazeni added a comment -

          Please review the attached interim patch.

          Show
          Ramin Moazeni added a comment - Please review the attached interim patch.
          Hide
          Kathey Marsden added a comment -

          Thanks Ramin for looking at this test conversion. The main thing I think is that we really need to make sure that we are testing everything the old test tested. Below are some specific comments, but it would be good to do a close review comparing the old and new test to make sure that we have assertions for all of the cases that were tested by canon comparison before. Anyway, here are some comments ....

          Typically the test class name for the junit tests starts with a capital letter. e.g. ParameterMetaDataJdbc30Test

          parameterMetaDataJdbc30_app.properties and master/parameterMetaDataJdbc30.out should be deleted.

          Attach the output of svn stat to your next patch.

          When I run the test all the tests fail with
          parameterMetaDataJdbc30Test)java.sql.SQLException: Schema 'SAMPLE' does not exist
          This is I think because setUp attempts to drop the table that is not there.
          I think you would be better off using CleanDatabaseSetup and the decorateSQL method to create your table and function, rather than creating it with each fixture. There are lots of examples of CleanDatabaseSetup usage in other tests.

          The dumpExpectedSQLExceptions method only checks an SQLException's sqlstate against itself, so would always pass. Instead of calls to dumpExpectedSQLExceptions
          you should have assertSQLState("XXXXX",e) where XXXX is the hardcoded expected SQLState. You can see the expected states by looking at the canon in functionTests/master/parameterMetaDataJdbc30.out or just temporarily printing SQLState to see what it is.

          Calls like assertTrue ("Jira 44 failed.", "22019".equals(e.getSQLState()) can be replaced by assertSQLState

          The System.out's in the old test need to be translated into assertions. e.g.
          //System.out.println("parameters count for callable statement is: " + paramMetaData.getParameterCount());
          should be replaced with an assertEquals call.

          dumpParameterMetaDataNegative doesn't look quite right to me. It seems like it would always fail. Instead of

          try

          { fail("parameter isNullable " + paramMetaData.isNullable(-1)); }

          catch (SQLException e)

          { dumpExpectedSQLExceptions(e); }

          I would expect something like.
          try

          { parameterMetaData.isNullable(-1); fail("parameterMetaData.isNullable(-1) should have failed); }

          catch (SQLException se)

          { assertSQLState("XCL13",se.getSQLState()); }

          For the code where we have if(!usingDerbyNetClient()) there should be an explanation of why we don't run with client with a Jira reference. If there is a bug and it is not filed, file one.

          The fixtures testParameterMetadataWithXXXParameters () doesn't actually seem to test the parameterMetaData. I don't know if that is an artifact of the old test but we should add in some testing.

          Show
          Kathey Marsden added a comment - Thanks Ramin for looking at this test conversion. The main thing I think is that we really need to make sure that we are testing everything the old test tested. Below are some specific comments, but it would be good to do a close review comparing the old and new test to make sure that we have assertions for all of the cases that were tested by canon comparison before. Anyway, here are some comments .... Typically the test class name for the junit tests starts with a capital letter. e.g. ParameterMetaDataJdbc30Test parameterMetaDataJdbc30_app.properties and master/parameterMetaDataJdbc30.out should be deleted. Attach the output of svn stat to your next patch. When I run the test all the tests fail with parameterMetaDataJdbc30Test)java.sql.SQLException: Schema 'SAMPLE' does not exist This is I think because setUp attempts to drop the table that is not there. I think you would be better off using CleanDatabaseSetup and the decorateSQL method to create your table and function, rather than creating it with each fixture. There are lots of examples of CleanDatabaseSetup usage in other tests. The dumpExpectedSQLExceptions method only checks an SQLException's sqlstate against itself, so would always pass. Instead of calls to dumpExpectedSQLExceptions you should have assertSQLState("XXXXX",e) where XXXX is the hardcoded expected SQLState. You can see the expected states by looking at the canon in functionTests/master/parameterMetaDataJdbc30.out or just temporarily printing SQLState to see what it is. Calls like assertTrue ("Jira 44 failed.", "22019".equals(e.getSQLState()) can be replaced by assertSQLState The System.out's in the old test need to be translated into assertions. e.g. //System.out.println("parameters count for callable statement is: " + paramMetaData.getParameterCount()); should be replaced with an assertEquals call. dumpParameterMetaDataNegative doesn't look quite right to me. It seems like it would always fail. Instead of try { fail("parameter isNullable " + paramMetaData.isNullable(-1)); } catch (SQLException e) { dumpExpectedSQLExceptions(e); } I would expect something like. try { parameterMetaData.isNullable(-1); fail("parameterMetaData.isNullable(-1) should have failed); } catch (SQLException se) { assertSQLState("XCL13",se.getSQLState()); } For the code where we have if(!usingDerbyNetClient()) there should be an explanation of why we don't run with client with a Jira reference. If there is a bug and it is not filed, file one. The fixtures testParameterMetadataWithXXXParameters () doesn't actually seem to test the parameterMetaData. I don't know if that is an artifact of the old test but we should add in some testing.
          Hide
          Myrna van Lunteren added a comment -

          Unchecking 10.3. If this conversion gets checked in before 10.3, it can be marked fixed in 10.3 when it's being resolved.

          Show
          Myrna van Lunteren added a comment - Unchecking 10.3. If this conversion gets checked in before 10.3, it can be marked fixed in 10.3 when it's being resolved.
          Hide
          Ramin Moazeni added a comment -

          Attaching another patch with this issue that address the previous review.
          Feedback is appreciated.

          Show
          Ramin Moazeni added a comment - Attaching another patch with this issue that address the previous review. Feedback is appreciated.
          Hide
          Kathey Marsden added a comment -

          Hi Ramin,

          I think we are still in a situation where we are not testing everything the original test tested.
          In the old test for each parameter it would print the following information:

          parameters count for callable statement is 4
          Parameter number : 1
          parameter isNullable PARAMETER_NULLABLE
          parameter isSigned true
          parameter getPrecision 10
          parameter getScale 0
          parameter getParameterType 4
          parameter getParameterTypeName INTEGER
          parameter getParameterClassName java.lang.Integer
          parameter getParameterMode PARAMETER_MODE_IN

          That means that you will need to add assertions for each of these calls for each of the parameters in order to test the same thing.

          Show
          Kathey Marsden added a comment - Hi Ramin, I think we are still in a situation where we are not testing everything the original test tested. In the old test for each parameter it would print the following information: parameters count for callable statement is 4 Parameter number : 1 parameter isNullable PARAMETER_NULLABLE parameter isSigned true parameter getPrecision 10 parameter getScale 0 parameter getParameterType 4 parameter getParameterTypeName INTEGER parameter getParameterClassName java.lang.Integer parameter getParameterMode PARAMETER_MODE_IN That means that you will need to add assertions for each of these calls for each of the parameters in order to test the same thing.
          Hide
          Ramin Moazeni added a comment -

          Attaching another patch that addresses previous review.

          Show
          Ramin Moazeni added a comment - Attaching another patch that addresses previous review.
          Hide
          Kathey Marsden added a comment -

          Thanks Ramin for the patch.
          I committed it with revision
          New Revision: 547349

          URL: http://svn.apache.org/viewvc?view=rev&rev=547349

          It would be good to add some javadoc comments and I did have this question. There is a comment

          // TODO: Some of the OUT params are getting reported as IN_OUT for embedded.
          // Network server reports it correctly.

          but I don't see any test showing this behavior. Do we need a new test and Jira entry for this or is it no longer an issue?

          Show
          Kathey Marsden added a comment - Thanks Ramin for the patch. I committed it with revision New Revision: 547349 URL: http://svn.apache.org/viewvc?view=rev&rev=547349 It would be good to add some javadoc comments and I did have this question. There is a comment // TODO: Some of the OUT params are getting reported as IN_OUT for embedded. // Network server reports it correctly. but I don't see any test showing this behavior. Do we need a new test and Jira entry for this or is it no longer an issue?
          Hide
          Knut Anders Hatlen added a comment -

          JDBCHarnessJavaTest started failing with ClassNotFoundException after this commit. Fixed in revision 547567.

          Show
          Knut Anders Hatlen added a comment - JDBCHarnessJavaTest started failing with ClassNotFoundException after this commit. Fixed in revision 547567.
          Hide
          Ramin Moazeni added a comment -

          Hi Kathey

          I have added javadoc comments to the test. The patch is attached.

          As for the following comment,
          // TODO: Some of the OUT params are getting reported as IN_OUT for embedded.
          // Network server reports it correctly.
          I was not able to reproduce it and therefore, I removed the comment.

          Please note that derbyall and suites.All both passed without any errors.

          Show
          Ramin Moazeni added a comment - Hi Kathey I have added javadoc comments to the test. The patch is attached. As for the following comment, // TODO: Some of the OUT params are getting reported as IN_OUT for embedded. // Network server reports it correctly. I was not able to reproduce it and therefore, I removed the comment. Please note that derbyall and suites.All both passed without any errors.
          Hide
          A B added a comment -

          As of svn # 547349 for this issue, suites.All has been failing when run with weme6.1. It turns out the that new ParameterMetaDataJdbc30Test fails with the following error:

          Failed to invoke suite():java.lang.NoClassDefFoundError: java.sql.ParameterMetaData

          There's no stack trace or logging so I don't have any more than that...

          Show
          A B added a comment - As of svn # 547349 for this issue, suites.All has been failing when run with weme6.1. It turns out the that new ParameterMetaDataJdbc30Test fails with the following error: Failed to invoke suite():java.lang.NoClassDefFoundError: java.sql.ParameterMetaData There's no stack trace or logging so I don't have any more than that...
          Hide
          A B added a comment -

          Filed DERBY-2862 for the weme6.1 failure since I haven't heard anything back on this...

          Show
          A B added a comment - Filed DERBY-2862 for the weme6.1 failure since I haven't heard anything back on this...
          Hide
          Myrna van Lunteren added a comment -

          Now this is interesting, what do I mark this fixed as? The new test went into 10.3.0.0, the cleanup into 10.4 and 10.3.1.1. As test issues don't need to go into the release notes, it doesn't matter from that point of view. Marking all 3 versions.

          Show
          Myrna van Lunteren added a comment - Now this is interesting, what do I mark this fixed as? The new test went into 10.3.0.0, the cleanup into 10.4 and 10.3.1.1. As test issues don't need to go into the release notes, it doesn't matter from that point of view. Marking all 3 versions.
          Hide
          Ramin Moazeni added a comment -

          Reopening this issue to fix the assertSQLState()s.

          The following is the comments from Dan posted on derby-dev alias:
          -----------------------------------------------------------------------------------------
          All the assertSQLState() have the incorrect parameters in that test
          (parameterMetadataJdbc30Test) so that they can never fail.

          E.g.

          assertSQLState("22019", e.getSQLState(), e);

          The first argument is the message, the second the expected state. Here
          the SQL state from the exception itself is being passed as expected, and
          thus will be compared to itself and always pass. If no message is needed
          then the call should be:

          assertSQLState("22019", e);
          -----------------------------------------------------------------------------------------

          Show
          Ramin Moazeni added a comment - Reopening this issue to fix the assertSQLState()s. The following is the comments from Dan posted on derby-dev alias: ----------------------------------------------------------------------------------------- All the assertSQLState() have the incorrect parameters in that test (parameterMetadataJdbc30Test) so that they can never fail. E.g. assertSQLState("22019", e.getSQLState(), e); The first argument is the message, the second the expected state. Here the SQL state from the exception itself is being passed as expected, and thus will be compared to itself and always pass. If no message is needed then the call should be: assertSQLState("22019", e); -----------------------------------------------------------------------------------------
          Hide
          Ramin Moazeni added a comment -

          Attaching the v4 diff to fix the issues Dan pointed out with regard to assertSQLState()s.

          Show
          Ramin Moazeni added a comment - Attaching the v4 diff to fix the issues Dan pointed out with regard to assertSQLState()s.
          Hide
          Andrew McIntyre added a comment -

          Should rev 572662 be backported to the 10.3 branch so we can close this issue again?

          Show
          Andrew McIntyre added a comment - Should rev 572662 be backported to the 10.3 branch so we can close this issue again?
          Hide
          Dyre Tjeldvoll added a comment -

          rev 572662 merged to 10.3 in revision 638919.

          Show
          Dyre Tjeldvoll added a comment - rev 572662 merged to 10.3 in revision 638919.

            People

            • Assignee:
              Ramin Moazeni
              Reporter:
              Ramin Moazeni
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development