Add tests of row hash to trackChangesTest

What is adding migrate_map properties to the source?

I tested this on Drupal 9.4.x, standard install and was not able to reproduce the problem.

I used the Drupal 7 test fixture as the source and the d7_user_role migration, with track_changes: true added. I ran drush mim d7_user_role. I then used the debugger and checked the hash in Row::rehash() before and after mim --update d7_user_role.

Maybe not needed but I added assertions in TrackChangesTest to confirm the hash does not change when the source does not change.

Fix spelling error.

Remove an unneccesary change.

What is adding migrate_map properties to the source?

That's a great question. This can't be a core source plugin right?

As for the new assertions, it looks like there's some kind of copy-paste typo below:

+++ b/core/modules/migrate/tests/src/Kernel/TrackChangesTest.php
@@ -131,6 +145,16 @@ public function testTrackChanges() {
+    $this->assertNotEquals($original_hash[1], $new_hash[1]);
+    $this->assertEquals($original_hash[2], $new_hash[2]);
+    $this->assertNotEquals($original_hash[1], $new_hash[1]);
+    $this->assertNotEquals($original_hash[1], $new_hash[1]);

I'm thinking this was intended to be more like this:

$this->assertNotEquals($original_hash[1], $new_hash[1]);
$this->assertEquals($original_hash[2], $new_hash[2]);
$this->assertNotEquals($original_hash[3], $new_hash[3]);
$this->assertNotEquals($original_hash[4], $new_hash[4]);

I also have a question. The IS and comments refer to using the --update option when running drush mim, but the test obviously doesn't rely on drush. Do we need to add $id_map->prepareUpdate(); somewhere(s) in the test?

FWIW, the new assertion still pass if I throw add $id_map->prepareUpdate(); before the re-migration.

I'll set this to NW to fix that copy/paste typo, but then it should be good. Thanks!

@danflanagan8, thanks for the review. Good point about using prepareUpdate.

#8. It turns out the both 'Item 2' and 'Item 4' are modified so two of the hashes change. At the end of the test I added the prepareUpdate() and retested.

If we add the re-import after calling prepareUpdate the assertions would be more useful if they were all assertEquals. The assertNotEquals assertions are kind of funny here.

+++ b/core/modules/migrate/tests/src/Kernel/TrackChangesTest.php
@@ -144,6 +169,17 @@ public function testTrackChanges() {
+
+    // Test hashes again after forcing all rows to be re-imported.
+    $id_map->prepareUpdate();
+    for ($i = 1; $i < 5; $i++) {
+      $row = $id_map->getRowBySource(['tid' => $i]);
+      $new_hash[$i] = $row['hash'];
+    }
+    $this->assertNotEquals($original_hash[1], $new_hash[1]);
+    $this->assertEquals($original_hash[2], $new_hash[2]);
+    $this->assertNotEquals($original_hash[3], $new_hash[3]);
+    $this->assertEquals($original_hash[4], $new_hash[4]);

Maybe this last foreach could store the has in a variable called $newer_hash such that each of the assertions would look like $this->assertEquals($new_hash[#], $newer_hash[#]);.

Well, that was a real oops. I did mean to execute the migration again after the prepareUpdate.

#10. Done. I agree, it reads better.

Double oops: I hadn't even noticed the migration didn't get run again after preparing for update.

The test certainly looks good now. So what do we do here? You've proven the reported bug is not a problem in core. Do we change this to a task for adding coverage related to the hash?

I think it's reasonable coverage to add. The only place I can find explicit coverage related to hashing is in Drupal\Tests\migrate\Unit\RowTest. The new coverage is testing for unexpected interaction between trackChanges and prepareUpdate, which seems ok to add.

I'm going to update the IS and category and RTBC.

How about changing the title to what this is actually doing.

Committed and pushed be8ec278470 to 10.0.x and 83394db7aa6 to 9.4.x and 7bd674b5249 to 9.3.x. Thanks!

Backported to 9.3.x as additional test coverage is always welcome.