Fernanda Pizzorno
added a comment - 30/Oct/06 17:22 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?

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
added a comment - 31/Oct/06 09:38 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.

Fernanda Pizzorno
added a comment - 01/Nov/06 22:39 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?

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

Daniel John Debrunner
added a comment - 02/Nov/06 01:04 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();