Problem/Motivation

Most tests in the migrate_drupal test group are failing on SQLite.

Proposed resolution

1. Fix the 'content_field_multivalue' table to not use a serial type (autoincrement) for the 'vid' field. This table is simulating a field data table, and those do not have an autoincrement on the vid column either.

2. The database name needs some special handling because it currently includes the relative path to the sqlite file.

Remaining tasks

Figure out if the database name needs special escape handling.

User interface changes

Nope.

API changes

Nope.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu’s picture

Status: Active » Needs review
FileSize
54.32 KB
607 bytes

Here's a full fail log for the migrate_drupal test group and the first part of the fix. For now I'm just curious if this causes any problems on MySQL.. it shouldn't :)

amateescu’s picture

Status: Needs review » Needs work

Ok, that small change doesn't break anything on MySQL, setting to NW because we still need to figure out part 2 from the issue summary.

benjy’s picture

+++ b/core/modules/migrate_drupal/src/Tests/Table/d6/ContentFieldMultivalue.php
@@ -27,7 +27,7 @@ public function load() {
-          'type' => 'serial',
+          'type' => 'int',

You can't make changes to these files manually, they're auto-generated dumps.

amateescu’s picture

@benjy, then what's the correct procedure to update the dumps, the one explained here https://www.drupal.org/sandbox/benjy/2405029? If so, "Login to the D6 site, make changes as required." this step means that I actually need to have a D6 installation?

amateescu’s picture

FileSize
2.3 KB

In the meantime, I made some progress here. With the patch attached, half of the migrate_drupal tests are passing but the other half fails with something like this:

    Exception Uncaught e Connection.php     612 Drupal\Core\Database\Connection->ha
    Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[HY000]: General
    error: 5 database is locked: DELETE FROM {migrate_message_d6_upload} 
    WHERE  (sourceid1 = 1) ; Array
    (
    )
     in Drupal\migrate\Plugin\migrate\id_map\Sql->delete() (line 617 of
    /home/andrei/work/d8.dev/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php).
    Drupal\Core\Database\Connection->handleQueryException(Object, 'DELETE FROM
    {migrate_message_d6_upload} 
    WHERE  (sourceid1 = 1) ', Array, Array)
    Drupal\Core\Database\Driver\sqlite\Connection->handleQueryException(Object,
    'DELETE FROM {migrate_message_d6_upload} 
    WHERE  (sourceid1 = 1) ', Array, Array)
    Drupal\Core\Database\Connection->query('DELETE FROM
    {migrate_message_d6_upload} 
    WHERE  (sourceid1 = :db_condition_placeholder_0) ', Array, Array)
    Drupal\Core\Database\Query\Delete->execute()
    Drupal\Core\Database\Driver\sqlite\Delete->execute()
    Drupal\migrate\Plugin\migrate\id_map\Sql->delete(Array, 1)
    Drupal\migrate\MigrateExecutable->import()
    Drupal\migrate_drupal\Tests\d6\MigrateUploadTest->setUp()
    Drupal\simpletest\TestBase->run(Array)
    simpletest_script_run_one_test('338',
    'Drupal\migrate_drupal\Tests\d6\MigrateUploadTest')

I tried a rerolled version of #1120020-40: SQLite database locking errors cause fatal errors but no luck so far.. still digging :/

benjy’s picture

this step means that I actually need to have a D6 installation?

Yes exactly. The code for that database is in the sandbox.

The most up-to-date instructions I believe are in /core/scripts/migrate-dump-d6.sh

amateescu’s picture

Status: Needs work » Needs review
FileSize
101.89 KB
43.66 KB

I spent the last couple of days trying to fix the database locking issue mentioned in #5 but that is not really a migrate_drupal problem and we already have an issue for it: #1120020: SQLite database locking errors cause fatal errors

Updated ContentFieldMultivalue properly via the d6.gz database dump and I'm attaching the current status for the migrate_drupal tests. Without this patch, all of them were failing.

amateescu’s picture

FileSize
103.92 KB
2.03 KB

Together with #2465633: Bring back the custom Statement class for the SQLite driver and another small fix here, we have all the individual migrate_drupal tests passing! The one that runs all of them in one go, Drupal\migrate_drupal\Tests\d6\MigrateDrupal6Test, is still failing :/ Probably because it skips the setUp()/tearDown() methods of individual test classes and it runs a couple of tests on the wrong database file.

I think we should open a separate issue for that one test class and just go ahead here with a patch that fixes everything else :)

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
    @@ -458,4 +458,14 @@ public function popTransaction($name) {
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function getFullQualifiedTableName($table) {
    +    $prefix = $this->tablePrefix($table);
    +
    +    // Don't include the SQLite database file name as part of the table name.
    +    return $prefix . $table;
    +  }
    

    Just in general, I was thinking whether we should add unit tests for those methods, even if we would have to skip the constructor.

  2. +++ b/core/modules/migrate_drupal/src/Tests/Table/d6/ContentFieldMultivalue.php
    @@ -27,9 +27,10 @@ public function load() {
           'fields' => array(
             'vid' => array(
    -          'type' => 'serial',
    +          'type' => 'int',
               'not null' => TRUE,
               'length' => '10',
    +          'default' => '0',
    

    That totally makes sense, and was probably just a c&p error.

  3. +++ b/core/modules/migrate_drupal/src/Tests/d6.gz
    @@ -619,90 +617,104 @@ w1
    ... meh ... it crashes drupal.org if you post that binary data ...
    

    That change looks bogus ... just kidding

benjy’s picture

The creating of the binary patch looks wrong, also, very small. It's usually 4-500Kbs? Some instructions on creating the patch for the gz file here: https://www.drupal.org/node/2452709#comment-9743751

getFullQualifiedTableName

I wish i'd spotted this when the method was added, getFullyQualifiedTableName() would have been much better. Maybe we can still rename this in a follow-up.

amateescu’s picture

Issue tags: +D8 Accelerate Dev Days
FileSize
142.55 KB
853 bytes

Just in general, I was thinking whether we should add unit tests for those methods, even if we would have to skip the constructor.

Then we would have to write three unit tests for each db driver. And Connection::$prefixes is protected and only set in the constructor, so I don't even know where I would start for a unit test. Let's just add an integration test that checks if this method works in all db drivers.

The creating of the binary patch looks wrong, also, very small. It's usually 4-500Kbs? Some instructions on creating the patch for the gz file here: https://www.drupal.org/node/2452709#comment-9743751

I re-did that d6.gz dance and this time I think it should be good, at least it's a proper binary patch :)

Running the new test from the interdiff without the patch:

Test summary
------------

Drupal\system\Tests\Database\BasicSyntaxTest                  22 passes             1 exceptions             

Test run duration: 4 sec

...

Exception Uncaught e Connection.php     609 Drupal\Core\Database\Connection->ha
Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[HY000]: General
error: 1 near ".": syntax error: SELECT COUNT(*) AS expression
FROM 
(SELECT 1 AS expression
FROM 
tmpd8sqlited8.simpletest858248.test t) subquery; Array
(
)
 in Drupal\simpletest\TestBase->run()

With the patch applied:

Test summary
------------

Drupal\system\Tests\Database\BasicSyntaxTest                  23 passes                                      

Test run duration: 4 sec
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Looks fine now!

The last submitted patch, 5: 2454669-5.patch, failed testing.

amateescu’s picture

The test failure above is for an older patch, the last one from #11 is fine.

alexpott’s picture

+++ b/core/modules/migrate_drupal/src/Tests/Table/d6/ContentFieldMultivalue.php
@@ -27,9 +27,10 @@ public function load() {
       'fields' => array(
         'vid' => array(
-          'type' => 'serial',
+          'type' => 'int',
           'not null' => TRUE,
           'length' => '10',
+          'default' => '0',
           'unsigned' => TRUE,
         ),

Just wondering how we got this wrong?

catch’s picture

I'd guess that's a copy and paste error from a vid column on the node revision table.

benjy’s picture

The dumps used to be managed by hand so a copy and paste error seems likely to me.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 629003e on 8.0.x
    Issue #2454669 by amateescu: SQLite: Fix tests in migrate_drupal test...

Status: Fixed » Closed (fixed)

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