David W. Van Couvering wrote:
I agree they should be submitted as independent patches. But they
should also be committable independently.
They are committable independently, however it is a good idea to commit
the patches incrementally, as they were submitted:
1. Review and commit DERBY-918. Test with existing junit tests
2. Review and commit DERBY-934. Test using junit.textui.TestRunner or by
using the feature provided in DERBY-918.
3. Review and commit 795. Test using existing junit tests (934)
I can't verify DERBY-918 without a test that works with the .junit test
type. That's provided in DERBY-934. Why not provide a simple sanity
Junit test with this patch that I can use to verify?
I agree that it would be great to have a simple sanity junit test.
The feature DERBY-918 is the ability to launch a junit test from the old
harness, without having to make a new main method for every junit test.
It can be tested with the old junit tests. The fact that the old tests
fails, (due to the fact that they were never designed to run in
junit.textui.TestRunner), do not make them valueless in terms of
verifying the feature.
Then, I can't verify DERBY-795 is fixed without applying and running the
tests in DERBY-934. It would be great if DERBY-795 included a test that
verifies it is fixed as part of the patch.
DERBY-795 comes with a test in the description. It is not part of the
patch, since I know this will be tested as part of another patch.
DERBY-934 can be run directly from junit.textui.TestRunner's main, and
can therefore be verified without DERBY-918.
That would make them *truly* independent, not just in terms of what they
are about, but in terms of the ability of the committer to review and
test them independently.
I do not see how it affects the committers ability to review the patches
independently. Huge patches with a lot of soon-to-be obsolete code, is
harder to review than small patches, which can be reviewed and committed
incrementally.
Andreas
David
Andreas Korneliussen wrote:
David W. Van Couvering wrote:
Hi, Andreas. Upon further thought, once we work through the comments
on DERBY-934 (still to come) I am going to go ahead and apply all
these patches at once, no need for you to do extra work. But a
request for next time to please try and keep your patches independent
instead of interdependent.
That is great.
I think the patches are independent, however they are slightly related.
DERBY-918: a patch for improving the test harness
DERBY-934: a set of tests which can be run independentely using
junit.textui.TestRunner or by using the test harness with patch 918
DERBY-795: a patch for a specific bug in derby. I did not submit extra
tests for this, since it is covered in 934, however there is a simple
java program there, which can be run to verify the fix and the bug
There are no compile dependencies between these patches. I think it
was much better to submit these as independent patches, instead of in
one huge patch.
Andreas
Thanks!
David
David W. Van Couvering wrote:
Hi, Andreas, your set of patches have a set of dependencies which
are a little confusing at first, and ultimately somewhat intractable:
DERBY-795 is tested by DERBY-934
DERBY-934 can't be run without, and therefore depends upon, DERBY-918
I really can't just commit one of these patches at a time, it has to
be all or none.
I really would like each of these patches stand on their own, or at
a minimum don't submit a dependent patch until the patch it depends
upon has been committed.
Here's what I would like to see:
DERBY-918 comes with its own sample unit test that verifies that the
.junit test type works. Something very simple and easy.
DERBY-795 has its own test that comes with it, rather than being
tested by DERBY-934
I have some comments on DERBY-934 too, I'll send these in a separate
email.
Thanks,
David
David W. Van Couvering wrote:
Crud, I missed this comment somehow, I'll look at DERBY-934 again,
I bet *both* my questions will be answered :)
I'll get back to you if I need anything else, Andreas.
David
Andreas Korneliussen (JIRA) wrote:
[ http://issues.apache.org/jira/browse/DERBY-795?page=all ]
Andreas Korneliussen updated DERBY-795:
---------------------------------------
Attachment: DERBY-795.diff
DERBY-795.diff
Attached is a fix for this issue.
The problem is detected by the jdbcapi/SURQueryMix.junit test
provided in DERBY-934, when running in embedded mode.
After calling ResultSet.relative(0) the cursor loses its position
-----------------------------------------------------------------
Key: DERBY-795
URL: http://issues.apache.org/jira/browse/DERBY-795
Project: Derby
Type: Bug
Components: JDBC
Versions: 10.1.2.1
Environment: Any
Reporter: Andreas Korneliussen
Assignee: Andreas Korneliussen
Priority: Minor
Attachments: DERBY-795.diff, DERBY-795.diff
After calling rs.relative(0), on a scrollable ResultSet, the
cursor looses its position, and a rs.getXXX(..) fails with:
SQL Exception: Invalid cursor state - no current row.
Probably caused by the following logic in
ScrollInsensitiveResultSet.getRelativeRow(int row):
// Return the current row for 0
if (row == 0)
{
if ((beforeFirst || afterLast) ||
(!beforeFirst && !afterLast)) {
return null;
} else {
return getRowFromHashTable(currentPosition);
}
}
The if () will always evaluate to true, regardless of the values
of beforeFirst and afterLast
Test code:
import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Statement;
public class RelativeZeroIssue {
public static void main(String[] args) throws Exception {
Class.forName("org.apache.derby.jdbc.EmbeddedDriver");
Connection con =
DriverManager.getConnection("jdbc:derby:testdb2;create=true");
con.setAutoCommit(false);
try { Statement statement =
con.createStatement();
/** Create the table */
statement.execute("create table t1(id int)");
statement.execute("insert into t1 values
1,2,3,4,5,6,7,8");
Statement s =
con.createStatement(ResultSet.TYPE_SCROLL_INSENSITIVE,
ResultSet.CONCUR_READ_ONLY);
ResultSet rs = s.executeQuery("select * from t1");
rs.next();
System.out.println(rs.getInt(1));
System.out.println(rs.relative(0));
System.out.println(rs.getInt(1));
} finally {
con.rollback();
con.close();
}
}
}
Output from test:
1
false
Exception in thread "main" SQL Exception: Invalid cursor state -
no current row.
at
org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Unknown Source)
at
org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Unknown Source)
at
org.apache.derby.impl.jdbc.Util.generateCsSQLException(Unknown
Source)
at
org.apache.derby.impl.jdbc.EmbedConnection.newSQLException(Unknown
Source)
at
org.apache.derby.impl.jdbc.ConnectionChild.newSQLException(Unknown
Source)
at
org.apache.derby.impl.jdbc.EmbedResultSet.checkOnRow(Unknown Source)
at
org.apache.derby.impl.jdbc.EmbedResultSet.getColumn(Unknown Source)
at
org.apache.derby.impl.jdbc.EmbedResultSet.getInt(Unknown Source)
at
derbytest.RelativeZeroIssue.main(RelativeZeroIssue.java:51)
Java Result: 1
