Postponed on #3265483: Handle block migration for modules moved to contrib and #3265424: Move migrate related aggregator tests to the module in preparation of removal in d10

Problem/Motivation

Follow up to #3265483: Handle block migration for modules moved to contrib to change a test to use the aggregator test fixture and not the one in migrate_drupal.

Steps to reproduce

Proposed resolution

in \Drupal\Tests\aggregator\Kernel\Migrate\d6\MigrateBlockTest remove use Drupal\Tests\migrate_drupal\Kernel\d6\MigrateDrupal6TestBase; so that \Drupal\Tests\aggregator\Kernel\Migrate\d6\MigrateDrupal6TestBase is used which means that \Drupal\Tests\aggregator\Kernel\Migrate\d6\MigrateDrupal6TestBase::getFixtureFilePath will load the path to the fixture in aggregator module.

Update $modules to only those needed for this test, noting that aggregator is installed in the base class.

Remaining tasks

Patch
Review
Commit

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

CommentFileSizeAuthor
#13 3267274-13.patch896 bytesquietone
#13 interdiff.txt514 bytesquietone
#4 3267274-3.patch913 bytesquietone

Comments

quietone created an issue. See original summary.

quietone’s picture

quietone commented
Issue summary: View changes
Status: Active » Postponed

Adding postponed details.

daffie’s picture

daffie commented
Status: Postponed » Active

#3265424: Move migrate related aggregator tests to the module in preparation of removal in d10 and #3265483: Handle block migration for modules moved to contrib have landed.

quietone’s picture

quietone commented
Status: Active » Needs review
StatusFileSize
new 3267274-3.patch913 bytes

Removed the @todo and updated the $modules list because \Drupal\Tests\migrate_drupal\Kernel\d6\MigrateDrupal6TestBase enables modules that \Drupal\Tests\aggregator\Kernel\Migrate\d6\MigrateDrupal6TestBase does not. The test now only installs the modules it needs.

The patch applies to 9.4.x an 10.

daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 4: 3267274-3.patch, failed testing. View results

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs work » Reviewed & tested by the community

Test fails are known randoms, so I sent a retest.

Setting back to RTBC to restore status -- I have not reviewed this myself yet.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

The title and IS here do not describe what's currently happening in the patch; could we update them to explain the current approach? Thanks!

quietone’s picture

quietone commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update

The patch is changing the fixture that is loaded in the test. Added more to the IS.

xjm’s picture

xjm
she/her
Primary language English
commented
Title: Use aggregator fixture in d6/MigrateBlockTest » Use aggregator fixture instead of migrate_drupal fixture in d6/MigrateBlockTest
quietone’s picture

quietone commented

Right, what is obvious to me as a migrate maintainer is not obvious to others.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented

@quietone, I took a look at the MigrateDrupal6TestBase from migrate_drupal and I didn't see taxonomy as one of the $modules.

When I apply the patch and then remove the line that adds taxonomy the test still passes.

Is there something I'm missing? Or should we update the patch so that we don't enable taxonomy?

quietone’s picture

quietone commented
StatusFileSize
new interdiff.txt514 bytes
new 3267274-13.patch896 bytes

@danflanagan8, thanks!

But Grr! I recall working on something else at the same time that needed taxonomy and I seemed to have mixed them up. Yes, let's remove it.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Status: Needs review » Reviewed & tested by the community

Thanks, @quietone. This one's ready to commit as far as I can tell.

  • catch committed 8de366f on 10.0.x 
    Issue #3267274 by quietone, xjm, danflanagan8: Use aggregator fixture...

  • catch committed d56f0b4 on 9.4.x 
    Issue #3267274 by quietone, xjm, danflanagan8: Use aggregator fixture...
catch’s picture

catch
he/him
Primary language English
commented
Version: 10.0.x-dev » 9.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 10.0.x and cherry-picked to 9.4.x, thanks!

Status: Fixed » Closed (fixed)

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