simpletest throws exceptions on pgsql
bjaspan - May 10, 2008 - 19:42
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | system.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
Description
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.
#1
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.
#2
For some reason, the patch didn't apply:
The change was easy to do myself, so I did a re-roll using
cvs diff -up. Reviewing now.#3
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?
#4
This patch does not affect upgrade from pre-D7 sites. Upgrading does not run system_install(), so the pgsql domains are not re-created.
#5
Well, I'd say it shouldn't go into CVS until that is resolved.
#6
@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!
#7
@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 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. :)
#8
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.
#9
Comment added.
#10
#9 eliminates exceptions on pgsql.
#11
There's a line with excessive whitespace introduced after the new comment lines. Looks okay apart from this. I didn't test.
#12
Re-roll without whitespace.
#13
I've extended the documentation a bit and committed this patch to CVS HEAD. Thanks everyone.
#14
Automatically closed -- issue fixed for two weeks with no activity.
#15
Drupal 6 needs this, too.
#16
#17
Looks like you've even included Dries' comment changes. Looks good, committed.
#18
Automatically closed -- issue fixed for two weeks with no activity.