Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 10.3.1.4
    • Component/s: Test
    • Labels:
      None
    1. derby-1895.diff
      403 kB
      Fernanda Pizzorno
    2. derby-1895.html
      9 kB
      Fernanda Pizzorno
    3. derby-1895.stat
      1 kB
      Fernanda Pizzorno
    4. derby-1895v2.diff
      401 kB
      Fernanda Pizzorno
    5. derby-1895v2.stat
      1 kB
      Fernanda Pizzorno

      Activity

      Hide
      Fernanda Pizzorno added a comment -

      The attached patch (derby-1895.diff) converts the test jdbcapi/blobclob4blob.java to junit. The attached html file (derby-1895.html) explains the changes done to the test.

      A summary of what it tested by this test can be found at: http://wiki.apache.org/db-derby/BlobClob4BlobTestCoverage.

      Can someone please review the patch?

      Show
      Fernanda Pizzorno added a comment - The attached patch (derby-1895.diff) converts the test jdbcapi/blobclob4blob.java to junit. The attached html file (derby-1895.html) explains the changes done to the test. A summary of what it tested by this test can be found at: http://wiki.apache.org/db-derby/BlobClob4BlobTestCoverage . Can someone please review the patch?
      Hide
      Knut Anders Hatlen added a comment -

      Hi Fernanda. Thank you for rewriting the test! I had a quick look at your patch and the description of it. A couple of question/comments:

      • Some of the test cases (testSetClobToIntColumn, testSetClobToIntColumn, testSetClobToIntColumn) have comments saying "need to run prepareCLOBMAIN first", but there is no such method.
      • I'm not sure I understand what testClobFinalizer and testBlobFinalizer test. Is their only purpose to force the finalizers to run? If so, would it make sense to add System.runFinalization() after System.gc().
      • The testPosition* cases use random start position and length. If these tests fail, it could be hard to reproduce because we don't know the actual values. Could the start position and length be part of the error message? (I saw the println() statements, but they only print the information when the verbose flag is true.)
      • runPositionStringTest() has a workaround for DERBY-1917. When the test has been committed, a comment should be added to 1917 that the test should be cleaned up when it has been fixed.
      • The suite() method adds each test case manually. I think it would be better to use the built-in JUnit functionality for adding all test* methods and only add those test cases that don't run under DerbyNet manually. That would make it easier to add more test cases later, and it would be more consistent with the other JUnit tests.
      • Minor nit: many calls to assertTrue("XXX", false) would be easier to read if written as fail("XXX").
      • I found many constructions similar to this:

      + try {
      + clob.length();
      + if (usingEmbedded())

      { + assertTrue("FAIL - should not be able to access large log " + + "after commit", false); + }

      + } catch (SQLException e)

      { + checkException(BLOB_ACCESSED_AFTER_COMMIT, e); + }

      If I understand these constructions correctly, they expect embedded to fail with a given SQLState, but the network client and JCC should succeed. But under DerbyNet and DerbyNetClient, these test cases will succeed both when the call succeeds and when the call fails with the given SQLState. Is this how it was intended?

      Show
      Knut Anders Hatlen added a comment - Hi Fernanda. Thank you for rewriting the test! I had a quick look at your patch and the description of it. A couple of question/comments: Some of the test cases (testSetClobToIntColumn, testSetClobToIntColumn, testSetClobToIntColumn) have comments saying "need to run prepareCLOBMAIN first", but there is no such method. I'm not sure I understand what testClobFinalizer and testBlobFinalizer test. Is their only purpose to force the finalizers to run? If so, would it make sense to add System.runFinalization() after System.gc(). The testPosition* cases use random start position and length. If these tests fail, it could be hard to reproduce because we don't know the actual values. Could the start position and length be part of the error message? (I saw the println() statements, but they only print the information when the verbose flag is true.) runPositionStringTest() has a workaround for DERBY-1917 . When the test has been committed, a comment should be added to 1917 that the test should be cleaned up when it has been fixed. The suite() method adds each test case manually. I think it would be better to use the built-in JUnit functionality for adding all test* methods and only add those test cases that don't run under DerbyNet manually. That would make it easier to add more test cases later, and it would be more consistent with the other JUnit tests. Minor nit: many calls to assertTrue("XXX", false) would be easier to read if written as fail("XXX"). I found many constructions similar to this: + try { + clob.length(); + if (usingEmbedded()) { + assertTrue("FAIL - should not be able to access large log " + + "after commit", false); + } + } catch (SQLException e) { + checkException(BLOB_ACCESSED_AFTER_COMMIT, e); + } If I understand these constructions correctly, they expect embedded to fail with a given SQLState, but the network client and JCC should succeed. But under DerbyNet and DerbyNetClient, these test cases will succeed both when the call succeeds and when the call fails with the given SQLState. Is this how it was intended?
      Hide
      Fernanda Pizzorno added a comment -

      Knut Anders' comments are addressed in the attached patch (derby-1895v2.diff). I have succesfully run org.apache.derbyTesting.functionTests.tests.jdbcapi._Suite with this patch. Can someone please review it?

      Show
      Fernanda Pizzorno added a comment - Knut Anders' comments are addressed in the attached patch (derby-1895v2.diff). I have succesfully run org.apache.derbyTesting.functionTests.tests.jdbcapi._Suite with this patch. Can someone please review it?
      Hide
      Fernanda Pizzorno added a comment -

      I am also not sure what testClobFinalizer and testBlobFinalizer test. I left them as they were because there was a note saying that they are included in the test run to exercise the code even though the didn't produce any output.

      Knut Anders wrote:

      [...]

      • I'm not sure I understand what testClobFinalizer and testBlobFinalizer test. Is their only purpose to force the finalizers to run? If so, would it make sense to add System.runFinalization() after System.gc().
        [...]
      Show
      Fernanda Pizzorno added a comment - I am also not sure what testClobFinalizer and testBlobFinalizer test. I left them as they were because there was a note saying that they are included in the test run to exercise the code even though the didn't produce any output. Knut Anders wrote: [...] I'm not sure I understand what testClobFinalizer and testBlobFinalizer test. Is their only purpose to force the finalizers to run? If so, would it make sense to add System.runFinalization() after System.gc(). [...]
      Hide
      Dyre Tjeldvoll added a comment -

      I have run the test successfully, both as part of jdbcapi._Suite and as part of suites.All. So unless the test(C|B)lobFinalizer are considered blockers, I think it would good to commit this patch.

      Show
      Dyre Tjeldvoll added a comment - I have run the test successfully, both as part of jdbcapi._Suite and as part of suites.All. So unless the test(C|B)lobFinalizer are considered blockers, I think it would good to commit this patch.
      Hide
      Knut Anders Hatlen added a comment -

      Thanks Fernanda. Committed revision 465601.

      Show
      Knut Anders Hatlen added a comment - Thanks Fernanda. Committed revision 465601.

        People

        • Assignee:
          Fernanda Pizzorno
          Reporter:
          Fernanda Pizzorno
        • Votes:
          1 Vote for this issue
          Watchers:
          0 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved:

            Development