If the MySQL auto_increment_increment variable is not the default 1 (say, 5), this test will fail to pass as it'll return that Value '2' is identical to value '6'.

/**
   * Tests the Drupal 6 custom block to Drupal 8 migration.
   */
  public function testBlockMigration() {
    /** @var BlockContent $block */
    $block = BlockContent::load(1);
    $this->assertIdentical('My block 1', $block->label());
    $this->assertIdentical('1', $block->getRevisionId());

    // var_dump($block->getRevisionId());
    // die();
    // returns ResponseText: string(1) "1"

    $this->assertTrue(REQUEST_TIME <= $block->getChangedTime() && $block->getChangedTime() <= time());
    $this->assertIdentical('en', $block->language()->getId());
    $this->assertIdentical('<h3>My first custom block body</h3>', $block->body->value);
    $this->assertIdentical('full_html', $block->body->format);

    $block = BlockContent::load(2);
    $this->assertIdentical('My block 2', $block->label());
    $this->assertIdentical('2', $block->getRevisionId());

    // var_dump($block->getRevisionId());
    // die();
    // returns ResponseText: string(1) "6"

    $this->assertTrue(REQUEST_TIME <= $block->getChangedTime() && $block->getChangedTime() <= time());
    $this->assertIdentical('en', $block->language()->getId());
    $this->assertIdentical('<h3>My second custom block body</h3>', $block->body->value);
    $this->assertIdentical('full_html', $block->body->format);
  }

}

And the corresponding records:

mysql> select * from simpletest567326block_content;
+----+-------------+-------+--------------------------------------+
| id | revision_id | type  | uuid                                 |
+----+-------------+-------+--------------------------------------+
|  1 |           1 | basic | 84da446b-506f-47bf-9f01-1fc8b510e51a |
|  2 |           6 | basic | 61ded913-2392-4671-9400-decfdfb97b84 |
+----+-------------+-------+--------------------------------------+
2 rows in set (0.00 sec)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

anavarre’s picture

Looks like a good bet would be PDO::lastInsertId

anavarre’s picture

Status: Active » Needs review
FileSize
1.14 KB

Took a stab at it.

anavarre’s picture

FileSize
1.14 KB

Some invisible whitespace made it to the patch. Trying again...

anavarre’s picture

FileSize
1.13 KB

FFS. Had to edit the patch file.

The last submitted patch, 3: 2462893-3.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 4: 2462893-4.patch, failed testing.

anavarre’s picture

Status: Needs work » Needs review
FileSize
1.14 KB

Status: Needs review » Needs work

The last submitted patch, 7: 2462893-7.patch, failed testing.

Status: Needs work » Needs review

ultimike queued 7: 2462893-7.patch for re-testing.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Looks proper to me. Ideally, the test bots would have a non-default auto-increment so that any regression would get caught. That can be a follow-up.

xjm’s picture

Hm, I find myself wondering what the intention of this assertion is anyway? Is it asserting the specific revision IDs for a reason? Looking at the dump provided, it expects the IDs of the blocks to be 1 and 2, respectively -- this is hardcoded. But why do we care about what the revision IDs are?

The assertion was added in #2250429: d6_block migration not migrating all blocks. I guess if the idea is just to ensure that the newly created entities have revision IDs, then that's fine, and the patch is certainly an improvement.

xjm’s picture

+++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateBlockContentTest.php
@@ -63,9 +63,12 @@ public function testBlockMigration() {
+    // Take into account a non-default MySQL auto_increment_increment value.

Also, I don't think the comment should refer to MySQL, because it should apply to any storage, right?

benjy’s picture

Status: Reviewed & tested by the community » Needs work

Is it asserting the specific revision IDs for a reason?

We usually assert revision ids to ensure that the revision id has come across since we preserve ids.

In the case of blocks, I don't think they have revisions in D6, making this test pointless. I'd opt to just remove it.

anavarre’s picture

Status: Needs work » Needs review
FileSize
1.28 KB

#12 - I can't talk about every single storage, but for PGSQL the equivalent would be SERIAL

Patch adresses the consensus in #11 and #13. No interdiff since it's a different approach.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

OK, lets go with it.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: 2462893-14.patch, failed testing.

Status: Needs work » Needs review

anavarre queued 14: 2462893-14.patch for re-testing.

anavarre’s picture

Status: Needs review » Reviewed & tested by the community

Changing status again.

xjm’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Simpletest

That makes more sense to me, thanks!

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed and pushed to 8.0.x.

  • xjm committed c530a08 on 8.0.x
    Issue #2462893 by anavarre, benjy: MigrateBlockContentTest assumes auto-...

Status: Fixed » Closed (fixed)

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