Problem/Motivation

Now that work has begun in earnest on the D7-D8 upgrade path, I think that automated tests for D6-D8 should be in their own test group, distinct from the automated tests for D7-D8. This is just a minor organizational thing, but there will be well over 100 automated tests when both upgrade paths are completed so it seems to make sense to keep things organized.

Proposed Resolution

Divide migrate_drupal's automated test suite (for the actual migrations, not plugin unit tests) into two groups: "migrate_drupal_6.x" and "migrate_drupal_7.x".

Remaining Tasks

^ That.

API & UI Changes

None.

CommentFileSizeAuthor
#11 2495271-11.patch65.24 KBphenaproxima
#9 interdiff-2495271-8-9.txt1.13 KBphenaproxima
#9 2495271-9.patch65.47 KBphenaproxima
#8 interdiff-2495271-5-8.txt15.56 KBphenaproxima
#8 2495271-8.patch66.76 KBphenaproxima
#5 2495271-5.patch45.6 KBquietone
#5 interdiff-2495271-1-5.txt43.16 KBquietone
#1 2495271-1.patch45.6 KBphenaproxima
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima commented
Status: Active » Needs review
FileSize
2495271-1.patch45.6 KB

It is a patch.

cilefen’s picture

cilefen CreditAttribution: cilefen commented

Could spaces in the group names present a problem anywhere?

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima commented

Not as far as I know; it works OK for me when using the SimpleTest UI. I don't know how, or if, run-tests.sh handles this.

chx’s picture

chx CreditAttribution: chx commented

While there's no explicit mention of spaces support in the annotation parser this https://phpunit.de/manual/current/en/appendixes.annotations.html#appendi... very heavily indicates the parser will pick them up and from there, it's a free ride, I guess.

It'd be easy to test from command line, cd core; phpunit --group ...

quietone’s picture

quietone CreditAttribution: quietone commented
FileSize
interdiff-2495271-1-5.txt43.16 KB
2495271-5.patch45.6 KB

Yes, the space does make a difference. For runtests.sh if a group name has a space it needs to be quoted. Without the space it was running 'migrate_drupal' tests, which is expected.

Attached patch changes the space to an underscore.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima commented
Status: Needs review » Reviewed & tested by the community

Looks great to me.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Needs review

I asked @phenaproxima if there was any reason not to also use a so-named group for the unit tests, e.g. core/modules/migrate_drupal/tests/src/Unit/source/d6/NodeTypeTest.php. It's mentioned in the summary, but not why. He said he didn't remember a specific reason not to.

So setting NR to discuss that. :)

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima commented
Issue summary: View changes
FileSize
2495271-8.patch66.76 KB
interdiff-2495271-5-8.txt15.56 KB

@xjm raises a great point. There's no particular reason why the source unit tests should not have the same group. This patch fixes that, and removes a few defunct test classes. Because you know what they say about cleanliness.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima commented
FileSize
2495271-9.patch65.47 KB
interdiff-2495271-8-9.txt1.13 KB

Whoops! Accidentally included some cruft in #8.

benjy’s picture

benjy CreditAttribution: benjy at CodeDrop commented 
+++ b/core/modules/migrate_drupal/tests/src/Unit/source/d6/ViewModeTest.php
@@ -12,7 +12,7 @@
+ * @group migrate_drupal_6.x

Does the ".x" give us anything, why not just have migrate_drupal_6/7 ?

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima commented
FileSize
2495271-11.patch65.24 KB

@benjy: Point. We don't gain anything by the ".x", it was just a style thing. I have changed it to "migrate_drupal_6" in this patch. Fewer keystrokes are better keystrokes!

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima commented
Priority: Minor » Major
Issue tags: +blocker, +Migrate critical

This is now a migrate-critical blocker of #2533886: [meta] Move module-specific migration support into the particular modules supported.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima commented
Priority: Major » Minor
Status: Needs review » Closed (won't fix)
Issue tags: -blocker, -Migrate critical

Never mind. I have found a temporary hack to work around the blocking problem, so I'll move ahead with that instead rather than causing an pileup. #2533886: [meta] Move module-specific migration support into the particular modules supported pretty much renders this issue obsolete anyway.