Problem

  1. FieldUnitTestBase::createFieldWithInstance() has a first $suffix argument, which is used to dynamically create non-existing properties on the test class instance.
  2. It's fine to provide easy DX/access to fields/instances/definitions that have been created for a test, but setting + relying on dynamic properties that do not exist is not.

History

  1. This code was invented in #1821906: Allow more Field API public API functions to act on a single field within an entity (and unfortunately backported to D7).
  2. #1932382: Use DrupalUnitTestBase for Field API tests. converted the test class into DUTB/KTB later on, but seemingly ignored that issue.

Proposed solution

  1. Keep the DX idea (which is a fair one IMO), but move the dynamic properties into a single $this->fields bag.

API changes

Just in case we consider test base classes as "API"...

 $this->createFieldWithInstance();
 $this->createFieldWithInstance('_second');
...
-$this->assertTrue($this->field->id());
-$this->assertTrue($this->field_second->id());
+$this->assertTrue($this->fields->field->id());
+$this->assertTrue($this->fields->field_second->id());
CommentFileSizeAuthor
#21 2306539.21.patch69.44 KBswentel
#17 2306539.16.patch69.53 KBalexpott
#17 11-16-interdiff.txt43.26 KBalexpott
#11 drupal8.field-unit-test-base.11.patch69.65 KBsun
#11 interdiff.txt7.61 KBsun
#9 drupal8.field-unit-test-base.9.patch63.61 KBsun
#7 drupal8.field-unit-test-base.7.patch59.06 KBsun
#4 drupal8.field-unit-test-base.4.patch58.55 KBsun
#1 drupal8.field-unit-test-base.1.patch46.95 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Active » Needs review
FileSize
drupal8.field-unit-test-base.1.patch46.95 KB
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Issue summary: View changes
Parent issue: #1591852: Convert field tests to PSR-0 » #1821906: Allow more Field API public API functions to act on a single field within an entity

Sorry, blamed the wrong original issue; a more granular pickaxe revealed #1821906: Allow more Field API public API functions to act on a single field within an entity

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Issue summary: View changes
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
drupal8.field-unit-test-base.4.patch58.55 KB

QuickEditTestBase::createFieldWithInstance() is a custom re-implementation and has the same problem, so applying the same fix there.

The last submitted patch, 1: drupal8.field-unit-test-base.1.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 4: drupal8.field-unit-test-base.4.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
drupal8.field-unit-test-base.7.patch59.06 KB

Status: Needs review » Needs work

The last submitted patch, 7: drupal8.field-unit-test-base.7.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
drupal8.field-unit-test-base.9.patch63.61 KB

Rebased against HEAD.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

Overall it's great clean-up!

  1. +++ b/core/modules/field/src/Tests/ConfigFieldDefinitionTest.php
@@ -24,6 +24,9 @@ class ConfigFieldDefinitionTest extends FieldUnitTestBase {
+  private $entityType;
+  private $bundle;

+++ b/core/modules/field/src/Tests/CrudTest.php
@@ -26,6 +26,10 @@ class CrudTest extends FieldUnitTestBase {
+  private $field;
+  private $another_field;
+  private $instance_definition;

+++ b/core/modules/field/src/Tests/FieldValidationTest.php
@@ -16,6 +16,10 @@
+  private $entityType;
+  private $bundle;
+  private $entity;

    No doc blocks? 

  2. +++ b/core/modules/field/src/Tests/FieldUnitTestBase.php
@@ -24,10 +24,29 @@
+    $this->fields = new \ArrayObject(array(), \ArrayObject::ARRAY_AS_PROPS);

    would be great to add comment here

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
interdiff.txt7.61 KB
drupal8.field-unit-test-base.11.patch69.65 KB

Sure. But re: 2) The $fields bag is verbosely documented on the property already; no need to repeat that.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs review » Reviewed & tested by the community

I think that's ready

swentel’s picture

swentel CreditAttribution: swentel commented

The only confusing this for me is the name of the bag: $this->fields.

We've renamed field to field_storage in #2287727: Rename FieldConfig to FieldStorageConfig and hopefully soon will rename instance to field. So maybe just rename to $this->fields to $this->fieldBag or so ?

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

For now, my immediate need (blocker) is to get rid of the arbitrary property overloading in those tests.

That's why I went with the most direct equivalent. The current thing in HEAD as well as in this patch is only marginally related to a field bag; it's a collection of arbitrary variables lumped together.

I'd totally +1 a follow-up issue to seek for a better and more appropriate redesign, but if possible, it would be great if we could do this as a first step.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

Yep it's not an array of fields either - how about fieldTestData - which is exactly what it is.

swentel’s picture

swentel CreditAttribution: swentel commented

I can live with fieldTestData as well. #2312093: Rename FieldInstanceConfig to FieldConfig is opened and I'll work on that next week, I'm fine with doing the rename in that issue, it's not that big.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
FileSize
11-16-interdiff.txt43.26 KB
2306539.16.patch69.53 KB

Let's just do it right first time - less noise in #2312093: Rename FieldInstanceConfig to FieldConfig which will be plenty big already.

swentel’s picture

swentel CreditAttribution: swentel commented

Ok +1 if green.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 17: 2306539.16.patch, failed testing.

yched’s picture

yched CreditAttribution: yched commented

Awesome cleanup. The ad-hoc dynamic properties in FieldUnitTestbase have long been an ugly hack.
(note: IIRC, this comes from far earlier than #1821906: Allow more Field API public API functions to act on a single field within an entity, was added in the very first days of D7 Field API...)

Curious to know why the patch uses an ArrayObject instead of just an array, but not a blocker.
RTBC +1.

swentel’s picture

swentel CreditAttribution: swentel commented
Status: Needs work » Reviewed & tested by the community
FileSize
2306539.21.patch69.44 KB

Ok, alex, go ahead :)

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Component: simpletest.module » field system

Any chance to move forward here? :-)

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Closely related: #2314123: Fix various tests

yched’s picture

yched CreditAttribution: yched commented

@sun: just curious, why did you use ArrayObject instead of just an array ?

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

@yched: No particular reason, really. The existing code accesses properties. Since this presents a critical blocker for my work, I tried hard to avoid any possible delays, and simply re-implemented the current DX/API/design, sans class property overloading.

FWIW, I do think that it (property access) looks more appealing. However, this entire construct is an Anti-API to begin with. As already mentioned in #14, I think we should rethink this test code/API from scratch in a separate follow-up issue.

yched’s picture

yched CreditAttribution: yched commented

However, this entire construct is an Anti-API to begin with. As already mentioned in #14, I think we should rethink this test code/API from scratch in a separate follow-up issue

Fully agreed, it always made me cringe a bit. Just never was enough of a priority :-/
I do understand the reasoning for this "good enough" cleanup step, thanks a lot for that @sun.

If no specific reason, I'd simply vote to go back to an array instead of an ArrayObject ? But no strong objection to getting this in as is.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Assigned: sun » catch

Since alexpott contributed to this one, seeing if maybe catch could review.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Assigned: catch » sun
Status: Reviewed & tested by the community » Fixed

Actually, alexpott and I talked through this on IRC; this doesn't need to be reviewed by catch. It's already been looked at by several other folks, and the changes are confined to tests.

Therefore, committed and pushed to 8.x. Thanks!

  • webchick committed 0b0cdc2 on 8.0.x 
    Issue #2306539 by swentel, alexpott, sun: Fixed FieldUnitTestBase::...
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Parent issue: #1821906: Allow more Field API public API functions to act on a single field within an entity » #2304461: KernelTestBaseTNG™
Related issues: +#1821906: Allow more Field API public API functions to act on a single field within an entity

Status: Fixed » Closed (fixed)

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