Problem/Motivation

There are several Migration Kernel test files that run setup twice but do not need to. The only difference in the tests are the assertions run after the migration. There is no need to do the setup more than once. The migration can be run once and then all the assertions

This was already done in #3165763: Combine two tests to one in d7 MigrateFieldTest and MigrateFieldInstance. This issue does the same for the other migration tests, of which there are 10.

Also, an annoyance factor when working with those tests.

Proposed resolution

Combine the assertions

Remaining tasks

Patch
Review
Commit

CommentFileSizeAuthor
#3 3192900-3.patch9.6 KBquietone
#2 3192900-2.patch9.6 KBquietone

Issue fork drupal-3192900

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quietone created an issue. See original summary.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Active » Needs review
Issue tags: +Novice
FileSize
3192900-2.patch9.6 KB

Assuming this passes test, the review here is straightforward and suitable for a novice.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
FileSize
3192900-3.patch9.6 KB

As soon as I uploaded I realized I hadn't run commit-code-check.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Priority: Normal » Minor

Matroskeen made their first commit to this issue’s fork.

Matroskeen opened merge request !277

Matroskeen’s picture

Matroskeen
Primary language Ukrainian
Location 🇺🇦 Ukraine, Lutsk
CreditAttribution: Matroskeen at DevBranch, Drupal Ukraine Community commented
Status: Needs review » Needs work

The patch needed a re-roll after #2867959: Replace usages of deprecated AssertLegacyTrait::assertIdentical, so I created a merge request based on patch #3.

I prepared some numbers to see why these changes are good:
core/modules/comment/tests/src/Kernel/Migrate/d7/MigrateCommentTest.php
Before the patch: 1.12 minutes
After: 33.21 seconds

core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserTest.php
Before the patch: 59.59 seconds
After: 34.75 seconds

I reviewed other similar migrations and I think it might be possible to optimize at least some of these:

  • core/modules/taxonomy/tests/src/Kernel/Migrate/d6/MigrateTermNodeComplete.php
  • core/modules/language/tests/src/Kernel/Migrate/d6/MigrateLanguageContentSettingsTest.php
  • core/modules/user/tests/src/Kernel/Migrate/d6/MigrateUserPictureD6FileTest.php

Moving to NW for that.

Matroskeen’s picture

Matroskeen
Primary language Ukrainian
Location 🇺🇦 Ukraine, Lutsk
CreditAttribution: Matroskeen at DevBranch, Drupal Ukraine Community commented
quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented

Thanks for putting some numbers on this. It is nice to see the improvement.

I am not surprised there are more. I count 31 migrate kernel tests with more that one test method.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs work » Needs review
Matroskeen’s picture

Matroskeen
Primary language Ukrainian
Location 🇺🇦 Ukraine, Lutsk
CreditAttribution: Matroskeen at DevBranch, Drupal Ukraine Community commented
Status: Needs review » Reviewed & tested by the community

Looks good to me, thanks!

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Issue summary: View changes

Your welcome.

alexpott’s picture

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

Committed 82c63a2 and pushed to 9.2.x. Thanks!

  • alexpott committed 82c63a2 on 9.2.x 
    Issue #3192900 by quietone, Matroskeen: Combine some migration kernel...

Matroskeen closed merge request !277

Status: Fixed » Closed (fixed)

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