Problem/Motivation

Was looking for how to write a test and I looked at D6NodeDeriverTest/D7NodeDeriverTest and noticed something odd.

Both contain 2 tests. They test if a node_translation migration is created with and without the content_translation and language modules installed. Seems simple enough. But there is an important difference. For d6, the node module is installed for both tests. For d7 the node module is only installed when content_translation and language are installed. This means that the test for ensuring that no node_translation migration are generated without content_translation and language isn't running the node deriver at all, thus the test isn't doing what it should. That test needs to install the node module.

Went back to the original issue #2669964: Migrate Drupal 7 core node translations to Drupal 8 and found that I did it. In #20, I said "I've pretty much copy/pasted the D6 node translation code to D7." And then got the test working without fully understanding what was going on.

The fix should be straight forward so I am marking as Novice.

Proposed resolution

Modify the d7 MigrateNodeDeriverTest::testNoTranslations() to install the node module.
Modify the assertion to use $this->assertArrayNotHasKey(). Something like this

$this->assertArrayHasNoKey('d7_node_translation:article', $migrations,
      "No node translation migrations without content_translation"");

Remaining tasks

Write a patch
Review it
Commit it

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Comments

quietone created an issue. See original summary.

shashikant_chauhan’s picture

Status: Active » Needs review
StatusFileSize
new945 bytes

Adding patch for it.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

We've enabled the node module and we've got a test passing green now. But I don't see that assertArrayNotHasKey is any better than assertEmpty vs not having the 'node' module even enabled. It is going to return empty instances. But I guess that's what we have reviewers for. And this seems to be correct now. So RTBC.

quietone’s picture

Yes, I got that wrong about the assertion.

gábor hojtsy’s picture

Status: Reviewed & tested by the community » Needs review

Is the array going to be empty either way with or without node module due to requesting only translation migrations? I agree we should enable the node module here since the test is testing if the node module only makes node translation migrations (which it does not). But not sure either of the assertion change.

heddn’s picture

Status: Needs review » Needs work

The assertion is empty in either case. I ran the test through xdebug and we do in fact enter the code now and we are getting back the correct result. But the assertion is pointless since it doesn't tell us much. We'd be better off asserting that node module is enabled and asserting empty on migrations.

jofitz’s picture

Status: Needs work » Needs review
StatusFileSize
new1.34 KB
new1.4 KB

As suggested by @heddn in #6: edited the test to assert that node module is enabled and $migrations is empty.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

  • Gábor Hojtsy committed ccde72d on 8.5.x
    Issue #2914668 by Jo Fitzgerald, shashikant_chauhan, heddn, quietone:...

  • Gábor Hojtsy committed e9c1990 on 8.4.x
    Issue #2914668 by Jo Fitzgerald, shashikant_chauhan, heddn, quietone:...
gábor hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Thanks makes a lot of sense!

Status: Fixed » Closed (fixed)

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