Current tests only account for 1:1 relationship between fields and instances.
We should test the case of several instances for a field.

Files: 
CommentFileSizeAuthor
#19 more_tests_for_field_config-1960574-19.patch15.81 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 56,278 pass(es).
[ View ]
#16 more_tests_for_field_config-1960574-16.patch16.88 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 55,049 pass(es).
[ View ]
#13 more_tests_for_field_config-1960574-13.patch16.86 KBsmiletrl
PASSED: [[SimpleTest]]: [MySQL] 55,939 pass(es).
[ View ]
#13 interdiff-10-13.txt8.79 KBsmiletrl
#11 more_tests_for_field_config-1960574-10.patch16.84 KBsmiletrl
FAILED: [[SimpleTest]]: [MySQL] 55,938 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#11 interdiff-9-10.txt12.73 KBsmiletrl
#9 1960574-field-cmi-more-config-tests-9.patch9.28 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 55,724 pass(es).
[ View ]
#9 interdiff.txt4.15 KBswentel
#7 1960574-field-cmi-more-config-tests.patch5.13 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 55,571 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Comments

yched’s picture

Status:Active» Postponed
swentel’s picture

Issue tags:+Field API

tagging

yched’s picture

Status:Postponed» Active

Un-postponing.

swentel’s picture

Actually, we test it for the body instance:

<?php
   
foreach (array('article', 'page') as $node_type) {
     
$config = config("field.instance.node.$node_type.body")->get();
?>

So I think we're good imo.

yched’s picture

That's just for the upgrade path.
I think @xjm was thinking of the config import tests.

swentel’s picture

right, duh, I need to learn to read titles.

swentel’s picture

Status:Active» Needs review
StatusFileSize
new5.13 KB
FAILED: [[SimpleTest]]: [MySQL] 55,571 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 1960574-field-cmi-more-config-tests.patch, failed testing.

swentel’s picture

Status:Needs work» Needs review
StatusFileSize
new4.15 KB
new9.28 KB
PASSED: [[SimpleTest]]: [MySQL] 55,724 pass(es).
[ View ]

Right, more fields, means more stuff to delete :)

smiletrl’s picture

Nice job!
Some nitpicks though:^

FieldImportCreateTest.php:

+    // A field with multiple instances.
+    $field_2 = entity_load('field_entity', $field_id);
+    $this->assertTrue($field, 'The second field was created.');

I think here should be

+    // A field with multiple instances.
+    $field_2 = entity_load('field_entity', $field_id_2);
+    $this->assertTrue($field_2, 'The second field was created.');
     // Check that the field and instance do not exist yet.
     $this->assertFalse(entity_load('field_entity', $field_id));
     $this->assertFalse(entity_load('field_instance', $instance_id));

This patch doesn't check $field_2 and $instance_2 before enabling field_test_config module.

It might be a good thing to add this multiple instances test in

  /**
   * Tests creating fields and instances during config import.
   */
  function testImportCreate() {

// Add the coresponding entries to the current manifest data.
This is not from the patch, but in the original code -- wrong word *corresponding*.

FieldImportDeleteTest.php:

+    $instance_id_2a = "test_entity.test_bundle.$field_id";
+    $instance_id_2b = "test_entity.test_bundle.$field_id_2";

I think you want $instance_id_2b to has a different bundle -- +      $instance_id_2b = "test_entity.test_bundle_2.$field_id_2"; and $instance_id_2a has a different field id -- $field_id_2:)

-    // Check that the config was correctly imported.
-    $field = entity_load('field_entity', $field_id);
-    $this->assertTrue($field, 'The field was created.');
-    $instance = entity_load('field_instance', $instance_id);
-    $this->assertTrue($instance, 'The field instance was created.');

Any particular reason to delete these checks? In this test file, it uses $this->installConfig(array('field_test_config')); to enable configuration, different from

module_enable(array('field_test_config'));

in FieldImportCreateTest.php. I think it's necessary to add this check and for new field and instances in this patch.

-    $field_uuid = $field->uuid;
+    // Get the uuid's for the fields.
+    $field_uuid = entity_load('field_entity', $field_id)->uuid();
+    $field_uuid_2 = entity_load('field_entity', $field_id)->uuid();

Could be $field_uuid_2 = entity_load('field_entity', $field_id_2)->uuid();, although I prefer this way $field->uuid^^

Finally, while this patch creates a new field and two based different instances, I guess a second instance with a different bundle based on current field in these tests will be okay:)

smiletrl’s picture

StatusFileSize
new12.73 KB
new16.84 KB
FAILED: [[SimpleTest]]: [MySQL] 55,938 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Patch updated according to #10.

For 'uuid' in staging yml file, I used following code to generate them. Not sure I did it correctly.

use Drupal\Component\Uuid\Uuid;
$ob = new Uuid();
$uuid = $ob->generate();
dsm($uuid);
$uuid = $ob->generate();
dsm($uuid);
$uuid = $ob->generate();
dsm($uuid);

Status:Needs review» Needs work

The last submitted patch, more_tests_for_field_config-1960574-10.patch, failed testing.

smiletrl’s picture

Status:Needs work» Needs review
StatusFileSize
new8.79 KB
new16.86 KB
PASSED: [[SimpleTest]]: [MySQL] 55,939 pass(es).
[ View ]

Fixed some bugs.

swentel’s picture

Status:Needs review» Needs work

Needs a reroll

error: patch failed: core/modules/field/lib/Drupal/field/Tests/FieldImportDeleteTest.php:68
error: core/modules/field/lib/Drupal/field/Tests/FieldImportDeleteTest.php: patch does not apply
YesCT’s picture

Issue tags:+Needs reroll

tagging.
also, instructions if someone new wants to try it: http://drupal.org/patch/reroll

swentel’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new16.88 KB
PASSED: [[SimpleTest]]: [MySQL] 55,049 pass(es).
[ View ]
swentel’s picture

Status:Needs review» Reviewed & tested by the community

This is good to go btw, mine was just a re-roll.

swentel’s picture

Status:Reviewed & tested by the community» Needs work

Manifests are gone as of #1998576: Make the config import process use full config trees again, so this needs work

swentel’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new15.81 KB
PASSED: [[SimpleTest]]: [MySQL] 56,278 pass(es).
[ View ]

And here's the reroll.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed 939a8a8 and pushed to 8.x. Thanks!

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