Thread: Tests citext casts

Tests citext casts

From
Kenneth Marshall
Date:
I installed and ran the citext tests both with and without
the patch and had failures both times. The patch applied
cleanly and the "make;make install" completed without errors.
I have attached the two regression.diffs files, one without
the patch applied and the other with the patch.

Regards,
Ken Marshall

Attachment

Re: Tests citext casts

From
"David E. Wheeler"
Date:
On Nov 5, 2008, at 6:40 AM, Kenneth Marshall wrote:

> I installed and ran the citext tests both with and without
> the patch and had failures both times. The patch applied
> cleanly and the "make;make install" completed without errors.
> I have attached the two regression.diffs files, one without
> the patch applied and the other with the patch.

What patch was it you applied? And is this CVS HEAD that you're
testing? What locale/collation is your database configured with?

Thanks,

David

Re: [RRR] Tests citext casts

From
Kenneth Marshall
Date:
On Wed, Nov 05, 2008 at 09:04:04AM -0800, David E. Wheeler wrote:
> On Nov 5, 2008, at 6:40 AM, Kenneth Marshall wrote:
>
>> I installed and ran the citext tests both with and without
>> the patch and had failures both times. The patch applied
>> cleanly and the "make;make install" completed without errors.
>> I have attached the two regression.diffs files, one without
>> the patch applied and the other with the patch.
>
> What patch was it you applied? And is this CVS HEAD that you're testing? 
> What locale/collation is your database configured with?
>
> Thanks,
>
> David
>
David,

I am using the anonymous CVS repository, it returns the following
information in pg_catalog.pg_settings:

postgres=# select * from pg_catalog.pg_settings where name like 'server_%';       name        | setting  | unit |
              category                     |                     short_desc                     | extra_desc | c
 
ontext  | vartype |  source  | min_val | max_val | enumvals | boot_val  | reset_
val | sourcefile | sourceline 
--------------------+----------+------+-----------------------------------------
-----------+----------------------------------------------------+------------+--
--------+---------+----------+---------+---------+----------+-----------+-------
----+------------+------------server_encoding    | UTF8     |      | Client Connection Defaults / Locale and 
Formatting | Sets the server (database) character set encoding. |            | i
nternal | string  | override |         |         |          | SQL_ASCII | UTF8     |            |
server_version    | 8.4devel |      | Preset Options                                    | Shows the server version.
                    |            | i
 
nternal | string  | default  |         |         |          | 8.4devel  | 8.4dev
el  |            |           server_version_num | 80400    |      | Preset Options                                    |
Showsthe server version as an integer.            |            | i
 
nternal | integer | default  | 80400   | 80400   |          | 80400     | 80400    |            |           
(3 rows)

The patch that I used is from the link in the commitfest 2008-11 wiki
which points to:

http://archives.postgresql.org/message-id/0BD2E4E9-AF5A-4B4F-B546-027424EE8FAD@kineticode.com

Cheers,
Ken


Re: [RRR] Tests citext casts

From
"David E. Wheeler"
Date:
On Nov 5, 2008, at 12:34 PM, Kenneth Marshall wrote:

> I am using the anonymous CVS repository, it returns the following
> information in pg_catalog.pg_settings:

What is lc_collate set to?

% show lc_collate;

FWIW, I just ran the tests myself and all passed, with and without the  
patch (using en_US.UTF-8). I think that the regression tests generally  
expect to be run with the C locale, though en_US generally works fine,  
too, given that ASCII ordering has the same semantics.

Best,

David


Re: [RRR] Tests citext casts

From
Kenneth Marshall
Date:
On Fri, Nov 07, 2008 at 10:15:17AM -0800, David E. Wheeler wrote:
> On Nov 5, 2008, at 12:34 PM, Kenneth Marshall wrote:
>
>> I am using the anonymous CVS repository, it returns the following
>> information in pg_catalog.pg_settings:
>
> What is lc_collate set to?
>
> % show lc_collate;
>
> FWIW, I just ran the tests myself and all passed, with and without the 
> patch (using en_US.UTF-8). I think that the regression tests generally 
> expect to be run with the C locale, though en_US generally works fine, too, 
> given that ASCII ordering has the same semantics.
>
> Best,
>
> David
>
David,

Thank you for the pointers. lc_collate is set to en_US.UTF-8. I
re-initdb the database with the --no-locale option and then the
tests passed successfully. Thank you for the reminder that the
regression tests need to run against a C locale database.

Regards,
Ken


Re: [RRR] Tests citext casts

From
"David E. Wheeler"
Date:
On Nov 7, 2008, at 10:43 AM, Kenneth Marshall wrote:

> Thank you for the pointers. lc_collate is set to en_US.UTF-8.

Huh. Same as for me.

> I re-initdb the database with the --no-locale option and then the
> tests passed successfully. Thank you for the reminder that the
> regression tests need to run against a C locale database.

Great, thank you!

David



Re: [RRR] Tests citext casts - reviewed

From
Kenneth Marshall
Date:
The patch for the citext tests applied to module cleanly
and the patched files resulted in a clean "make installcheck"
run for the citext module. My previous problem was the result
of not testing with a C locale database. This patch is ready
to be applied.

Regards,
Ken Marshall


Re: [RRR] Tests citext casts

From
Tom Lane
Date:
Kenneth Marshall <ktm@rice.edu> writes:
> Thank you for the pointers. lc_collate is set to en_US.UTF-8. I
> re-initdb the database with the --no-locale option and then the
> tests passed successfully. Thank you for the reminder that the
> regression tests need to run against a C locale database.

Actually, they don't expect that; all of the core regression tests pass
under multiple locales.  (I think there's even one variant file that's
there specifically to make 'em pass in sv_SE ...)  The reason for taking
trouble over this is that "make installcheck" tests against an installed
server, which quite likely isn't using C locale.  Since the contrib
modules are *only* testable in "make installcheck" fashion, this is
actually a bigger consideration for them than for the core tests.

In a quick test on a Fedora box, citext is the only core or contrib test
that fails in en_US.  (This is true in HEAD, even without having applied
the proposed patch.)  It would be good to clean that up.

We could fix it by having multiple variant expected files for C and
non-C locales, which is exactly what the core tests do.  However,
I'm loath to apply that approach when the citext test already has XML vs
no-XML variants; we would then need two variant files per locale
variant, which is a bit unreasonable from a maintenance standpoint.

My inclination is to remove the XML-dependent citext tests, which don't
seem especially useful, and then we can have whatever variants we need
for locales.  citext locale behavior seems much more interesting than
testing whether it casts to xml or not.
        regards, tom lane


Re: [RRR] Tests citext casts

From
"David E. Wheeler"
Date:
On Nov 7, 2008, at 11:15 AM, Tom Lane wrote:

> In a quick test on a Fedora box, citext is the only core or contrib  
> test
> that fails in en_US.  (This is true in HEAD, even without having  
> applied
> the proposed patch.)  It would be good to clean that up.

Huh. There must be something different about the collation for en_US  
on Fedora than there is for darwin (what I'm using), because for me,  
as I said, all tests pass. It's just ASCII, though, so I don't know  
why it would be any different.

> We could fix it by having multiple variant expected files for C and
> non-C locales, which is exactly what the core tests do.  However,
> I'm loath to apply that approach when the citext test already has  
> XML vs
> no-XML variants; we would then need two variant files per locale
> variant, which is a bit unreasonable from a maintenance standpoint.

This is why I like TAP.

> My inclination is to remove the XML-dependent citext tests, which  
> don't
> seem especially useful, and then we can have whatever variants we need
> for locales.  citext locale behavior seems much more interesting than
> testing whether it casts to xml or not.

Agreed, but I admit to being mystified as to why things would be  
sorting any differently on darwin vs. Fedora. I kept everything in  
ASCII, on your advice, to keep from having to deal with crap like this.

Best,

David



Re: [RRR] Tests citext casts

From
Tom Lane
Date:
"David E. Wheeler" <david@kineticode.com> writes:
> Huh. There must be something different about the collation for en_US  
> on Fedora than there is for darwin (what I'm using), because for me,  
> as I said, all tests pass.

Yeah, Darwin seems to just use ASCII sort order in en_US (couldn't say
about its other locales).  glibc-based systems definitely don't though.

>> We could fix it by having multiple variant expected files for C and
>> non-C locales, which is exactly what the core tests do.  However,
>> I'm loath to apply that approach when the citext test already has  
>> XML vs no-XML variants; we would then need two variant files per locale
>> variant, which is a bit unreasonable from a maintenance standpoint.

> This is why I like TAP.

And how would TAP reduce the number of expected results?
        regards, tom lane


Re: [RRR] Tests citext casts

From
"David E. Wheeler"
Date:
On Nov 7, 2008, at 11:50 AM, Tom Lane wrote:

>> This is why I like TAP.
>
> And how would TAP reduce the number of expected results?

TAP doesn't compare output to expected output files. It's simply a  
test result output stream. A separate program then harnesses that  
output, looks at what passed and what failed, and emits a report. So  
you only have to maintain one file of tests. It makes test-driven  
development a lot simpler, not to mention enabling better conditional  
testing, TODO tests, skipping tests, etc.

Best,

David



Re: [RRR] Tests citext casts

From
Tom Lane
Date:
"David E. Wheeler" <david@kineticode.com> writes:
> On Nov 7, 2008, at 11:50 AM, Tom Lane wrote:
>> And how would TAP reduce the number of expected results?

> TAP doesn't compare output to expected output files. It's simply a  
> test result output stream. A separate program then harnesses that  
> output, looks at what passed and what failed, and emits a report. So  
> you only have to maintain one file of tests.

... and you have very limited visibility into what went wrong, if
anything goes wrong.  That's not real attractive for the buildfarm
environment.  I like being able to see the actual query output.
        regards, tom lane


Re: [RRR] Tests citext casts

From
"David E. Wheeler"
Date:
On Nov 7, 2008, at 12:12 PM, Tom Lane wrote:

> ... and you have very limited visibility into what went wrong, if
> anything goes wrong.  That's not real attractive for the buildfarm
> environment.  I like being able to see the actual query output.

It depends on how you write it - you can add a lot of descriptive  
information about what's being tested in each assertion. To me, the  
results are the most important, and I can look in the test file for  
the troubling code.

Anyway, we all like what we're used to, I guess.

Best,

David



Re: [RRR] Tests citext casts

From
Tom Lane
Date:
"David E. Wheeler" <david@kineticode.com> writes:
> On Nov 7, 2008, at 11:15 AM, Tom Lane wrote:
>> My inclination is to remove the XML-dependent citext tests, which don't
>> seem especially useful, and then we can have whatever variants we need
>> for locales.  citext locale behavior seems much more interesting than
>> testing whether it casts to xml or not.

> Agreed, but I admit to being mystified as to why things would be  
> sorting any differently on darwin vs. Fedora. I kept everything in  
> ASCII, on your advice, to keep from having to deal with crap like this.

Patch applied with this adjustment.
        regards, tom lane


Re: [RRR] Tests citext casts

From
"David E. Wheeler"
Date:
On Nov 7, 2008, at 3:18 PM, Tom Lane wrote:

>> Agreed, but I admit to being mystified as to why things would be
>> sorting any differently on darwin vs. Fedora. I kept everything in
>> ASCII, on your advice, to keep from having to deal with crap like  
>> this.
>
> Patch applied with this adjustment.

Great. So does it now pass all tests on Fedora?

Thanks,

David


Re: [RRR] Tests citext casts

From
Tom Lane
Date:
"David E. Wheeler" <david@kineticode.com> writes:
> On Nov 7, 2008, at 3:18 PM, Tom Lane wrote:
>> Patch applied with this adjustment.

> Great. So does it now pass all tests on Fedora?

Yeah, I checked with both C and en_US.utf8 locales.  It's likely that
these two cases will cover them all, though I was too lazy to check...
        regards, tom lane