simpletest throws exceptions on pgsql [#257009]

It appears that the core simpletest framework re-runs the install process with a $db_prefix. On pgsql, we do CREATE DOMAIN {,small,big}int_unsigned CHECK (VALUE >= 0) in system.install. These have a single global name so re-running the install generates "already exists" errors.

There are a variety of ways out of this gotcha. My suggestion is http://drupal.org/node/257006. I do not think these are duplicate issues (this one is about unit tests throwing exceptions, that one is about unifying an inconsistency between how int, float, and numeric unsigned columns are handled), though my proposed solution conveniently fixes both.

Comment	File	Size	Author
#15	system.install-257009-15_D6.patch	1.79 KB	Darren Oh
#15
#12	simpletest-pgsql-create-domain-257009-12.patch	1.67 KB	mustafau
#12
#9	simpletest-pgsql-create-domain-257009-9.patch	1.74 KB	bjaspan
#9
#2	simpletest-pgsql-create-domain-257009-2.patch	1.41 KB	Freso
#2
#1	simpletest-pgsql-create-domain-257009-1.patch	1.43 KB	bjaspan
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

bjaspan CreditAttribution: bjaspan commented 10 May 2008 at 20:31

Status:

Active

» Needs review

File	Size
simpletest-pgsql-create-domain-257009-1.patch	1.43 KB

Actually, I decided to fix this more expediently by simply not trying to create the unsigned domains if they already exist. This gives a very short patch that makes the simpletests stop throwing exceptions on pgsql. This fix does not prevent the more comprehensive one contemplated in http://drupal.org/node/257006, but certainly lessens my motivation to take on the complex update issues. :-)

To test this patch, run any tests on pgsql. If they pass without exceptions, the patch works.

Comment #2

Freso CreditAttribution: Freso commented 8 June 2008 at 15:06

Component:

unit testing

» tests

File	Size
simpletest-pgsql-create-domain-257009-2.patch	1.41 KB

For some reason, the patch didn't apply:

freso@nayru /s/h/l/h/drupal7> wget http://drupal.org/files/issues/simpletest-pgsql-create-domain-257009-1.p...
[...]
freso@nayru /s/h/l/h/drupal7> patch -p0 < simpletest-pgsql-create-domain-257009-1.patch
patching file modules/system/system.install
Hunk #1 FAILED at 305.
1 out of 1 hunk FAILED -- saving rejects to file modules/system/system.install.rej

The change was easy to do myself, so I did a re-roll using cvs diff -up. Reviewing now.

Comment #3

Freso CreditAttribution: Freso commented 8 June 2008 at 15:23

I tested it against #264836: Bike shed test fails (PostgreSQL?), which removed the PHP exceptions there. So it works as advertised. :)

However, it being in system.install... won't that mean that old sites upgrading to Drupal 7 won't get the benefit of this? Just curious, as that wouldn't be something we'd want... right?

Comment #4

bjaspan CreditAttribution: bjaspan commented 9 June 2008 at 16:02

This patch does not affect upgrade from pre-D7 sites. Upgrading does not run system_install(), so the pgsql domains are not re-created.

Comment #5

Freso CreditAttribution: Freso commented 10 June 2008 at 11:48

Status:

Needs review

» Needs work

Well, I'd say it shouldn't go into CVS until that is resolved.

Comment #6

bjaspan CreditAttribution: bjaspan commented 10 June 2008 at 14:13

Status:

Needs work

» Needs review

@Freso: You have not actually presented a problem with the current patch. The upgrade process from pre-D7 is not affected by the bug I reported and is not affected by the patch I've presented, so there is nothing to resolve regarding upgrading from pre-D7 sites pertaining to this bug before the patch can be committed.

I'm putting this back to "code needs review" so I do not forget to do so later. However, if you can describe an actual problem with the patch, by all means do so. Thanks!

Comment #7

Freso CreditAttribution: Freso commented 10 June 2008 at 15:15

@bjaspan: The perceived problem (which I thought you confirmed #4) is that sites upgrading from 6.x will still have tests fail on pgsql systems, as the pgsql domains are not re-created as you put it. I haven't tested it, so can't say if this is really so, but, yeah. I thought you confirmed this in #4 - sorry to put words in your mouth if you didn't. :)

Comment #8

catch

he/him

English

CreditAttribution: catch commented 10 June 2008 at 15:22

I think the issue is that simpletest.module is running that hunk of code for a second time - which would otherwise never happen - so the changes only exist to accommodate this behaviour, and therefore only in D7 (afaik the D6 contrib verson of simpletest doesn't do this either). Since this isn't immediately obvious (and I might not have it right anyway) a code comment might help.