Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner created an issue. See original summary.

dawehner’s picture

Issue summary: View changes
FileSize
181.69 KB

Let's see where we are.

dawehner’s picture

Some fixes ...

Status: Needs review » Needs work

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

The last submitted patch, 2: 2684141-2.patch, failed testing.

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
184.12 KB
1.95 KB

Does this help?

Status: Needs review » Needs work

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
184.21 KB
3.27 KB

Let's see ...

Status: Needs review » Needs work

The last submitted patch, 9: 2684141-9.patch, failed testing.

benjy’s picture

+1 from me, would love to see this.

dawehner’s picture

Yeah, let's wait until the migration as plugin is in.

dawehner’s picture

This has to wait until #2625696: Make migrations themselves plugins instead of config entities is in, as it will conflict quite a bit.

dawehner’s picture

Status: Needs work » Needs review
FileSize
177.2 KB

Reroll

Status: Needs review » Needs work

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

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

benjy’s picture

Not sure what the CI error is?

+++ b/core/modules/action/tests/src/Kernel/Migrate/d6/MigrateActionConfigsTest.php
@@ -2,20 +2,20 @@
+class MigrateActionConfigsTest extends \Drupal\Tests\migrate_drupal\Kernel\d6\MigrateDrupal6TestBase {

Needs a use statement, there a few of these.

dawehner’s picture

Assigned: dawehner » Unassigned

Unassigning from myself. I'll work on it at sometime, unless someone else takes it.

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

benjy’s picture

Status: Needs work » Needs review
FileSize
171.86 KB

Really keen to see this, running from PHPStorm is so convenient. Re-rolled and fixed the absolute namespaces at the same time.

Status: Needs review » Needs work

The last submitted patch, 20: 2684141-20.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
176.08 KB
4.13 KB

Found the fatal, there was an empty test file. Also changed a few more tests over to KTB. Had some fails with EntityFileTest, not sure if that is specific to my environment or not, see what the bot says.

Status: Needs review » Needs work

The last submitted patch, 22: 2684141-22.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
175.08 KB
1.51 KB

More empty files.

Status: Needs review » Needs work

The last submitted patch, 24: 2684141-24.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
174.49 KB
4.91 KB

Some more fixes, I reverted back EntityFileTest because that uses $base_url which doesn't seem to get set correctly in the new KTB.

Status: Needs review » Needs work

The last submitted patch, 26: 2684141-26.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
175.43 KB

Down to one fail, not sure how to get the vfs stuff working in MigrateFileTest

dawehner’s picture

@benjy
Just to be clear, the old kernel test did the same stuff when setting up the request.

Status: Needs review » Needs work

The last submitted patch, 28: 2684141-28.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
181.93 KB
14.48 KB

Fixed both files tests using vfs, thanks to dawehner for the help getting public:// working.

Status: Needs review » Needs work

The last submitted patch, 31: 2684141-31.patch, failed testing.

dawehner’s picture

Wait, is the idea really to remove the test?

benjy’s picture

hmm, that test passed locally for me, could be something to do with the global temp files on the bot.

benjy’s picture

No, that was just a bad interdiff, I actually moved it. You can tell because its the one that failed in #31 :)

benjy’s picture

Status: Needs work » Needs review
FileSize
181.93 KB
592 bytes

Fixed.

dawehner’s picture

Status: Needs review » Needs work

Thanks a lot to take over this!

  1. +++ b/core/modules/file/tests/src/Kernel/Migrate/EntityFileTest.php
    @@ -76,7 +74,7 @@ protected function localFileDataProvider() {
    -      [['filepath' => 'core/modules/simpletest/files/image-test.jpg'], 'public://remote-file.jpg', TRUE, $base_url . '/'],
    +      [['filepath' => 'core/modules/simpletest/files/image-test.jpg'], 'public://remote-file.jpg', TRUE, DRUPAL_ROOT . '/'],
    

    Nitpick: Let's use $this->root

  2. +++ b/core/modules/migrate/src/Tests/MigrateEmbeddedDataTest.php
    index baa4149..8268c2f 100644
    --- a/core/modules/migrate/src/Tests/MigrateEventsTest.php
    
    --- a/core/modules/migrate/src/Tests/MigrateEventsTest.php
    +++ b/core/modules/migrate/src/Tests/MigrateEventsTest.php
    
    +++ b/core/modules/migrate/src/Tests/MigrateEventsTest.php
    index e65607a..b1877d6 100644
    --- a/core/modules/migrate/src/Tests/MigrateInterruptionTest.php
    
    --- a/core/modules/migrate/src/Tests/MigrateInterruptionTest.php
    +++ b/core/modules/migrate/src/Tests/MigrateInterruptionTest.php
    
    +++ b/core/modules/migrate/src/Tests/MigrateInterruptionTest.php
    index 30211e1..e915319 100644
    --- a/core/modules/migrate/src/Tests/MigrateMessageTest.php
    
    --- a/core/modules/migrate/src/Tests/MigrateMessageTest.php
    +++ b/core/modules/migrate/src/Tests/MigrateMessageTest.php
    
    +++ b/core/modules/migrate/src/Tests/MigrateMessageTest.php
    index 1f4461f..c910a02 100644
    --- a/core/modules/migrate/src/Tests/MigrateSkipRowTest.php
    
    --- a/core/modules/migrate/src/Tests/MigrateSkipRowTest.php
    +++ b/core/modules/migrate/src/Tests/MigrateSkipRowTest.php
    
    +++ b/core/modules/migrate/src/Tests/MigrateSkipRowTest.php
    index 998337d..e1c1875 100644
    --- a/core/modules/migrate/src/Tests/MigrationTest.php
    
    --- a/core/modules/migrate/src/Tests/MigrationTest.php
    +++ b/core/modules/migrate/src/Tests/MigrationTest.php
    

    We missed to move those files

benjy’s picture

Status: Needs work » Needs review
FileSize
198.97 KB
6.82 KB

Thanks for the review, pretty excited to get this in, the experience of running these locally is much better although its a shame the bot doesn't give better results.

Feedback done.

dawehner’s picture

Thanks for the review, pretty excited to get this in, the experience of running these locally is much better although its a shame the bot doesn't give better results.

Well, a) this will improve at some point and b) I just don't care that much as you need to run them locally anyway in order to fix stuff.

benjy’s picture

I went over the patch looks pretty good to me, every file seemed to have a namespace and a comment update along with the file being moved.

I've removed one assertion from the EntityFileTest because of a limitation with realpath and vfs which was causing it to fail. We have a similar assertion right below that tests the same thing without vfs anyway.

benjy’s picture

FileSize
866 bytes

Forgot the interdiff.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you benjy!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 4478bb1 and pushed to 8.1.x. Thanks!

diff --git a/core/modules/block/tests/src/Kernel/Migrate/d7/MigrateBlockTest.php b/core/modules/block/tests/src/Kernel/Migrate/d7/MigrateBlockTest.php
index 81f8399..3d177c4 100644
--- a/core/modules/block/tests/src/Kernel/Migrate/d7/MigrateBlockTest.php
+++ b/core/modules/block/tests/src/Kernel/Migrate/d7/MigrateBlockTest.php
@@ -81,7 +81,6 @@ protected function setUp() {
    *   The block label.
    * @param string $label_display
    *   The block label display setting.
-
    */
   public function assertEntity($id, $plugin_id, array $roles, $pages, $region, $theme, $weight, $label, $label_display) {
     $block = Block::load($id);
diff --git a/core/modules/migrate_drupal/tests/src/Kernel/MigrateDrupalTestBase.php b/core/modules/migrate_drupal/tests/src/Kernel/MigrateDrupalTestBase.php
index 0fd5f87..68c4211 100644
--- a/core/modules/migrate_drupal/tests/src/Kernel/MigrateDrupalTestBase.php
+++ b/core/modules/migrate_drupal/tests/src/Kernel/MigrateDrupalTestBase.php
@@ -8,7 +8,6 @@
 namespace Drupal\Tests\migrate_drupal\Kernel;
 
 use Drupal\Core\Database\Database;
-use Drupal\Component\Plugin\Exception\PluginNotFoundException;
 use Drupal\Tests\migrate\Kernel\MigrateTestBase;
 
 /**
diff --git a/core/modules/path/tests/src/Kernel/Migrate/d6/MigrateUrlAliasTest.php b/core/modules/path/tests/src/Kernel/Migrate/d6/MigrateUrlAliasTest.php
index c14b331..02d4583 100644
--- a/core/modules/path/tests/src/Kernel/Migrate/d6/MigrateUrlAliasTest.php
+++ b/core/modules/path/tests/src/Kernel/Migrate/d6/MigrateUrlAliasTest.php
@@ -9,7 +9,6 @@
 
 use Drupal\migrate\Plugin\MigrateIdMapInterface;
 use Drupal\Core\Database\Database;
-use Drupal\path\Tests\Migrate\d6\pid;
 use Drupal\Tests\migrate_drupal\Kernel\d6\MigrateDrupal6TestBase;
 
 /**

Fixed on commit

  • alexpott committed 4202cdd on 8.2.x
    Issue #2684141 by benjy, dawehner: Convert migrate kernel tests to...

Status: Fixed » Closed (fixed)

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