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
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | 2914668-7.patch | 1.4 KB | jofitz |
| #7 | interdiff-2-7.txt | 1.34 KB | jofitz |
| #2 | 2914668-2.patch | 945 bytes | shashikant_chauhan |
Comments
Comment #2
shashikant_chauhan commentedAdding patch for it.
Comment #3
heddnWe'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.
Comment #4
quietone commentedYes, I got that wrong about the assertion.
Comment #5
gábor hojtsyIs 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.
Comment #6
heddnThe 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.Comment #7
jofitzAs suggested by @heddn in #6: edited the test to assert that node module is enabled and $migrations is empty.
Comment #8
heddnBack to RTBC.
Comment #11
gábor hojtsyThanks makes a lot of sense!