Thread: Unit test patches
Hi, running the unit tests against the latest cvs, i get 3 failures...
Patches attached to correct them.
The first occurs in DatabaseMetaDataTest.testTables line 104:
It fetches all the columns for any table beginning with test - i happen
to have other tables named test... which causes this to fail. I've
modified it to look for the testmetadata table, alternatively the docs
should be altered to state that the test database should be clean.
The second in TimeTest.timeTest : line 269 & 283:
The timezone tests fail because the as it happens local daylight savings
is in effect, the code tries to test for this by submitting the time
being tested, but this actually works out daylight savings for the time
at the epoch (1/1/1970). I've modified it to use DST_OFFSET instead
(note - this may fail if ran during the switchover period).
and finally TimeTest.testGetTimeZone : line 84:
Fails because although the timezone offset is added to local_offset, the
dst isn't taken into account. If DST is in effect the offset is out.
I've corrected this by adding the DST_OFFSET value;
Also i did get an error on MiscTest.testSingleThreadCancel - but i can't
repeat this... Is this expected?
[junit] Testcase:
testSingleThreadCancel(org.postgresql.test.jdbc2.MiscTest)
: Caused an ERROR
[junit] ERROR: canceling statement due to user request
[junit] org.postgresql.util.PSQLException: ERROR: canceling
statement due to
user request
[junit] at
org.postgresql.core.v3.QueryExecutorImpl.receiveErrorResponse
(QueryExecutorImpl.java:1646)
[junit] at
org.postgresql.core.v3.QueryExecutorImpl.processResults(Query
ExecutorImpl.java:1380)
...
Thanks
JOHN
Index: DatabaseMetaDataTest.java
===================================================================
RCS file: /cvsroot/jdbc/pgjdbc/org/postgresql/test/jdbc2/DatabaseMetaDataTest.java,v
retrieving revision 1.44
diff -u -r1.44 DatabaseMetaDataTest.java
--- DatabaseMetaDataTest.java 7 Nov 2008 09:11:37 -0000 1.44
+++ DatabaseMetaDataTest.java 29 Apr 2009 19:44:51 -0000
@@ -99,7 +99,7 @@
assertTrue( "getTables() returned too many rows", rs.next() == false);
rs.close();
- rs = dbmd.getColumns("", "", "test%", "%" );
+ rs = dbmd.getColumns("", "", "testmetadat%", "%" );
assertTrue( rs.next() );
assertEquals( "testmetadata", rs.getString("TABLE_NAME") );
assertEquals( "id", rs.getString("COLUMN_NAME") );
@@ -498,7 +498,7 @@
assertEquals("b", rs.getString(4));
assertTrue(!rs.next());
-
+
rs.close();
}
Index: TimeTest.java
===================================================================
RCS file: /cvsroot/jdbc/pgjdbc/org/postgresql/test/jdbc2/TimeTest.java,v
retrieving revision 1.18
diff -u -r1.18 TimeTest.java
--- TimeTest.java 8 Jan 2008 06:56:31 -0000 1.18
+++ TimeTest.java 30 Apr 2009 10:34:54 -0000
@@ -59,7 +59,7 @@
cal.setTimeZone(TimeZone.getTimeZone("GMT"));
- long localOffset = Calendar.getInstance().get(Calendar.ZONE_OFFSET);
+ long localOffset = Calendar.getInstance().get(Calendar.ZONE_OFFSET) +
Calendar.getInstance().get(Calendar.DST_OFFSET);
/* set the time to midnight to make this easy */
assertEquals(1, stmt.executeUpdate(TestUtil.insertSQL("testtime", "'00:00:00','00:00:00'")));
@@ -266,10 +266,8 @@
assertNotNull(t);
java.sql.Time tmpTime = java.sql.Time.valueOf("5:1:2");
int localoffset = java.util.Calendar.getInstance().getTimeZone().getRawOffset();
- if (java.util.Calendar.getInstance().getTimeZone().inDaylightTime(tmpTime))
- {
- localoffset += 60 * 60 * 1000;
- }
+ localoffset += java.util.Calendar.getInstance().get(Calendar.DST_OFFSET);
+
int Timeoffset = 3 * 60 * 60 * 1000;
tmpTime.setTime(tmpTime.getTime() + Timeoffset + localoffset);
assertEquals(makeTime(tmpTime.getHours(), tmpTime.getMinutes(), tmpTime.getSeconds()), t);
@@ -279,10 +277,7 @@
assertNotNull(t);
tmpTime = java.sql.Time.valueOf("23:59:59");
localoffset = java.util.Calendar.getInstance().getTimeZone().getRawOffset();
- if (java.util.Calendar.getInstance().getTimeZone().inDaylightTime(tmpTime))
- {
- localoffset += 60 * 60 * 1000;
- }
+ localoffset += java.util.Calendar.getInstance().get(Calendar.DST_OFFSET);
Timeoffset = -11 * 60 * 60 * 1000;
tmpTime.setTime(tmpTime.getTime() + Timeoffset + localoffset);
assertEquals(makeTime(tmpTime.getHours(), tmpTime.getMinutes(), tmpTime.getSeconds()), t);
On Fri, 1 May 2009, John Lister wrote: > The first occurs in DatabaseMetaDataTest.testTables line 104: > > It fetches all the columns for any table beginning with test - i happen to > have other tables named test... which causes this to fail. I've modified it > to look for the testmetadata table, alternatively the docs should be altered > to state that the test database should be clean. It is expected that the test database is empty. > The second in TimeTest.timeTest : line 269 & 283: > The timezone tests fail because the as it happens local daylight savings is > in effect, the code tries to test for this by submitting the time being > tested, but this actually works out daylight savings for the time at the > epoch (1/1/1970). I've modified it to use DST_OFFSET instead (note - this > may fail if ran during the switchover period). What timezone are you running the tests in? I don't see them in US/Pacific. Reproducing the failure would help. Kris Jurka
Kris Jurka wrote: > > > On Fri, 1 May 2009, John Lister wrote: > >> The first occurs in DatabaseMetaDataTest.testTables line 104: >> >> It fetches all the columns for any table beginning with test - i >> happen to have other tables named test... which causes this to fail. >> I've modified it to look for the testmetadata table, alternatively >> the docs should be altered to state that the test database should be >> clean. > > It is expected that the test database is empty. OK, maybe we should alter the docs, it only mentions that tables are created/dropped and shouldn't contain production tables, but nothing about it being empty. I'd forgotten i had some other tables in there... > >> The second in TimeTest.timeTest : line 269 & 283: >> The timezone tests fail because the as it happens local daylight >> savings is in effect, the code tries to test for this by submitting >> the time being tested, but this actually works out daylight savings >> for the time at the epoch (1/1/1970). I've modified it to use >> DST_OFFSET instead (note - this may fail if ran during the >> switchover period). > > What timezone are you running the tests in? I don't see them in > US/Pacific. Reproducing the failure would help. > GMT with DST in effect. JOHN
John Lister <john.lister-ps@kickstone.com> writes:
> Kris Jurka wrote:
>> What timezone are you running the tests in?
>>
> GMT with DST in effect.
Isn't that a self-contradictory statement? "GMT" implies a non
daylight savings time.
regards, tom lane
Tom Lane wrote: > John Lister <john.lister-ps@kickstone.com> writes: > >> Kris Jurka wrote: >> >>> What timezone are you running the tests in? >>> >>> >> GMT with DST in effect. >> > > Isn't that a self-contradictory statement? "GMT" implies a non > daylight savings time. > Ok, call it BST then :) I was trying to avoid confusion....
On Sun, 3 May 2009, John Lister wrote: > Ok, call it BST then :) I was trying to avoid confusion.... > OK, I get the failures if I set my timezone to 'Europe/London', but with your patch applied I get the same failure setting my timezone to 'US/Pacific'. I haven't looked to see exactly what's going on, but your patch isn't completely right. Can you get something to work for both of these cases? Kris Jurka
Kris Jurka wrote: > OK, I get the failures if I set my timezone to 'Europe/London', but > with your patch applied I get the same failure setting my timezone to > 'US/Pacific'. I haven't looked to see exactly what's going on, but > your patch isn't completely right. Can you get something to work for > both of these cases? I've just noticed at the top of the timezone test that it sets the default to be GMT+1 (which happens to be BST). I suspect something wierd is going on with this default and the actual timezone. I'll investigate... JOHN
Kris Jurka wrote: > > > On Sun, 3 May 2009, John Lister wrote: > >> Ok, call it BST then :) I was trying to avoid confusion.... >> > > OK, I get the failures if I set my timezone to 'Europe/London', but > with your patch applied I get the same failure setting my timezone to > 'US/Pacific'. I haven't looked to see exactly what's going on, but > your patch isn't completely right. Can you get something to work for > both of these cases? Ignore my previous email, i was looking at the Timezone test, the problem occurs with the time Test... JOHN
Kris Jurka wrote:
>
>
> On Sun, 3 May 2009, John Lister wrote:
>
>> Ok, call it BST then :) I was trying to avoid confusion....
>>
>
> OK, I get the failures if I set my timezone to 'Europe/London', but
> with your patch applied I get the same failure setting my timezone to
> 'US/Pacific'. I haven't looked to see exactly what's going on, but
> your patch isn't completely right. Can you get something to work for
> both of these cases?
>
OK, i've done some more testing and i'd be grateful if anyone can repeat
this or tell me the fundamental flaw in my logic.... I don't think it
has anything to do with the driver and apologies as i realise that this
is more of a straight java question.. I've rebooted between tests after
changing the timezone, etc to see if that had any effect - which it didn't
Windows XP SP3 - JDK 1.6.0.13.
for all of these i do this first:
Time t=new Time(28862000); // this should be 8:01:02 UTC
Now taking the string of t using toString() i get the following (when
dst is not active i've simply shifted the clock forward 6 months)
CET (GMT+1, DST auto-adjust enabled and not active) - 09:01:02
- This is ok...
CEST (GMT +1, DST auto-adjust enabled and active) - 09:01:02
- Actually Correct as the date part is 1/1/1970 which isn't DST
CET (GMT+1, DST auto-adjust disabled) - 09:01:02
- Again this is ok...
(same for other time zones)
Now the odd bit
Europe/London (DST auto-adjust enabled but not active) - 09:01:02 -
This is wrong, it should be equiv to GMT
Europe/London (DST auto-adjust enabled and active) - 09:01:02 -
This is wrong (DST shouldn't matter as the date part is 1/1/1970)
Europe/London (DST auto-adjust disabled) - 08:01:02
- ok and just GMT as we'd expect
Am i going mad - its been a long day or is something wierd going on?
This is what is causing the problem, the driver sets the time using
toString() as above and inserts the wrong time into the db. BTW the
check for DST when reading back the data in the test is superfluous as
the date associated with the time is always 1/1/1970 as i originally
spotted except I incorrectly assumed the current DST was applied which
it shouldn't be...
Thanks
JOHN
John Lister wrote: > for all of these i do this first: > Time t=new Time(28862000); // this should be 8:01:02 UTC > > Now the odd bit > Europe/London (DST auto-adjust enabled but not active) - 09:01:02 - > This is wrong, it should be equiv to GMT Yes, but not at the unix epoch. See the section titled "Permanent summer, 1968–1971" http://www.nmm.ac.uk/explore/astronomy-and-time/time-facts/british-summer-time/
Kris Jurka wrote: > John Lister wrote: > >> for all of these i do this first: >> Time t=new Time(28862000); // this should be 8:01:02 UTC >> >> Now the odd bit >> Europe/London (DST auto-adjust enabled but not active) - 09:01:02 >> - This is wrong, it should be equiv to GMT > > Yes, but not at the unix epoch. See the section titled "Permanent > summer, 1968–1971" > > http://www.nmm.ac.uk/explore/astronomy-and-time/time-facts/british-summer-time/ > I assumed that something was going on around the epoch, but didn't realise that - amazing what you learn about your own country. Still there is bizarre behaviour with the java time stuff (nothing new there then...) Assuming timezone=Europe/London then TimeZone t=TimeZone.getDefault(); t.inDaylightTime(new Date(28862000L)) == false t.getRawOffset() == 0 t.getOffset(28862000L) == 3600000 It would seem that inDaylightTime doesn't take into account historical stuff... Anyway patch attached that fixes the problem for all (tested) timezones.. Can you let me know how you get on... Also, for the failing test - I'm fairly sure the driver shouldn't be inserting "9:01:02" for the value of "5:1:2+03" when the current timezone is "Europe/London" without DST - as it uses the epoch to calculate the offsets which while historically correct are now incorrect. Note that setting it to simply GMT works as expected. I suspect that the date component should be initialised to the current date instead of 1/1/1970 for internal manipulation within the driver and reset before returning anything to the user as a sql.Time value. Thoughts JOHN Index: TimeTest.java =================================================================== RCS file: /cvsroot/jdbc/pgjdbc/org/postgresql/test/jdbc2/TimeTest.java,v retrieving revision 1.18 diff -u -r1.18 TimeTest.java --- TimeTest.java 8 Jan 2008 06:56:31 -0000 1.18 +++ TimeTest.java 7 May 2009 19:07:16 -0000 @@ -59,9 +59,9 @@ cal.setTimeZone(TimeZone.getTimeZone("GMT")); - long localOffset = Calendar.getInstance().get(Calendar.ZONE_OFFSET); + int localOffset=Calendar.getInstance().getTimeZone().getOffset(midnight.getTime()); - /* set the time to midnight to make this easy */ + // set the time to midnight to make this easy assertEquals(1, stmt.executeUpdate(TestUtil.insertSQL("testtime", "'00:00:00','00:00:00'"))); assertEquals(1, stmt.executeUpdate(TestUtil.insertSQL("testtime", "'00:00:00.1','00:00:00.01'"))); assertEquals(1, stmt.executeUpdate(TestUtil.insertSQL("testtime", "CAST(CAST(now() AS timestamp without time zone)AS time),now()"))); @@ -265,11 +265,7 @@ t = rs.getTime(1); assertNotNull(t); java.sql.Time tmpTime = java.sql.Time.valueOf("5:1:2"); - int localoffset = java.util.Calendar.getInstance().getTimeZone().getRawOffset(); - if (java.util.Calendar.getInstance().getTimeZone().inDaylightTime(tmpTime)) - { - localoffset += 60 * 60 * 1000; - } + int localoffset=java.util.Calendar.getInstance().getTimeZone().getOffset(tmpTime.getTime()); int Timeoffset = 3 * 60 * 60 * 1000; tmpTime.setTime(tmpTime.getTime() + Timeoffset + localoffset); assertEquals(makeTime(tmpTime.getHours(), tmpTime.getMinutes(), tmpTime.getSeconds()), t); @@ -278,11 +274,7 @@ t = rs.getTime(1); assertNotNull(t); tmpTime = java.sql.Time.valueOf("23:59:59"); - localoffset = java.util.Calendar.getInstance().getTimeZone().getRawOffset(); - if (java.util.Calendar.getInstance().getTimeZone().inDaylightTime(tmpTime)) - { - localoffset += 60 * 60 * 1000; - } + localoffset=java.util.Calendar.getInstance().getTimeZone().getOffset(tmpTime.getTime()); Timeoffset = -11 * 60 * 60 * 1000; tmpTime.setTime(tmpTime.getTime() + Timeoffset + localoffset); assertEquals(makeTime(tmpTime.getHours(), tmpTime.getMinutes(), tmpTime.getSeconds()), t);
On Thu, 7 May 2009, John Lister wrote: > It would seem that inDaylightTime doesn't take into account historical > stuff... > > Anyway patch attached that fixes the problem for all (tested) > timezones.. Can you let me know how you get on... Looks good to me. Applied back to 8.1. 8.0 has some other time test problems that look like they've been there forever. Not backpatching as I don't fully understand what's going on there and this is just a test case fix. Kris Jurka
Kris Jurka wrote: > Looks good to me. Applied back to 8.1. 8.0 has some other time test > problems that look like they've been there forever. Not backpatching > as I don't fully understand what's going on there and this is just a > test case fix. > Cheers. I'm still not happy about changing the test case to work in europe/london. I did try playing with the time setting functions to avoid using the epoch as the base for times so that they were based on the current date thus avoiding any historical DST or other zone issues - but it broke a lot of the other tests which expected the epoch as the base. I guess this is a fundamental flaw in the java.sql.Time class (which has many other problems). I don't use TIME separately (only TIMESTAMP) so this isn't a huge deal for me, but this may bite someone else. JOHN
> I guess this is a fundamental flaw in the java.sql.Time class (which > has many other problems). I don't use TIME separately (only TIMESTAMP) > so this isn't a huge deal for me, but this may bite someone else. Should have also said that i can't see a nice way of fixing it as i suspect both sql time and java time weren't originally designed to deal with timestamps and that there is unlikely a nice fix for all cases... JOHN
Here are my results after experimenting on refreshRow with the standard JDBC and a modified (and much faster) version
whichuses the names returned by the resultset (called "labels") instead of going to the catalog to get the actual
columnname (a very slow process).
I tested three resultsets: straight column names, an alias that merely renames a column, and an alias that names an
expressionresult.
Both versions handle straight column names but the modified version fails when a column is renamed.
This is interesting: Both fail if there's an expression.
Create table
create table testdb (
recno serial primary key,
name varchar,
sum decimal,
count decimal);
Load/reload table
delete from testdb;
insert into testdb(name,sum,count) values ('Able',100,5);
insert into testdb(name,sum,count) values ('Baker',200,3);
insert into testdb(name,sum,count) values ('Caty',50,2);
The experiment
Create a resultset using each of these three queries
1 select name, sum, recno from testdb
2 select name, sum as sumalias, recno from testdb
3 select name, sum / count as avg, recno from testdb
For each, position at the second row (Baker) and update the name,
then try to refresh the row and see what happens.
Standard JDBC's refreshRow does this for each column
selectSQL.append( fields[i].getColumnName(connection) );
and the modified JDBC does this
selectSQL.append( fields[i].getColumnLabel() );
Experimental results for the three resultsets.
1 Both versions of JDBC did the same thing with straight column names.
2 The modified version failed because sumalias is not a column name.
3 Both failed because of the expression.
Discussion
Using the modified version is essentially as good as the standard version
if one doesn't use aliases just to rename columns (without expressions).
Eg if you do select * from table you're ok.
Both fail with expressions, so the standard JDBC is no better than the
modified JDBC.
Question
Isn't the best solution to reuse the original query instead of either the
column name or label? That would deliver the average as was intended by
the original select.
John