Problem/Motivation

I just discovered that there are two test files testing the D7 node deriver, MigrateNodeDeriverTest.php and NodeMigrateDeriverTest.php. The difference in name made me wonder if NodeMigrateDeriverTest was doing something unusual. It is not, it is just confusing have the two files. Lets merge them into one.

Tagging as a novice task.

Proposed resolution

Move d7\NodeMigrateDeriverTest::testBuilder to d7\MigrateNodeDeriverTest::testBuilder.
Delete the file NodeMigrateDeriverTest.php

Remaining tasks

Make a patch
Review commit

CommentFileSizeAuthor
#8 3083196-8.patch4.08 KBiyyappan.govind
#5 3083196-5.patch4.08 KBiyyappan.govind
#2 3083196-2.patch4 KBiyyappan.govind
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quietone created an issue. See original summary.

iyyappan.govind’s picture

iyyappan.govind
he/him
Primary language Tamil
Location Chennai, India
CreditAttribution: iyyappan.govind at UniMity Solutions Pvt Limited commented
Status: Active » Needs review
FileSize
3083196-2.patch4 KB

Please review my patch. Thanks

iyyappan.govind’s picture

iyyappan.govind
he/him
Primary language Tamil
Location Chennai, India
CreditAttribution: iyyappan.govind at UniMity Solutions Pvt Limited commented
quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs review » Needs work 
+++ b/core/modules/node/tests/src/Kernel/Migrate/d7/MigrateNodeDeriverTest.php
@@ -35,4 +35,23 @@ public function testTranslations() {
+  public function testBuilder() {

This needs a doc bloc. Could be something like, 'Tests that the field process is added to the pipeline.'

iyyappan.govind’s picture

iyyappan.govind
he/him
Primary language Tamil
Location Chennai, India
CreditAttribution: iyyappan.govind at UniMity Solutions Pvt Limited commented
Status: Needs work » Needs review
FileSize
3083196-5.patch4.08 KB

Added the doc block. Please review it. Thanks

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at ICON Interactive commented
Status: Needs review » Needs work 
/**
 * Test D7NodeDeriver.
 *
 * @group migrate_drupal_7
 */
class MigrateNodeDeriverTest extends MigrateDrupal7TestBase {

As long as we are in there, can we move this to @group node? I think it was copied over from contrib incorrectly.

iyyappan.govind’s picture

iyyappan.govind
he/him
Primary language Tamil
Location Chennai, India
CreditAttribution: iyyappan.govind at UniMity Solutions Pvt Limited commented

Can we add this as doc block? 

/**
 * Tests the d7_node node deriver.
 *
 * @group node
 */
iyyappan.govind’s picture

iyyappan.govind
he/him
Primary language Tamil
Location Chennai, India
CreditAttribution: iyyappan.govind at UniMity Solutions Pvt Limited commented
FileSize
3083196-8.patch4.08 KB

Updated the doc block. Please check it.

iyyappan.govind’s picture

iyyappan.govind
he/him
Primary language Tamil
Location Chennai, India
CreditAttribution: iyyappan.govind at UniMity Solutions Pvt Limited commented
Status: Needs work » Needs review
quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented

@mikelutz, the @group tags for our tests are not consistent. Should we have an issue to tidy that up?

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at ICON Interactive commented
Status: Needs review » Reviewed & tested by the community

@quietone They should always match the module machine_name, but I know we've moved tests from module to module and contrib to core etc, so I imagine there may be more that aren't right. Yes, we should have an issue to fix them all, but I'm still happy with fixing this one in this issue since we are in that file already.

iyyappan.govind’s picture

iyyappan.govind
he/him
Primary language Tamil
Location Chennai, India
CreditAttribution: iyyappan.govind at UniMity Solutions Pvt Limited commented

Hi mikelutz,
Thanks for reviewing the patch.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed 9c171f8 and pushed to 8.8.x. Thanks!

  • alexpott committed 9c171f8 on 8.8.x 
    Issue #3083196 by iyyappan.govind, quietone, mikelutz: Move d7\...

Status: Fixed » Closed (fixed)

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