Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.3.1.4
    • Fix Version/s: 10.3.1.4
    • Component/s: Test
    • Labels:
      None
    1. derby-2005.diff
      23 kB
      Fernanda Pizzorno
    2. derby-2005.stat
      0.7 kB
      Fernanda Pizzorno
    3. derby-2005v2.diff
      22 kB
      Fernanda Pizzorno
    4. derby-2005v2.stat
      0.7 kB
      Fernanda Pizzorno
    5. derby-2005v3.diff
      1 kB
      Fernanda Pizzorno
    6. derby-2005v3.stat
      0.2 kB
      Fernanda Pizzorno

      Activity

      Gavin made changes -
      Workflow jira [ 12387823 ] Default workflow, editable Closed status [ 12798158 ]
      Knut Anders Hatlen made changes -
      Status Resolved [ 5 ] Closed [ 6 ]
      Knut Anders Hatlen made changes -
      Derby Info [Patch Available]
      Fix Version/s 10.3.0.0 [ 12310800 ]
      Resolution Fixed [ 1 ]
      Status Open [ 1 ] Resolved [ 5 ]
      Hide
      Knut Anders Hatlen added a comment -

      Thanks Fernanda. Committed revision 470305.

      Show
      Knut Anders Hatlen added a comment - Thanks Fernanda. Committed revision 470305.
      Fernanda Pizzorno made changes -
      Derby Info [Patch Available]
      Fernanda Pizzorno made changes -
      Attachment derby-2005v3.diff [ 12344176 ]
      Attachment derby-2005v3.stat [ 12344177 ]
      Hide
      Fernanda Pizzorno added a comment -

      The attached patch (derby-2005v3.diff) addresses Daniel's comments. I have successfully run jdbcapi._Suite with this patch. Can someone please review/commit it?

      Show
      Fernanda Pizzorno added a comment - The attached patch (derby-2005v3.diff) addresses Daniel's comments. I have successfully run jdbcapi._Suite with this patch. Can someone please review/commit it?
      Hide
      Daniel John Debrunner added a comment -

      I committed the patch but it does contain some cleanup code that I think itself could be cleaned up, e.g. in runGetStreamTwiceTest

      } finally {
      if (st != null)

      { st.close(); }

      if (rs != null)

      { rs.close(); }

      if (is != null)

      { is.close(); }

      }

      In these cases st and rs can never be null, so the check to see if they are null is confusing. There is also no need to have them inside the finally block.
      The closing of the stream 'is' I think is also dubious at that point, the test does not expect the stream to be successfully opened so is there a benefit to having cleanup code for the stream?

      My feeling is that that the cleanup code can distract from the actual focus of the test, in this case did the second rs.getXXX fail.
      All that is really needed here is no finally block and then simply

      rs.close();
      st.close();

      or possibly since closing st is defined to close rs, just:

      st.close();

      Show
      Daniel John Debrunner added a comment - I committed the patch but it does contain some cleanup code that I think itself could be cleaned up, e.g. in runGetStreamTwiceTest } finally { if (st != null) { st.close(); } if (rs != null) { rs.close(); } if (is != null) { is.close(); } } In these cases st and rs can never be null, so the check to see if they are null is confusing. There is also no need to have them inside the finally block. The closing of the stream 'is' I think is also dubious at that point, the test does not expect the stream to be successfully opened so is there a benefit to having cleanup code for the stream? My feeling is that that the cleanup code can distract from the actual focus of the test, in this case did the second rs.getXXX fail. All that is really needed here is no finally block and then simply rs.close(); st.close(); or possibly since closing st is defined to close rs, just: st.close();
      Hide
      Daniel John Debrunner added a comment -

      Committed revision 470152 patch Thanks Fernanda

      Show
      Daniel John Debrunner added a comment - Committed revision 470152 patch Thanks Fernanda
      Fernanda Pizzorno made changes -
      Attachment derby-2005v2.stat [ 12344144 ]
      Attachment derby-2005v2.diff [ 12344143 ]
      Hide
      Fernanda Pizzorno added a comment -

      Thank you for reviewing the patch Kristian.

      I am submiting a new patch that addresses Kristian's comments. I have successfully run jdbcapi._Suite with this patch. Can someone please review/commit it?

      Show
      Fernanda Pizzorno added a comment - Thank you for reviewing the patch Kristian. I am submiting a new patch that addresses Kristian's comments. I have successfully run jdbcapi._Suite with this patch. Can someone please review/commit it?
      Hide
      Kristian Waagan added a comment -

      Hi Fernanda,

      The patch looks good as it is. I have a few questions/suggestions you could consider:
      1) Could the test-methods be declared to throw IOException and SQLException instead of Exception?
      2) As far as I can see, the TestDataStream/-Reader does not have any special functionality. To reduce the amount of code in the test class, could you instead use the existing stream/reader in 'functionTests.util.streams'?
      3) Very much a nit, feel free to ignore, a space is missing in front of the starting curly brace on lines 53 and 176 (in the diff).

      I ran the test individually and as part of jdbciapi/_Suite, both from classes and jars.
      For some reason (not related to this test I think), the security manager denied access to read the property 'user.dir' on the machine I tested on (Gentoo Linux, AMD64, Java 1.5.0_08-b03 and Java SE 6 b103). I had to add the permission to the policy file. Has anyone else seen this?

      Good work on the test, I think it can be committed as soon as the patch available flag is set.

      Show
      Kristian Waagan added a comment - Hi Fernanda, The patch looks good as it is. I have a few questions/suggestions you could consider: 1) Could the test-methods be declared to throw IOException and SQLException instead of Exception? 2) As far as I can see, the TestDataStream/-Reader does not have any special functionality. To reduce the amount of code in the test class, could you instead use the existing stream/reader in 'functionTests.util.streams'? 3) Very much a nit, feel free to ignore, a space is missing in front of the starting curly brace on lines 53 and 176 (in the diff). I ran the test individually and as part of jdbciapi/_Suite, both from classes and jars. For some reason (not related to this test I think), the security manager denied access to read the property 'user.dir' on the machine I tested on (Gentoo Linux, AMD64, Java 1.5.0_08-b03 and Java SE 6 b103). I had to add the permission to the policy file. Has anyone else seen this? Good work on the test, I think it can be committed as soon as the patch available flag is set.
      Kristian Waagan made changes -
      Affects Version/s 10.3.0.0 [ 12310800 ]
      Component/s Test [ 11413 ]
      Fernanda Pizzorno made changes -
      Field Original Value New Value
      Attachment derby-2005.stat [ 12343946 ]
      Attachment derby-2005.diff [ 12343945 ]
      Hide
      Fernanda Pizzorno added a comment -

      The attached patch (derby-2005.diff) converts the test jdbcapi/Stream.java to Junit.

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

      I have successfully run jdbcapi._Suite with this patch. Can someone please review it?

      Show
      Fernanda Pizzorno added a comment - The attached patch (derby-2005.diff) converts the test jdbcapi/Stream.java to Junit. A summary of what it tested by this test can be found at: http://wiki.apache.org/db-derby/StreamTestCoverage . I have successfully run jdbcapi._Suite with this patch. Can someone please review it?
      Hide
      Andrew McIntyre added a comment -

      testing JIRA mail - please ignore

      Show
      Andrew McIntyre added a comment - testing JIRA mail - please ignore
      Fernanda Pizzorno created issue -

        People

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

          Dates

          • Created:
            Updated:
            Resolved:

            Development