Re: 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

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(dot)lister-ps(at)kickstone(dot)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(dot)lister-ps(at)kickstone(dot)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

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:
>
>
> 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

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

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 which uses the names returned by the resultset (called "labels") instead of going to the catalog to get the actual column name (a very slow process).

I tested three resultsets: straight column names, an alias that merely renames a column, and an alias that names an expression result.

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