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-2005v3.diff
      1 kB
      Fernanda Pizzorno
    2. derby-2005v3.stat
      0.2 kB
      Fernanda Pizzorno
    3. derby-2005v2.diff
      22 kB
      Fernanda Pizzorno
    4. derby-2005v2.stat
      0.7 kB
      Fernanda Pizzorno
    5. derby-2005.diff
      23 kB
      Fernanda Pizzorno
    6. derby-2005.stat
      0.7 kB
      Fernanda Pizzorno

      Activity

      Hide
      Knut Anders Hatlen added a comment -

      Thanks Fernanda. Committed revision 470305.

      Show
      Knut Anders Hatlen added a comment - Thanks Fernanda. Committed revision 470305.
      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
      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.
      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

        People

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

          Dates

          • Created:
            Updated:
            Resolved:

            Development