It just struck me while working on the node revision translations that the testing of revisions post migration was a limited to one node in both the d6 and d7 tests. I thought it a good idea to test all the revisions.

CommentFileSizeAuthor
#2 3079298-2.patch8.2 KBquietone
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
FileSize
3079298-2.patch8.2 KB

And a patch

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Active » Needs review
mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at ICON Interactive commented
Status: Needs review » Reviewed & tested by the community

Always a fan of more tests. :-) These look good.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Needs review 
+++ b/core/modules/node/tests/src/Kernel/Migrate/d6/MigrateNodeRevisionTest.php
@@ -20,6 +29,32 @@ class MigrateNodeRevisionTest extends MigrateNodeTestBase {
+  protected function assertRevision($id, $langcode, $title, $log, $timestamp) {

+++ b/core/modules/node/tests/src/Kernel/Migrate/d7/MigrateNodeRevisionTest.php
@@ -0,0 +1,129 @@
+  protected function assertRevision($id, $langcode, $title, $log, $timestamp) {

this looks duplicated - could it go in a trait or base class?

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented

@larowlan, yes they could and I have thought about that more than once while working on this patch and other Migrate Kernel tests. I choose not to do it here because of precedence, we haven't been making a base class or trait for asserting an entity. The other reason is that I'd prefer that anything we do to rationalize the testing be it's own meta where we take a moment to discuss how to do it for all entities. That way we all agree and then many of the children of that can probably be a novice issue as well.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented

Made an issue to consider creating common assert entity methods got the migration kernel tests. #3080745: Create common assert entity methods for migration tests

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Status: Needs review » Reviewed & tested by the community

+1 on #6. Follow-up is opened. Back to RTBC.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Fixed

Happy with a followup

Committed d2116d6 and pushed to 8.8.x. Thanks!

  • larowlan committed d2116d6 on 8.8.x 
    Issue #3079298 by quietone: Add more tests of node revisions

Status: Fixed » Closed (fixed)

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