Convert assetEqual to assertIdentical in migrate_drupal

Benjy,

Is this something that we can have a newbie do? It doesn't seem like it is much more than a big search-and-replace...

-mike

I've been looking at this, seemed a good way to poke around.

What is the recommended way to compare arrays?

Line 136-138 of MigrateFieldInstanceTest.php states

 // This is the only way to compare arrays.
$this->assertFalse(array_diff_assoc($field->getSettings(), $expected));
$this->assertFalse(array_diff_assoc($expected, $field->getSettings()));

but that is the only case I found like that. Usually assertEqual is used.

Either you do that to compare two arrays -- but even that compares only arrays that do not contain arrays. Here, look at this:

function kindaIdentical($v1, $v2) {
  return $v1 === $v2 || (is_array($v1) && is_array($v2) && kindaIdenticalArrays($v1, $v2));
}
function kindaIdenticalArrays(array $a1, array $a2) {
  if (count($a1) !== count($a2)) {
    return FALSE;
  }
  foreach ($a1 as $k => $v) {
    if (!isset($a2[$k]) || !kindaIdentical($v, $a2[$k])) {
      return FALSE;
    }
  }
  return TRUE;
}

use this. Put it in the NestedArray class, I would say. It's kinda useful: ['foo' => 'bar', 'this' => 'that'] is kinda identical to ['this' => 'that', 'foo' => 'bar'] after all.

@chx, thanks. But if the arrays are identical and contain a NULL, kindaIdenticalArrays will return False because "isset() will return FALSE if testing a variable that has been set to NULL".

Since strings are returned, then shall we keep assertEqual be kept for testing integers and NULL?

It is unexpected to get a string back when when an int was loaded. Is this behavior documented?

> Is this behavior documented?

core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
98:      \PDO::ATTR_STRINGIFY_FETCHES => TRUE,

core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
114:      \PDO::ATTR_STRINGIFY_FETCHES => TRUE,

Kind of yes.

@chx, thanks. That lead to some interesting reading.

The attached patch changes assertEqual to assertIdentical only when both arguments are strings. But without some standard future tests could be written with assertEquals and not assertIdentical. This looks like a good case for an agreed approach and documentation. Is that suitable for a documentation issue?

Still need to address comment #8.

Oops, left one line using assertIdentical on ''.

Those failures seem to be the result of testing the empty string or NULL, not a TRUE/FALSE test. Anyway, MigrateNodeBundleSettingsTest.php has been changed to test integer to integer.

That just leaves 88 more to change and I should be able to do that over this weekend.

Another installment. This just leaves the arrays plus a few others that are not straightforward. I need to look at those when I am not as tired as I am now.

@benjy, I may take you up on that offer, after this busy weekend.

Managed to get some time and converted all but 3 assertEquals successfully.

And the interdiff, with a proper name.

Down to the last 3.

MigrateCckFieldValuesTest.php - Both failures relate to a multivalue field.
MigrateFieldInstanceTest.php - An array test with an empty string ('') that fails when assertIdentical is used.

This should be right.

MigrateFieldInstanceTest.php - No, it's not the empty string. I missed that the value is a float.

assertIdentical fails because the value being tested is either a float or a string, depending on which test is run. When MigrateCckFieldValuesTest is run by itself $planet_node->field_multivalue->value returns a string. When MigrateDrupal6Test is run $planet_node->field_multivalue->value returns a float. Why?

I think this it finished now. All but one assertEqual has been changed to assertIdentical. There are two cases which I guess should be addressed in separate issues.

1) field_multivalue is defined in two files, MigrateCckFieldValuesTest.php, as an integerl, and in Drupal6FieldInstance.php, as a decimal. When CckFieldValues test is run, it expects an integer casted as a string. But if the full d6 migration test is run first, it will create field_multivalue as a float with precision 2 and the returned string will be 'NN.00' not 'NN' as expected.

I changed MigrateCckFieldValuesTest.php to match the way field_multivalue is created in Drupal6FieldInstance.php and all tests pass, irregardless of the order. The allows the tests to pass but a better solution is to fix the duplication.

2) And a similar thing is going on with MigrateNodeTest.php at line 58. If run by itself $node_revision->revision_log->value is Null but when the complete set of tests is run $node_revision->revision_log->value is ''. The test on line 58 remains an assertEqual.

Patch not longer applies. So just a reroll.

@benjy, regarding comment #18 and #35.

What I meant was, previously it was:

$this->assertEquals(NULL, FALSE); // Passes

And I changed to this:
$this->assertIdentical(NULL, FALSE); // Fails

The assertIdentical fails because it does type checking. And since NULL is the only possible value of type null assertIdentical with anything other than a NULL will fail. Whereas. assertEqual does type juggling, NULL is converted to boolean FALSE and comparing FALSE to FALSE will pass.

So, to change any assertEqual with a NULL argument to an assertIdentical I changed NULL to match the type of the source. That seems to me a good way to do it, since in a migration one should know the source data well. Or am I missing your point?

regarding comment #34
Yea, wasn't sure how to tackle this one, so just tried this poor method. The property sounds like it would work and I'll look into it. But still confused why the source data is not consistent for these tests.

Oh, so you want to check all instances where NULL is used in an assert, all instances of assertNotNull, assertNull and when a parameter of an assert is tested for null, i.e.
$this->assertIdentical(is_null($form_display), FALSE, "Form display node.story.default loaded with config.");

Is that right?

We have assertNull and assertNotNull for those. Both simpletest and phpunit.

Thanks for the the clarification.
Of the assertions involving NULL, most I can verify are checking that something does not exist and should stay.

The following files are the only ones that have tests that need a more experienced eye on them.

MigrateCommentVariableEntityFormDisplaySubjectTest

$component = entity_get_form_display('comment', 'comment_no_subject', 'default')
->getComponent('subject');
$this->assertNull($component);

MigrateUploadEntityFormDisplayTest.php

$display = entity_get_form_display('node', 'article', 'default');
$component = $display->getComponent('upload');
$this->assertTrue(is_null($component));

MigrateUserProfileEntityDisplayTest.php and MigrateUserProfileEntityFormDisplayTest.php
$this->assertNull($display->getComponent('profile_sell_address')); $this->assertNull($display->getComponent('profile_sold_to'));

MigrateFieldTest.php
$this->assertEqual($settings['default_image']['fid'], '');

Plus this one we talked about on IRC

MigrateAggregatorFeedTest.php - remove the test on 'hash' and creating a new issue to determine if hash needs to be migrated or not.

I'm getting inconsistent test results since yesterday so uploading this patch.

Right, fixed MigrateNodeTest in attached patch. That leaves these two remaining instances of assertEqual.

1. MigrateFieldTest.php
   $this->assertEqual($settings['default_image']['uuid'], '');

   $settings['default_image']['uuid'] is not set. But I'm not sure if it is migrated.

2. MigrateUserTest.php
   $this->assertEqual($user->getSignatureFormat(), reset($signature_format));

I'd appreciate some suggestions on these two.

Looking at

$this->assertEqual($user->getSignatureFormat(), reset($signature_format));

Changing this to

$this->assertIdentical($user->getSignatureFormat(), reset($signature_format));

and running the MigrateUserTest results in two fails, both when the D6 signature format = 0 (UIDs 16 and 17).

Since $signature_format is set earlier in the test, can't we just check to see if it is an empty array and set it to null if it is? Then assertIdentical will work.

-mike

Rerolled, in preparation for returning home and working on this in the next few days.

Lets try this reroll

New patch including changes suggested in #46 and #51.

#46 - Removed test for uuid.
#51 - Since there is a new issue for the testing of signature_format that is restored that to an assertEqual.

There should be one fail. It seems that my comment in #45 was incorrect and MigrateNodeTest.php still has a problem.
$this->assertEqual($node_revision->revision_log->value, '');

Yes, I would, and I've looked at it a bit, but l have far too many questions about migration and D8 in general to do it. I just wind up spinning my wheels, as the saying goes.

Well, I've found two things:

1. The test node created in MigrateNodeTestBase has a revision_log value of NULL.

    // Create a test node.
    $node = entity_create('node', array(
      'type' => 'story',
      'nid' => 1,
      'vid' => 1,
    ));

But when I create a node via the GUI the revision_log value is blank, presumably an empty string.

2. The migration of the revision_log value works when the string is not empty.

Added code to make the revision_log a string. Works for MigrateNodeTest but having trouble (again) getting the complete migration tests to run.

Made a mistake rerolling.

Sigh, another reroll mistake.

Good. And grep doesn't find any instances of 'assertEqual'.

Thanks. I couldn't have done this with out your help.

It did fulfill my goal of a being a good way to poke around. I know more about migration now and have heaps more questions.

git ac https://www.drupal.org/files/issues/2345833-67.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  121k  100  121k    0     0   231k      0 --:--:-- --:--:-- --:--:--  231k
error: patch failed: core/modules/migrate_drupal/src/Tests/d6/MigrateBlockTest.php:85
error: core/modules/migrate_drupal/src/Tests/d6/MigrateBlockTest.php: patch does not apply

Reroll.

Great - I think the increased strictness is worth it. Migrate is not blocked by the beta. Committed b1d2d7c and pushed to 8.0.x. Thanks!