Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#27 | 850852-follow-up.patch | 1.37 KB | Damien Tournoud |
#24 | 850852_transaction_24.patch | 8.52 KB | dmitrig01 |
#19 | 850852_transaction_19.patch | 6.1 KB | dmitrig01 |
#18 | 850852_transaction_18.patch | 6.29 KB | dmitrig01 |
#17 | 850852_transaction_17.patch | 6.51 KB | dmitrig01 |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedHere 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.Comment #2
Damien Tournoud CreditAttribution: Damien Tournoud commentedSo, the test bot confirmed that there is no measurable slow down on MySQL. Nice.
Comment #3
Damien Tournoud CreditAttribution: Damien Tournoud commentedCleaned-up patch.
Comment #4
Damien Tournoud CreditAttribution: Damien Tournoud commentedIt seems that db_table_exists() doesn't work correctly for a table with schema on SQLite. As a consequence, a bunch of tests fail.
Comment #5
Damien Tournoud CreditAttribution: Damien Tournoud commentedActually the failure is covered by #851698: DatabaseSchema_sqlite severely broken. This one is good to be reviewed :)
Comment #6
Damien Tournoud CreditAttribution: Damien Tournoud commentedAdded a destruct function, so that we prune the empty database files that are created by Simpletest.
Comment #7
chx CreditAttribution: chx commented"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.
Comment #8
chx CreditAttribution: chx commentedOne 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
Comment #9
andypostNew 3.7 has a great improvement http://www.sqlite.org/wal.html
Comment #10
dmitrig01 CreditAttribution: dmitrig01 commentedbump. we need this as it fixes a test failure
Comment #11
dmitrig01 CreditAttribution: dmitrig01 commentedComment #12
Damien Tournoud CreditAttribution: Damien Tournoud commentedIndeed. 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?
Comment #13
dmitrig01 CreditAttribution: dmitrig01 commentedwokring on it
Comment #14
dmitrig01 CreditAttribution: dmitrig01 commentedOk, cleaned the patch up with much guidance from DamZ.
Comment #15
dmitrig01 CreditAttribution: dmitrig01 commentedups, bad at managing patches
Comment #16
dmitrig01 CreditAttribution: dmitrig01 commentedmore
Comment #17
dmitrig01 CreditAttribution: dmitrig01 commentedWith fixes from IRC, removing filter and unique.
Comment #18
dmitrig01 CreditAttribution: dmitrig01 commented"look ma, no 'splosion"
Comment #19
dmitrig01 CreditAttribution: dmitrig01 commentedFixed a small comment thing
Comment #20
chx CreditAttribution: chx commentedDamZ and I both have tested this and we discussed it to bits on IRC.
Comment #21
chx CreditAttribution: chx commentedHere 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.
Comment #22
chx CreditAttribution: chx commentedFinally, 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.
Comment #23
webchickI've been told this causes additional test failures.
Also, can we get some comments in protected function preloadRegistry() {?
Comment #24
dmitrig01 CreditAttribution: dmitrig01 commentedFixed the failures, and added comments
Comment #25
chx CreditAttribution: chx commentedNice and well commented.
Comment #26
webchickGreat work on this, folks!
Committed to HEAD. :)
Comment #27
Damien Tournoud CreditAttribution: Damien Tournoud commentedOups, 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.
Comment #28
chx CreditAttribution: chx commentedPlease 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.
Comment #29
Damien Tournoud CreditAttribution: Damien Tournoud commentedWith this and #1004068: Complete the implementation of DatabaseSchema::addField() for SQLite, we are totally green.
http://ci.drupal.org/result.php?job=drupal-test-sqlite&build=2010-12-22_...
Comment #30
chx CreditAttribution: chx commentedThen please commit this. Above I only tested the module tests that's wny.
Comment #31
webchick*mutters something about 'reviewed and tested by community'* :P
Committed to HEAD.