D6->D8 Core block migrations missing settings tests

Since the issue this was postponed on was fixed I decided to reroll the patch. There is no interdiff because it fails on the d6 table file, MenuLinks.php

Here is the reroll. I didn't add an interdiff because it is much larger than the patch itself, due to moving from the dump Tables to the test fixture, drupal6.php

All the tests pass except for the aggregator and the book block tests. And they fail because those blocks are not migrated. Not sure why that is happening.

reroll

Fixed: \Drupal\Tests\block\Kernel\Migrate\d6\MigrateBlockTest

Test had been checked on local

Fixed: Drupal\Tests\book\Kernel\Migrate\d6\MigrateBookConfigsTest

@vprocessor, thanks for picking up on this. I notice that the patch in 21 is much larger that the one in #13. It looks like #21is a reroll of a patch before #13 and is a patch for 8.0.x. It is easy to tell because it contains Table files for the test fixture. These have been replaced with a full dump of the d6 database, core/modules/migrate_drupal/test/fixtures/drupal6.php. Also, WebAssert isn't related to this issue. I suggest starting from the patch in #13, which contains an updated test fixture that has the changes to the D6 database for this issue.

almost tests fixed

@quietone, ups, ok, will use patch from #13

Fixed:
Migrate_drupal.Drupal\Tests\migrate_drupal\Kernel\dependencies\MigrateDependenciesTest

Still needs reroll. Assuming vprocessor isn't working on this anymore.

Your branch is up-to-date with 'origin/8.2.x'.
$ curl https://www.drupal.org/files/issues/2422229-27.patch | patch -p1
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 81454  100 81454    0     0   109k      0 --:--:-- --:--:-- --:--:--  109k
patching file core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php
Hunk #2 succeeded at 97 (offset 4 lines).
Hunk #3 succeeded at 115 (offset 4 lines).
Hunk #4 succeeded at 127 (offset 4 lines).
Hunk #5 succeeded at 142 (offset 4 lines).
patching file core/modules/book/tests/src/Kernel/Migrate/d6/MigrateBookConfigsTest.php
patching file core/modules/forum/tests/src/Kernel/Migrate/d6/MigrateForumConfigsTest.php
patching file core/modules/migrate_drupal/tests/fixtures/drupal6.php
Hunk #9 succeeded at 9300 (offset 10 lines).
Hunk #10 succeeded at 9378 (offset 10 lines).
Hunk #11 succeeded at 21728 (offset 10 lines).
Hunk #12 succeeded at 32796 (offset 10 lines).
Hunk #13 succeeded at 35083 (offset 10 lines).
Hunk #14 succeeded at 35853 (offset 10 lines).
Hunk #15 succeeded at 38449 (offset 10 lines).
Hunk #16 succeeded at 38647 (offset 10 lines).
Hunk #17 succeeded at 41067 (offset 10 lines).
Hunk #18 succeeded at 41397 (offset 10 lines).
Hunk #19 succeeded at 41551 (offset 10 lines).
Hunk #20 succeeded at 42079 (offset 10 lines).
Hunk #21 succeeded at 43164 (offset 80 lines).
Hunk #22 succeeded at 43999 (offset 68 lines).
Hunk #23 succeeded at 44011 (offset 68 lines).
Hunk #24 succeeded at 44023 (offset 68 lines).
Hunk #25 succeeded at 44035 (offset 68 lines).
Hunk #26 succeeded at 44047 (offset 68 lines).
Hunk #27 succeeded at 44059 (offset 68 lines).
Hunk #28 succeeded at 44071 (offset 68 lines).
Hunk #29 succeeded at 44083 (offset 68 lines).
Hunk #30 succeeded at 44095 (offset 68 lines).
Hunk #31 succeeded at 44107 (offset 68 lines).
Hunk #32 succeeded at 44131 (offset 68 lines).
Hunk #33 succeeded at 44143 (offset 68 lines).
Hunk #34 succeeded at 44155 (offset 68 lines).
Hunk #35 succeeded at 44167 (offset 68 lines).
Hunk #36 succeeded at 44179 (offset 68 lines).
Hunk #37 succeeded at 44191 (offset 68 lines).
Hunk #38 succeeded at 44203 (offset 68 lines).
Hunk #39 succeeded at 44215 (offset 68 lines).
Hunk #40 succeeded at 44227 (offset 68 lines).
Hunk #41 succeeded at 44239 (offset 68 lines).
Hunk #42 succeeded at 44251 (offset 68 lines).
Hunk #43 succeeded at 44275 (offset 68 lines).
Hunk #44 succeeded at 44287 (offset 68 lines).
Hunk #45 succeeded at 44299 (offset 68 lines).
Hunk #46 succeeded at 44311 (offset 68 lines).
Hunk #47 succeeded at 44323 (offset 68 lines).
Hunk #48 succeeded at 44335 (offset 68 lines).
Hunk #49 succeeded at 44347 (offset 68 lines).
Hunk #50 succeeded at 44359 (offset 68 lines).
Hunk #51 succeeded at 44371 (offset 68 lines).
Hunk #52 succeeded at 44383 (offset 68 lines).
Hunk #53 succeeded at 44410 (offset 68 lines).
Hunk #54 succeeded at 44506 (offset 68 lines).
Hunk #55 succeeded at 44938 (offset 68 lines).
Hunk #56 succeeded at 44995 (offset 68 lines).
Hunk #57 succeeded at 45007 (offset 68 lines).
Hunk #58 FAILED at 45769.
Hunk #59 succeeded at 46059 (offset 68 lines).
Hunk #60 succeeded at 46807 (offset 80 lines).
Hunk #61 succeeded at 46834 (offset 80 lines).
Hunk #62 succeeded at 46847 (offset 80 lines).
Hunk #63 succeeded at 46858 (offset 80 lines).
Hunk #64 succeeded at 47031 (offset 84 lines).
Hunk #65 succeeded at 47412 (offset 88 lines).
1 out of 65 hunks FAILED -- saving rejects to file core/modules/migrate_drupal/tests/fixtures/drupal6.php.rej
patching file core/modules/migrate_drupal/tests/src/Kernel/dependencies/MigrateDependenciesTest.php
patching file core/modules/statistics/tests/src/Kernel/Migrate/d6/MigrateStatisticsConfigsTest.php
patching file core/tests/Drupal/KernelTests/KernelTestBase.php
Hunk #1 succeeded at 21 with fuzz 1 (offset -1 lines).
Hunk #2 succeeded at 1169 (offset -49 lines).

Rerolled the patch

Wrong branch. Rerolled according to 8.2.x branch.

We'll be watching this today if we can get this patch working.

Let me try to get this through.

Hmm, I got lots of

Reversed (or previously applied) patch detected!

Did you accidentally posted same patch twice? According to patch size it looks like that?

Reroll Try #3

I've resolved many of the test failures, but I cannot sort out 5 of the failing assertions in MigrateUpgrade6Test.

The aggregator, book, forum and statistics modules need to be enabled in MigrateUpgradeTestBase so those migrations run. That required the entities counts to changes in the d6 and d7 tests.

Changed d6_statistics_settings to statistics_settings in MigrateUpgradeForm. But, that likely means this will need a reroll when #2569805: For Drupal migration, identify the source module is committed.

Removed changes to KernelTestBase.php that appeared in patch #24.

Self-assigning for review.

1. +++ b/core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php
   @@ -45,11 +50,9 @@ protected function setUp() {
   -    block_rebuild();
   

   Why was this line removed? I seem to recall putting that there, and for a specific reason. Which, now that I'm looking at it here, should have been documented. Because I can't remember the issue where it was introduced. (Time to dig up the ol' git blame.)

2. +++ b/core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php
   @@ -91,17 +94,17 @@ public function assertEntity($id, $visibility, $region, $theme, $weight, $label,
   +    $this->assertSame(14, count($blocks));
   

   assertCount() is preferable here.

3. +++ b/core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php
   @@ -109,7 +112,9 @@ public function testBlockMigration() {
   +    $config = $this->config('block.block.user_2');
   +    $this->assertSame('5', $config->get('settings.items_per_page'));
   

   I'd rather not use simple config to do this assertion -- I'd prefer to load the block entity and check the settings from there.

4. +++ b/core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php
   @@ -119,9 +124,11 @@ public function testBlockMigration() {
   +    $config = $this->config('block.block.user_3');
   +    $this->assertSame('10', $config->get('settings.items_per_page'));
   

   Ditto.

5. +++ b/core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php
   @@ -132,6 +139,38 @@ public function testBlockMigration() {
   +    $visibility = [];
   +    $this->assertEntity('aggregator', $visibility, 'sidebar_second', 'bartik', -2, '', '0');
   

   Nit: $visibility is never used. Why not just pass an empty array as the second argument to assertEntity()?

6. +++ b/core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php
   @@ -132,6 +139,38 @@ public function testBlockMigration() {
   +    $config = $this->config('block.block.aggregator');
   +    $this->assertSame(7, $config->get('settings.block_count'));
   +    $this->assertSame('5', $config->get('settings.feed'));
   

   Rather than do stuff like this, could we change assertEntity() to accept another parameter, which is an array of block settings to assert? It doesn't have to be *all* the block settings, just the ones we want to check.

Needs a reroll.

1. Restored. You added it ages ago #2408165: Migration Files for Drupal 7 Blocks.
2. Fixed.
5. Fixed.
3, 4 and 6. All the occurrences of config->get are gone. A full settings array is built and passed to assertEntity for testings. A side effect is that it now easy to see that several of the blocks have an 'id' of 'broken'. That needs a bit of investigation.

Another thing is that arrays are built with two different styles and it is not pretty to read. I'd like to tidy that up, after all so much has changed because of items 3, 4, and 6. I hope that won't be out of of scope.

But first, lets make sure all the tests pass.

And the patch.

Removed duplicate tests and tidy up to make it easier to read.

There is a change in MigrateUpgradeForm.php, it may be correct, but could that be split with it's related tests (if that is possible) into a follow-up? Edit: mentioned in #48, may already have a follow-up

This patch is big and should only include adding tests of "everything's ok alarms". If we find a bug that's great but probably deserves it's own issue.

Fair enough. I have removed the fix in MigrateUpgradeForm. Created a new issue for the wee bug #2855611: statistics_settings migration not run in MigrateUpgrade tests.

Ah, of course, the patch in #57 is correct. That fix needs to be done here.

So, the statistics module need to be enabled for the MigrationBlockTest, which needs the statistics block and the statistics settings. That also means that statistics module must be enabled for MigrateUpgrade6Test. Now, once it is enabled, MigrateUpgrade test expects to find all the migrations that are listed in the big array in MigrateUpgradeForm. That array has d6_statistics_settings, which does not exist. Thus the failure seen in the results in #59.

To fix that the name must be changed in the big array.

I hope all that makes sense. I am a bit over tired.

Setting back to NR. Reviewers - review the patch in #57.

I found nothing remotely serious. Exceedingly close to being RTBC-ready. I think, however, we need to carefully explain (in the next comment) why we have Statistics-related changes in this patch, because that will look like noise, or patch pollution, to the committers.

1. +++ b/core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php
   @@ -35,10 +41,9 @@ protected function setUp() {
   +    // Set Bartik and Seven as the default public theme.
   

   It looks like Seven is no longer set as anything, so we should probably remove the mention. Kind of a nitpick, though, and certainly fixable on commit.

2. +++ b/core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php
   @@ -66,14 +70,12 @@ protected function setUp() {
   +  public function assertEntity($id, $visibility, $region, $theme, $weight, $settings, $status = TRUE) {
   

   $settings should be type hinted as array and defaulted as NULL so that asserting the settings can be optional. The description for $settings should then be prefixed with (optional).

3. +++ b/core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php
   @@ -81,10 +83,9 @@ public function assertEntity($id, $visibility, $region, $theme, $weight, $label,
   +    $block_settings = $block->get('settings');
   +    $block_settings['id'] = current(explode(':', $block_settings['id']));
   +    $this->assertEquals($settings, $block_settings);
   

   To wit, this should be wrapped in if (isset($settings)).

4. +++ b/core/modules/migrate_drupal_ui/src/Tests/MigrateUpgradeTestBase.php
   @@ -143,8 +152,11 @@ protected function testMigrateUpgrade() {
   +      $real_count = count(\Drupal::entityTypeManager()
   +        ->getStorage($entity_type)
   +        ->loadMultiple());
   

   A lighter-weight alternative would be \Drupal::entityQuery($entity_type)->count()->execute(). Just an observation, no particular need to change this (unless you want to).

In order to add the settings tests back into the MigrateBlockTest some modules, aggregator, block, forum and statistics, needed to be enabled so the relevant migrations would be run. That works fine for MigrateBlockTest.

And they need to be enabled in MigrateUpgradeTestBase or we get the errors in#45. Adding those modules exposed an error in the list of migrations in MigrateUpgradeForm, where the statistic setting migration was listed as d6_statistics_settings instead of the statistics_setting. Without the correct id the statistics_settings migration was not running and the tests would fail.

edit: Made all changes from #62.

C'est fantastique.

Re-rolled.

And back to green we go!

Retests passed, back to RTBC.

Retests passed, back to RTBC.

Committed and pushed 133dccc to 8.4.x and 59798d6 to 8.3.x. Thanks!

+++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
@@ -426,7 +426,7 @@ class MigrateUpgradeForm extends ConfirmFormBase {
-    'd6_statistics_settings' => [
+    'statistics_settings' => [

This makes this a bug fix. And it's great that we have UI test coverage here too.

Backported to 8.3.x because migrate is still experimental.