Now we have test coverage for the very simplest case which is a single, self-contained configuration value: site slogan.

However, there are many more complicated cases than that.

Suggestion: expand this test to also include the following scenario:

- Create content type.
- Create a field on the content type.
- Deploy.
- Assert that content type and field are found.
- Delete the field and re-create it.
- Deploy.
- Other edge cases?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

we need to cover installing/uninstalling modules, but i'll add that over in #1808248: Add a separate module install/uninstall step to the config import process.

which could use some reviews, hint hint.

alexpott’s picture

cilefen’s picture

Status: Active » Needs review
FileSize
3.27 KB

This is the content type only. If this looks good, somebody let me know and I will add the field.

cilefen’s picture

This patch, adds a field and attaches it to the content type, however $web_user gets a permission denied trying to access node/{type}/add after importing the configuration and I don't know why.

Status: Needs review » Needs work

The last submitted patch, 4: import-export-2108813-4.patch, failed testing.

cilefen’s picture

cilefen’s picture

Status: Needs work » Needs review
Anonymous’s picture

Status: Needs review » Needs work

yay for this patch.

however, the assumptions it makes are not quite right.

+    // Delete the content type we added earlier.
+    $this->drupalPostForm('admin/structure/types/manage/' . $this->content_type->type . '/delete', array(), 'Delete');
+    $type_exists = (bool) entity_load('node_type', $this->content_type->type);
+    $this->assertFalse($type_exists, 'The new content type has been deleted.');

except for properties set on $this, nothing persists between testExport() and testImport(). so, instead of deleting here, assert that the content type and field don't exist before the import.

cilefen’s picture

Done. Is it actually necessary to verify the content type and field do not exist before import?

cilefen’s picture

Status: Needs work » Needs review
Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

re #9, yes, i think asserting they don't exist is useful.

mtift’s picture

Looks good to me. Tests complete locally. Minor nitpick: the attached patch just cleans up 14 lines that introduced whitespace errors.

cilefen’s picture

@mtift I'm usually a stickler for that kind of thing. Thanks.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Great work, cilefen! :D This patch makes me very happy.

I'd be fine committing this patch as-is, but one of the things I was going for with these expanded tests was some of the edge cases with fields that we keep inventing different workarounds for in CMI. That's why the issue summary said to delete the field after creating it and test that scenario as well. Any way to expand the tests to cover that, or should we just go with this and do so in a follow-up? (I'm fine either way.)

cilefen’s picture

@webchick I don't understand Drupal CMI enough to know what "deploy" means in the issue summary. Can you point me to some info?

webchick’s picture

Sorry, when I say "deploy" that's short-hand for "call the doImport() function" :)

cilefen’s picture

This patch attempts to do the events in the order asked-for in the issue summary. It throws an exception on the final doImport:

Drupal\field\FieldException: Attempt to create an instance of unknown field 490d9e9f-e883-4ad1-b61d-297e3b119220 in Drupal\field\Entity\FieldInstance->__construct() (line 254 of /Applications/MAMP/htdocs/drupal8/core/modules/field/lib/Drupal/field/Entity/FieldInstance.php).

I don't know whether this is a problem with what I've done (totally possible), or an issue with CMI.

The last submitted patch, 17: import-export-2108813-17.patch, failed testing.

cilefen’s picture

Let's try that again. See comment #17.

The last submitted patch, 19: import-export-2108813-19.patch, failed testing.

webchick’s picture

Status: Needs review » Needs work

Testbot. :\

cilefen’s picture

It fails for me, too, but I don't know why.

cilefen’s picture

I just re-ran the test on my laptop and the problem is the same--after deleting the field, then re-adding the field, and on the last import attempt, this error is returned to the browser:

Drupal\field\FieldException: Attempt to create an instance of unknown field 179b515b-f2bb-4cc0-9000-c2732cfd5715 in Drupal\field\Entity\FieldInstance->__construct() (line 254 of /Applications/MAMP/htdocs/drupal8/core/modules/field/lib/Drupal/field/Entity/FieldInstance.php).

cilefen’s picture

I ran the steps manually and I get the same error in the same place.

mtift’s picture

Berdir’s picture

  1. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigExportImportUITest.php
    @@ -70,11 +84,43 @@ protected function setUp() {
    +
    ...
    +
    +    // Create a content type.
    +    $this->content_type = $this->drupalCreateContentType();
    +    $type_exists = (bool) entity_load('node_type', $this->content_type->type);
    +    $this->assertTrue($type_exists, 'The new content type has been created in the database.');
    

    I don't think we need to test here that drupalCreateContentType() works. the (bool) is also a bit strange, if anything, I'd do an assertTrue(!empty($type)), but I'd just leave that out.

  2. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigExportImportUITest.php
    @@ -85,28 +131,92 @@ function testExport() {
    +    $field_instances = entity_load_multiple('field_instance');
    +    foreach($field_instances as &$field_instance) {
    +      if($field_instance->field_name == $this->field->name) {
    +        $field_instance->delete();
    +      }
    +    }
    +    $fields = entity_load_multiple('field_entity');
    +    foreach($fields as &$field) {
    +      if($field->name == $this->field->name) {
    +        $field->delete();
    +      }
    +    }
    

    by reference (&) is not necessary when working with objects. Especially as you're not changing anything. Also missing spaces between if/foreach and (.

Not surprised this isn't working, that's a scary case ;) We should probably get @yched in here. I'm not even sure what it should try to do exactly, but my guess would be that it's some sort of conflict/order problem when those fields and instances are created/deleted. Just for pinning down the exact problem, does it work if create the new field with a different name?

cilefen’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 25: import-export-2108813-25.patch, failed testing.

The last submitted patch, 25: import-export-2108813-25.patch, failed testing.

webchick’s picture

Heh. Ok, let's go back to the first working patch then, and cover this expanded test case in a follow-up. The first patch does a good job of laying groundwork so the next time we jigger UUIDs or what have you we can add test coverage for the actual bug we're trying to fix.

So let's get a re-roll of #12 w/ the feedback from Berdir in #26 incorporated.

mtift’s picture

Status: Needs work » Needs review
FileSize
4.35 KB
2.82 KB

This patch should address the comments in #26 and #30. All of the tests work locally.

star-szr’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Tagging for reroll.

InternetDevels’s picture

Reroll. Removed unused variable $admin_role and added some doc to getInfo() and setUp().

star-szr’s picture

Status: Needs review » Needs work

Thanks @InternetDevels, we lost a bunch of assertions from what I can see so this needs another look.

cilefen’s picture

Status: Needs work » Needs review
FileSize
3.58 KB

This is essentially what we were trying in #12 with the suggestions from #26 included.

xjm’s picture

FileSize
2.7 KB

The test has been completely rewritten, so we need to just add the new test data and assertions into it.

xjm’s picture

Oops, crosspost. #35 has more coverage so go with that. :)

tim.plunkett’s picture

Is #2108809: Make config import/export tests pass with standard profile a truly separate issue and critical in its own right?

alexpott’s picture

Issue tags: -beta blocker +beta target

Changing to beta target since this is not actually going to make API changes :)

mtift’s picture

35: import-export-2108813-35.patch queued for re-testing.

mtift’s picture

Status: Needs review » Reviewed & tested by the community

Patch from #35 applies cleanly. Code looks good. Removes a few unused variables. Covers each of the items in the issue summary. Test complete locally. If testbot agrees, I'd say this is good to go.

The last submitted patch, 35: import-export-2108813-35.patch, failed testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Not green

martin107’s picture

Status: Needs work » Reviewed & tested by the community

#36 is green ... Its not easy being green.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Yes but according to #41 the rtbc was for the patch in #35 and #35 has more test coverage than #36.

Désiré’s picture

Assigned: Unassigned » Désiré
Désiré’s picture

Status: Needs work » Needs review
FileSize
4.07 KB

I have rerolled the patch #35, fixed the warnings, and added documentation.

jessebeach’s picture

Status: Needs review » Reviewed & tested by the community

#47 has the expanded test coverage from #35.

The tests cover the use cases outlined in the issue summary by webchick, namely:

  • - Create content type.
  • - Create a field on the content type.
  • - Deploy.
  • - Assert that content type and field are found.
  • - Delete the field and re-create it.
  • - Deploy.

The warnings have been addressed by enabling the node and field modules in the test setup. We're good to go here with this patch.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Excellent work, all!! This is great, and something we can continue to build from for fancier edge-cases as we find them.

Committed and pushed to 8.x. Thanks!

  • Commit 23edb3f on 8.x by webchick:
    Issue #2108813 by cilefen, mtift, Désiré, xjm, InternetDevels: Add...

Status: Fixed » Closed (fixed)

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