Problem/Motivation

The migration tests names don't all follow the same pattern. Some begin with Migrate and some don't.

The files in paths, tests/src/Kernel/Migrate are to be renamed.
The files in commerce_migrate/test and commerce_migrate/modules/woocommerce are not to be renamed.

Why? For consistency. So, when you look at just the filename it is clear what it is does. So, it is not confused with source plugin tests. To brighten my day.....

Proposed resolution

Rename migration tests, the tests that run a full migration before making assertions, in commerce_migrate to start with Migrate. These tests are in the path 'tests/src/Kernel/Migrate'. For example, rename ProductVariationTest.php to MigrateProductVariationTest.php.

Remaining tasks

Do it

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Comments

quietone created an issue. See original summary.

karthikkumarbodu’s picture

Issue tags: -novie +Novice

Fixed issue tag

ritzz’s picture

Assigned: Unassigned » ritzz
bramdriesen’s picture

Assigned: ritzz » Unassigned
Status: Active » Needs review
StatusFileSize
new177.67 KB

Attached a quick patch which renames all the tests to Append "Migrate". Nothing structural changed to the tests (only the class names) so tests should still pass.

quietone’s picture

Issue summary: View changes
Status: Needs review » Needs work

@BramDriesen, this quick patch looks good, just a few things. Most of the errors reflect that I should have added more detail to the Issue Summary, sorry about that.

Please make the patch with, 'git diff -M', so the renames are recognized as renames and the patch size is reduced. That will make it a lot easier to review. It's mentioned in the Advanced patch contributor guide in point 6 under section "Creating patches via topic branch".

  1. +++ b/tests/src/Kernel/MigrateCommerceMigrateTestTrait.php
    @@ -0,0 +1,443 @@
    +  ¶
    

    Delete trailing whitespace.

  2. +++ b/tests/src/Kernel/MigrateCommerceMigrationLabelExistTest.php
    @@ -0,0 +1,65 @@
    +class MigrateCommerceMigrationLabelExistTest extends MigrateDrupalTestBase {
    

    Ah, this doesn't need to be renamed, it isn't a migration test. It just creates the migrations and then asserts that the label exists.

  3. +++ b/tests/src/Kernel/MigrateCommerceMigrationProvidersExistTest.php
    @@ -0,0 +1,79 @@
    +class MigrateCommerceMigrationProvidersExistTest extends MigrateDrupalTestBase {
    

    Same here. This isn't a migration test and shouldn't be renamed.

  4. +++ b/modules/woocommerce/tests/src/Kernel/MigrateWoocommerceTestBase.php
    @@ -0,0 +1,49 @@
    +abstract class MigrateWoocommerceTestBase extends MigrateTestBase {
    

    Sorry, you wouldn't know this but this shouldn't be renamed either. The woocommerce code won't work as it is and will be removed soonish.

  5. +++ b/tests/src/Kernel/MigrateCommerceMigrateTestTrait.php
    @@ -0,0 +1,443 @@
    +trait MigrateCommerceMigrateTestTrait {
    

    Same here. This shouldn't be renamed.

The IS has been updated to identify the file names that are not to be renamed.

karthikkumarbodu’s picture

Status: Needs work » Needs review
StatusFileSize
new22.86 KB

Status: Needs review » Needs work
bramdriesen’s picture

Assigned: Unassigned » bramdriesen

Will reapply the patch now.

bramdriesen’s picture

Status: Needs work » Needs review
StatusFileSize
new26.65 KB
new24.87 KB

Patch and interdiff attached. Thanks for the diff -M tip :) didn't know that.

bramdriesen’s picture

StatusFileSize
new26.81 KB
new599 bytes

Doh, realised I removed the wrong whitespace :) new patch and interdiff uploaded. Ignore the previous one.

bramdriesen’s picture

Assigned: bramdriesen » Unassigned
heddn’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

After #2903078: Consistent use of @group in tests, I think this needs a reroll.

bramdriesen’s picture

Assigned: Unassigned » bramdriesen

Will do a re-roll for this.

karthikkumarbodu’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new22.07 KB

Did re-roll and uploaded the new patch file.

Status: Needs review » Needs work
bramdriesen’s picture

Status: Needs work » Needs review
StatusFileSize
new27.21 KB

Re-rolled patch attached. Interdiff wasn't really possible anymore since the previous patch didn't apply.

bramdriesen’s picture

StatusFileSize
new27.21 KB
new1.47 KB

Noticed two accidental enters I've added. Removed those.

bramdriesen’s picture

Assigned: bramdriesen » Unassigned
quietone’s picture

Issue summary: View changes
Status: Needs review » Needs work

Great! Just one more thing, the plugin tests don't need a rename.

+++ b/modules/ubercart/tests/src/Kernel/Migrate/d7/MigrateUbercart7TestBase.php
similarity index 97%
rename from modules/ubercart/tests/src/Kernel/Plugin/migrate/source/d6/BillingProfileTest.php

+++ b/modules/ubercart/tests/src/Kernel/Plugin/migrate/source/d6/MigrateBillingProfileTest.php
similarity index 97%
rename from modules/ubercart/tests/src/Kernel/Plugin/migrate/source/d6/OrderProductCurrencyTest.php

+++ b/modules/ubercart/tests/src/Kernel/Plugin/migrate/source/d6/MigrateOrderProductCurrencyTest.php
similarity index 97%
rename from modules/ubercart/tests/src/Kernel/Plugin/migrate/source/d6/OrderProductNoCurrencyTest.php

These don't need a rename because they are not full migration tests. They test a source plugin.

heddn’s picture

Status: Needs work » Needs review
StatusFileSize
new6.26 KB

So, after looking at the IS and the feedback in #19, I think I prefer that we don't prefix all the classes with the word 'Migrate'. It is already in the namespace, so we make the names longer. With that in mind, here's another approach at standardizing class names.

Basically I remove the prefixed 'Migrate' from MigrateFoo where-ever I found it.

heddn’s picture

Title: Rename migration tests to prepend Migrate » Standardize migration tests to not include prepended Migrate
quietone’s picture

Status: Needs review » Reviewed & tested by the community

Yea, I reckon you are right. It is a personal preference.

  • quietone committed 2f3fd03 on 8.x-2.x authored by heddn
    Issue #2904290 by BramDriesen, karthikkumarbodu, heddn: Standardize...
quietone’s picture

Status: Reviewed & tested by the community » Fixed

Thanks everyone!
@BramDriesen thanks for persisting with this despite my ongoing changes to the IS.

bramdriesen’s picture

Hehe no problem :) glad we found a better way to standardise it.

Status: Fixed » Closed (fixed)

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