As part of the effort to test multiple environments, we need to make sure that we can perform SQLite testing concurrently. That's currently not possible because SQLite locks the whole database when it needs to write.

Hopefully, we know have cleanly separated the test connections using prefixes (see #195416: Table prefixes should be per database connection), and SQLite support attaching different database files for each prefix.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Status: Active » Needs review
FileSize
3.63 KB

Here is a starter patch.

There is a critical bug in getPrefixInfo(), as in "it doesn't work at all". Not sure how we managed to do that.

We have to copy the data over manually in DrupalWebTestCase::preloadRegistry() because the current code assume that all the data is reachable from the same connection. It will be slightly slower on MySQL, I would like to know how much.

Damien Tournoud’s picture

So, the test bot confirmed that there is no measurable slow down on MySQL. Nice.

982,809	 File	bartik-whitespace_2.patch	 Result received from test client #223.	 07/11/2010 - 12:07:43
982,769	 File	bartik-whitespace_2.patch	 Requested by test client #223.	 07/11/2010 - 11:41:02
982,719	 File	850852-sqlite-concurrent-testing.patch	 Result received from test client #223.	 07/11/2010 - 11:27:33
982,664	 File	850852-sqlite-concurrent-testing.patch	 Requested by test client #223.	 07/11/2010 - 11:01:01
Damien Tournoud’s picture

Cleaned-up patch.

Damien Tournoud’s picture

Status: Needs review » Needs work

It seems that db_table_exists() doesn't work correctly for a table with schema on SQLite. As a consequence, a bunch of tests fail.

Damien Tournoud’s picture

Status: Needs work » Needs review

Actually the failure is covered by #851698: DatabaseSchema_sqlite severely broken. This one is good to be reviewed :)

Damien Tournoud’s picture

Added a destruct function, so that we prune the empty database files that are created by Simpletest.

chx’s picture

Status: Needs review » Needs work

"Iterate over all the possible attached databases: the main database, the default prefix and other prefixes. Remove the empty prefixes, which are equal to the main database."

"The constructor attached a database for every non-empty prefix. Remove these databases if they contain no tables. For example, simpletest drops its tables and so the database becomes empty."

In the code I do not get this 'main' thing in the destructor as it is not matched by the constructor.

chx’s picture

One would presume that the 'main' stuff comes from http://www.sqlite.org/lang_attach.html The database-names 'main' and 'temp' refer to the main database and the database used for temporary tables. The main and temp databases cannot be attached or detached.

but then you need to exempt temp too. More, you added a dot to every prefix so it can not be equal to main anyways. Would the constructor work with a 'main' database name anyways? Finally, why are the file names different, both attach commands are in the form of $connection_options['database'] . '-' . $prefix

Edit: sorry, I see now array('main.', $this->defaultPrefix), main is specifically added to the prefixes we check for emptiness. But then I do not see how the main database can be empty? Self-destruct Drupal? :)

Edit2: even if this what we can't can we write the more legible

$all_prefixes = $this->prefixes;
// Add the main database because...
$all_prefixes[] = 'main'.;
$all_prefixes[] = $this->defaultPrefix;
andypost’s picture

New 3.7 has a great improvement http://www.sqlite.org/wal.html

dmitrig01’s picture

bump. we need this as it fixes a test failure

dmitrig01’s picture

Title: Concurrent testing on SQLite » Fix transaction failure and allow concurrent testing on SQLite
Category: task » bug
Priority: Normal » Major
Damien Tournoud’s picture

Priority: Major » Critical

Indeed. Since #195416: Table prefixes should be per database connection, the logging of the test assertions happens in a separate database connection. Because of the way SQLite work (ie. it locks the whole database when starting a transaction), the transaction tests just cannot work: the assertions end up with General error: 5 database is locked exceptions.

Because this is contained in SQLite, and because this is the very last step before reaching 100% pass on SQLite, I'm going to bump this issue to critical.

It should not be a lot of work to get this in (mainly adding comments). Any taker?

dmitrig01’s picture

wokring on it

dmitrig01’s picture

Status: Needs work » Needs review
FileSize
9.05 KB

Ok, cleaned the patch up with much guidance from DamZ.

dmitrig01’s picture

FileSize
7.86 KB

ups, bad at managing patches

dmitrig01’s picture

FileSize
7.06 KB

more

dmitrig01’s picture

FileSize
6.51 KB

With fixes from IRC, removing filter and unique.

dmitrig01’s picture

dmitrig01’s picture

Fixed a small comment thing

chx’s picture

Status: Needs review » Reviewed & tested by the community

DamZ and I both have tested this and we discussed it to bits on IRC.

chx’s picture

Here is a screenshot of my testing http://img63.imageshack.us/img63/65/screenshot041hx.png -- the notice is a diff issue -- http://ci.drupal.org/result.php?job=drupal-test-sqlite&build=24 here is DamZ's.

chx’s picture

Finally, we have discussed whether moving that PRAGMA encoding utf8 above the ATTACH is necessary -- it's not, in fact the whole PRAGMA is moot because encoding can be either UTF-8 or UTF-16 and the PDO driver uses the UTF-8 way to access the database.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

I've been told this causes additional test failures.

Also, can we get some comments in protected function preloadRegistry() {?

dmitrig01’s picture

Status: Needs work » Needs review
FileSize
8.52 KB

Fixed the failures, and added comments

chx’s picture

Status: Needs review » Reviewed & tested by the community

Nice and well commented.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Great work on this, folks!

Committed to HEAD. :)

Damien Tournoud’s picture

Status: Fixed » Needs review
FileSize
1.37 KB

Oups, sorry, we slightly messed-up.

The new implementation of was completely incorrect (my bad, I suggested this course to Dmitri). As a result notices were thrown in tearDown(), causing the upgrade path tests to fail.

chx’s picture

Please link a ci job with this -- i only tested the module tests to pass not all and would love to see all tests with this patch.

Damien Tournoud’s picture

chx’s picture

Status: Needs review » Reviewed & tested by the community

Then please commit this. Above I only tested the module tests that's wny.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

*mutters something about 'reviewed and tested by community'* :P

Committed to HEAD.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.