Testing is broken [#840182]

quicksketch reports http://skitch.com/quicksketch/dkefx/php-stack-trace

I report that when nextIdDelete fires (shutdown function) from includes/database/mysql/database.inc after a test class is finished in the browser then prefixTables sees the simpletest prefix. Here is a dump once _simpletest_batch_operation is finished from prefixTables (i removed a bunch of newlines from the SQL queries to shorten it:

default prefix 
SELECT 1 AS expression FROM {sessions} sessions WHERE  (sid = :db_condition_placeholder_0)  FOR UPDATE
default prefix 
UPDATE {sessions} SET uid=:db_update_placeholder_0, cache=:db_update_placeholder_1, hostname=:db_update_placeholder_2, session=:db_update_placeholder_3, timestamp=:db_update_placeholder_4 WHERE  (sid = :db_condition_placeholder_0) 
default prefix 
SELECT data, created, expire, serialized FROM {cache_bootstrap} WHERE cid = :cid
default prefix 
SELECT 1 AS expression FROM  {cache_bootstrap} cache_bootstrap WHERE  (cid = :db_condition_placeholder_0)  FOR UPDATE
default prefix 
UPDATE {cache_bootstrap} SET serialized=:db_update_placeholder_0, created=:db_update_placeholder_1, expire=:db_update_placeholder_2, data=:db_update_placeholder_3 WHERE  (cid = :db_condition_placeholder_0) 
default prefix 
UPDATE {batch} SET batch=:db_update_placeholder_0 WHERE  (bid = :db_condition_placeholder_0) 
default prefix simpletest652543
SELECT MAX(value) FROM {sequences}

That's a db_merge from sessions, then a cache_set for cache_bootstrap, a batch refresh and then nextIdDelete. The right question to ask is why this function fires? It only got called during system install and we presumedly restore the shutdown functions. A dump of drupal_register_shutdown_function does not show nextIdDelete however.

Comment	File	Size	Author
#25	graham_number_of_comments.patch	1.92 KB	chx
#25
#23	graham_number_of_comments2.patch	2.08 KB	Berdir
#23
#22	graham_number_of_comments.patch	2.01 KB	chx
#22
#15	ignore_errors.patch	1.89 KB	chx
#15
#12	ignore_errors.patch	1.78 KB	chx
#12
#10	no_nextiddelete_testin.patch	902 bytes	chx
#10
#1	uglylittlebug.patch	624 bytes	chx
#1
	uglylittlebug.patch	1.7 KB	chx

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

chx CreditAttribution: chx commented 29 June 2010 at 06:33

File	Size
uglylittlebug.patch	624 bytes

With less debug.

Comment #2

catch

he/him

English

CreditAttribution: catch commented 29 June 2010 at 06:35

Status:

Needs review

» Reviewed & tested by the community

ouch.

Comment #3

chx CreditAttribution: chx commented 29 June 2010 at 06:46

Filed #840194: We still do not catch all exceptions separately.

Comment #4

Dries CreditAttribution: Dries commented 29 June 2010 at 13:43

Status:

Reviewed & tested by the community

» Fixed

Good catch. Committed to CVS HEAD. Thanks!

Comment #5

Berdir

German

Switzerland

CreditAttribution: Berdir commented 29 June 2010 at 14:22

We (I, requested by Crell) did not use drupal_register_shutdown_function() on purpose to avoid adding more dependencies. But as this has shown, it's needed, so...

Comment #6

Crell CreditAttribution: Crell commented 29 June 2010 at 15:17

Status:

Fixed

» Needs work

As Berdir notes, not using drupal_register_shutdown_function() there was very deliberate. chx, from your description I don't quite follow why we have to use it here. That's another hard dependency between subsystems that we have gone to great lengths to avoid.

Comment #7

Berdir

German

Switzerland

CreditAttribution: Berdir commented 29 June 2010 at 15:23

Status:

Needs work

» Fixed

The fix is correct (unfortunately ;)).

We clear the shutdown functions after a test run has finished so that they don't run in the original environment (and database)*. We can only do that with functions registered with drupal_register_shutdown_function() as we don't have any influence on those registered with register_shutdown_function().

* I just learned that in this issue, this was new to me too but makes sense. If that wouldn't be the case, we could use register_shutdown_function() and use try/catch to handle any exceptions.

Edit: I wonder if Crell is automatically subscribed to an issue when someone writes down his nickname in a comment... ;)

Comment #8

Damien Tournoud CreditAttribution: Damien Tournoud commented 29 June 2010 at 16:07

Status:

Fixed

» Needs work

No sorry, the fix is wrong.

It is by design that we are getting a prefixed database here. We just need to ignore the exceptions that might be thrown during the cleanup process.

Comment #9

Berdir

German

Switzerland

CreditAttribution: Berdir commented 29 June 2010 at 16:44

Yes, the prefixed database is by design.

However, the query should never run because it is supposed to be run in the test database and simpletest is responsible for cleaning up the shutdown functions. It can do that only if drupal_register_shutdown_function() is used so I don't see what's wrong with the fix.

In theory, to fully simulate an installation, simpletest could execute the shutdown functions *before* the test database is deleted but it doesn't matter since the whole install is deleted anyway.

That we get these unholy "exception thrown without stackstrace" errors again/still is a different issue and looked at in the other issue.

Comment #10

chx CreditAttribution: chx commented 29 June 2010 at 18:28

Status:

Needs work

» Needs review

File	Size
no_nextiddelete_testin.patch	902 bytes

Hrm.

Comment #11

Crell CreditAttribution: Crell commented 29 June 2010 at 18:32

Status:

Needs review

» Needs work

I think I still prefer DamZ's suggestion in #8. If the cleanup query fails there's no serious harm, it will only happen if the DB goes away mid-process which only happens during simpletest, and it introduces no dependencies, hard or soft, on the Drupal layers above it.

Comment #12

chx CreditAttribution: chx commented 29 June 2010 at 18:38

Status:

Needs work

» Needs review

File	Size
ignore_errors.patch	1.78 KB

Sure thing. I wrote this patch too before posting the other. :)

Comment #13

Dries CreditAttribution: Dries commented 29 June 2010 at 18:49

Oopsie! Glad we're fixing it already. :)

+++ includes/database/mysql/database.inc	2010-06-29 18:38:02 +0000
@@ -110,10 +110,18 @@ class DatabaseConnection_mysql extends D
+    // During testing, this function is called from shutdown with the
+    // wrong prefix so we need to ignore the database errors. There is no

Could we mention why it is called with the wrong prefix?

Comment #14

Crell CreditAttribution: Crell commented 29 June 2010 at 19:06

Status:

Needs review

» Needs work

There should also be a period, not a comma, in "ignoring errors here, if these queries".

Other than that and Dries' comment comment (is that a meta comment), I'm good with #12.

Comment #15

chx CreditAttribution: chx commented 29 June 2010 at 19:06

Status:

Needs work

» Needs review

File	Size
ignore_errors.patch	1.89 KB

No :D good god. that's difficult to explain.

Comment #16

mcrittenden CreditAttribution: mcrittenden commented 29 June 2010 at 20:17

Status:

Needs review

» Needs work

and those table

Should be tables.

shutdown is called  so we need

Two spaces between "called" and "so"

There is no problem with completely ignoring errors here, if
+    // these queries fail, the sequence will work just fine just take a bit
+    // more space.

Not a proper sentence.

Comment #17

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 29 June 2010 at 20:26

While we're at it, let's comment

-      drupal_register_shutdown_function(array($this, 'nextIdDelete'));
+      register_shutdown_function(array($this, 'nextIdDelete'));

this change to indicate that we're intentionally not going through drupal_register_shutdown_function(), so that someone later on doesn't try and "fix" this.

Comment #18

carlos8f CreditAttribution: carlos8f commented 29 June 2010 at 21:36

@mcrittenden

Should be tables.

A literal reference should be surrounded in quotes: "tables".

Two spaces between "called" and "so"

This needs a period at the end.

Not a proper sentence.

"Not a proper sentence." is not a proper sentence :)

Comment #19

Damien Tournoud CreditAttribution: Damien Tournoud commented 29 June 2010 at 21:45

I do not comment on meta-comments.

That was a meta-comment.

Comment #20

carlos8f CreditAttribution: carlos8f commented 29 June 2010 at 21:56

#18 implements hook_humor()

Comment #21

Crell CreditAttribution: Crell commented 29 June 2010 at 22:56

I will refrain from commenting on DamZ's lack of comment about carlos8f's comment regarding mcrittenden's comments about the code comments in the latest patch.

Comment #22

chx CreditAttribution: chx commented 30 June 2010 at 05:52

Status:

Needs work

» Needs review

File	Size
graham_number_of_comments.patch	2.01 KB

Comment #23

Berdir

German

Switzerland

CreditAttribution: Berdir commented 30 June 2010 at 06:06

File	Size
graham_number_of_comments2.patch	2.08 KB

Removed a trailing whitespace and added () to the function in the comment.

Comment #24

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 30 June 2010 at 06:59

+++ includes/database/mysql/database.inc
@@ -111,9 +113,19 @@ class DatabaseConnection_mysql extends DatabaseConnection {
+      // We know we are using MySQL here, so need for the slower db_delete().
+      $this->query('DELETE FROM {sequences} WHERE value < :value', array(':value' => $max_id));

need -> no need
I don't want to start another commenting avalanche, but this typo is really confusing.

Leaving at needs review for more reviews (...), but the next reroll should fix that.

Powered by Dreditor.

Comment #25

chx CreditAttribution: chx commented 30 June 2010 at 15:37

File	Size
graham_number_of_comments.patch	1.92 KB

you surely are aware that was there in HEAD and I just indented it?

Comment #26

Crell CreditAttribution: Crell commented 30 June 2010 at 15:58

Status:

Needs review

» Reviewed & tested by the community

Quick, before someone else comments!

Comment #27

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 30 June 2010 at 16:39

No I wasn't. Dreditor makes it too easy to ignore the deleted lines...
+1 on the RTBC as someone without deeper insight into either the DB-system or SImpleTest (=== 'me') can grasp the patch and its reasoning from the inline comments.