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
| Comment | File | Size | Author |
|---|---|---|---|
| #20 | 2904290-20.patch | 6.26 KB | heddn |
| #17 | interdiff-16-17.txt | 1.47 KB | bramdriesen |
| #17 | rename_migration_tests-2904290-17.patch | 27.21 KB | bramdriesen |
| #16 | rename_migration_tests-2904290-16.patch | 27.21 KB | bramdriesen |
| #4 | rename_migration_tests-2904290-4.patch | 177.67 KB | bramdriesen |
Comments
Comment #2
karthikkumarbodu commentedFixed issue tag
Comment #3
ritzz commentedComment #4
bramdriesenAttached 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.
Comment #5
quietone commented@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".
Delete trailing whitespace.
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.
Same here. This isn't a migration test and shouldn't be renamed.
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.
Same here. This shouldn't be renamed.
The IS has been updated to identify the file names that are not to be renamed.
Comment #6
karthikkumarbodu commentedComment #8
bramdriesenWill reapply the patch now.
Comment #9
bramdriesenPatch and interdiff attached. Thanks for the diff -M tip :) didn't know that.
Comment #10
bramdriesenDoh, realised I removed the wrong whitespace :) new patch and interdiff uploaded. Ignore the previous one.
Comment #11
bramdriesenComment #12
heddnAfter #2903078: Consistent use of @group in tests, I think this needs a reroll.
Comment #13
bramdriesenWill do a re-roll for this.
Comment #14
karthikkumarbodu commentedDid re-roll and uploaded the new patch file.
Comment #16
bramdriesenRe-rolled patch attached. Interdiff wasn't really possible anymore since the previous patch didn't apply.
Comment #17
bramdriesenNoticed two accidental enters I've added. Removed those.
Comment #18
bramdriesenComment #19
quietone commentedGreat! Just one more thing, the plugin tests don't need a rename.
These don't need a rename because they are not full migration tests. They test a source plugin.
Comment #20
heddnSo, 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.
Comment #21
heddnComment #22
quietone commentedYea, I reckon you are right. It is a personal preference.
Comment #24
quietone commentedThanks everyone!
@BramDriesen thanks for persisting with this despite my ongoing changes to the IS.
Comment #25
bramdriesenHehe no problem :) glad we found a better way to standardise it.