Tests for Field UI

First stab at this

Patch adds CRUD testing to field UI. I (think/ hope) it's already worth a review.

from IRC:

tha_sun: Amitaibu: I can think of the following test-cases
tha_sun: Amitaibu: Test case 1) "CRUD" operations, i.e. add a field, edit the field, delete the field.
tha_sun: Amitaibu: Test case 2) Field instance configuration, ensuring proper form output and changing of common instance (05:47:38 PM)
tha_sun: Amitaibu: 3) Field/widget configuration, ensuring proper form elements and changing of common field/widget configuration values
tha_sun: Amitaibu: The field type specific tests need to go into the respective field modules, so it's all about common UI functionality here
tha_sun: Amitaibu: 4+5+6) Ensuring proper output/behavior on overview screens: Manage fields, Field display, Field display per bundle

Test case 1 (CRUD) Is covered by the patch in #2.

Tests can come anytime from now until release, so this doesn't qualify as an API clean-up.

Great to see someone working on this important task, though! :D

Cross-referencing #609506: 'Undefined index' on admin/reports/fields and 'change widget' form which these tests should be expanded to catch.

Many thanks for tackling this, Amitaibu !

Here are a couple remarks.

+++ modules/field_ui/field_ui.test
@@ -0,0 +1,127 @@
+    // Add taxtonomy module, so the "Tags" field will be created.
+    parent::setUp('taxonomy');

taxonomy is part of the default profile, so there should be no need to enable it explicitly.

+++ modules/field_ui/field_ui.test
@@ -0,0 +1,127 @@
+    // Check all fields.
+    $fields = array(
+      t('Label'),
+      t('Name'),
+      t('Field'),
+      t('Widget'),
+      t('Operations'),
+      t('Add new field'),
+    );
+    foreach ($fields as $field) {
+      $this->assertText($field, t('%field field found', array('%field' => $field)));
+    }

That's a little vague. These words are bound to appear several times on that page, so it's not clear what is actually tested here.

+++ modules/field_ui/field_ui.test
@@ -0,0 +1,127 @@
+  function createField() {
+    // Missing data in the fields.
+    $edit['_add_new_field[label]'] = 'Field UI';
+    $this->drupalPost('admin/structure/types/manage/article/fields',  $edit, t('Save'));
+    $this->assertText(t('Add new field: you need to provide a field name.'), t('Field name is mandatory'));

This tests 'no field name entered', the comment should make it clearer.
We could check the other failure cases:
- any one of [field name, field label, field type, widget type] missing.
- invalid widget type for the selected field type.
- field_name already exists.
Possibly this could be done by initializing an array with the successive set of (invalid) form values, and run a loop which submits the form with each set of values and just check we're sent back on the same page with some error message (I don't think we need to test the actual message text).
Maybe this deserve a separate createFieldFailures() function.

+++ modules/field_ui/field_ui.test
@@ -0,0 +1,127 @@
+    $edit['_add_new_field[label]'] = 'Field UI';

Using 'Field UI' as the field label is kind of confusing later on ;-)

+++ modules/field_ui/field_ui.test
@@ -0,0 +1,127 @@
+    $this->assertText(t('Field UI field settings'), t('Field settings was found.'));

Minor, assert message should be 'Field creation: Field settings page was displayed'

+++ modules/field_ui/field_ui.test
@@ -0,0 +1,127 @@
+    $this->assertText('field_ui_test', t('Field was created and appears in overview page.'));

It might be worth checking that the actual field and instances have been created using field_info_field() and field_info_instance().
Same thing after 'add existing field' and 'delete field'

+++ modules/field_ui/field_ui.test
@@ -0,0 +1,127 @@
+    $this->drupalGet(('admin/structure/types/manage/page/fields'));

Minor : double parenthesis

+++ modules/field_ui/field_ui.test
@@ -0,0 +1,127 @@
+    // Add a new field based on an existing field.
+    $edit = array();
+    $edit['_add_existing_field[label]'] = 'Field UI 2';
+    $edit['_add_existing_field[field_name]'] =  'field_ui_test';
+    $edit['_add_existing_field[widget_type]'] = 'text_textfield';
+    $this->drupalPost("admin/structure/types/manage/page/fields",  $edit, t('Save'));

No actual asserts ?

+++ modules/field_ui/field_ui.test
@@ -0,0 +1,127 @@
+    $this->drupalPost('admin/structure/types/manage/article/fields/taxonomy_tags', array('instance[label]' => 'Edited tags'), t('Save settings'));

Minor : it's more legible to define an $edit array and pass it to drupalPost, like is done in the rest of the code.

More generally; but probably not a blocker for a first commit:
all field.module tests are run using the dummy 'test_field' field type, defined in simpletest/tests/field_test.module. This lets tests be independant wrt the specificities or bugs or later changes in a given 'actual' field type like text or number.
When we later test that actual field properties (cardinality, required, field-type specific settings, widget settings, etc...) can be entered and properly show up in the resulting $field and $instance definitions - and we'll need those tests at some point, then we can do so without depending on text.module's current state.
This requires that hook_field_settings_form(), hook_field_instance_settings_form(), hook_field_widget_settings_form() are added to field_test.module for the existing field, instance and widget settings, but this should be rather simple FAPI stuff.

I'm on crack. Are you, too?

yched,
Thank you for the great review!

Patch includes hook_field_settings_form(), hook_field_instance_settings_form(), hook_field_widget_settings_form() in field_test.module
Still todo, as you suggested:

* Create a separate createFieldFailures() function.
* Check fields using field_info_field() and field_info_instance().Same thing after 'add existing field' and 'delete field'

Letting test bot have a crack at it. Thanks so much, Amitaibu!

hmm, I had no failures on my local, I'll have a look tomorrow.

+++ modules/field_ui/field_ui.test
@@ -0,0 +1,153 @@
+    $field = t('Add existing fields');

The actual text reads 'Add existing field'
+ minor : indirecting the text through a $field variable looks overkill, and adds confusion around the term 'field' ;-)

+++ modules/field_ui/field_ui.test
@@ -0,0 +1,153 @@
+  function createField() {
...
+    // Go to the field edit page.
+    $this->drupalGet('admin/structure/types/manage/article/fields/field_ui_test');
+
+    // Assert settings implemented in hook_field_widget_settings_form().
+    $this->assertText(t('Field test field widget settings '), t('Field widget settings were found'));
+
+    // Assert settings implemented in hook_field_settings_form().
+    $this->assertText(t('Field test field instance settings'), t('Field instance settings were found'));
+
+    // Assert settings implemented in hook_field_settings_form().
+    $this->assertText(t('Field test field settings'), t('Field settings were found'));
+
+    // Populate the field settings.
+    $edit = array();
+    $edit['instance[widget][settings][field_test_widget_settings]'] = t('Field widget settings value');
+    $edit['instance[settings][field_test_instance_settings]'] = t('Field instance settings value');
+    $edit['field[settings][field_test_settings]'] = t('Field settings value');
+    $this->drupalPost(NULL, $edit, t('Save settings'));
+
+    $this->drupalGet('admin/structure/types/manage/article/fields/field_ui_test');
+    // Assert field settings were saved.
+    foreach ($edit as $element => $value) {
+      $this->assertRaw($value, t('%field were saved.', array('%field' => $value)));
+    }

This part tests 'field edit', it doesn't belong in createField().

Actually, we should test assigning settings during both the 'create field' and 'update fields' workflows. Not required for a first commit of the tests, though.

+++ modules/field_ui/field_ui.test
@@ -0,0 +1,153 @@
+    $this->assertText(t('Field test field widget settings '), t('Field widget settings were found'));

extra space at the end of the string.

+++ modules/simpletest/tests/field_test.module
@@ -104,6 +104,59 @@ function field_test_entity_info_alter(&$entity_info) {
+    '#default_value' => !empty($settings['field_test_widget_settings']) ? $settings['field_test_widget_settings'] : '',

Field API makes sure that $settings['field_text_widget_settings'] is present and initialized, so the !empty() test is not needed.
Same goes for the other settings form hooks.

This review is powered by Dreditor.

yched,
I have noticed that when I create a field, after "Save field settings" I'm redirected back to the "manage fields" page - and not to the "field edit page". From your comment I understand this is a bug, right?

#614030: Cannot edit fields for taxonomy terms also adds some tests.

re #13:

I have noticed that when I create a field, after "Save field settings" I'm redirected back to the "manage fields" page - and not to the "field edit page". From your comment I understand this is a bug, right?

Er, indeed. Last time I checked (but I've been on vacation for a month), you were taken to the 'instance + widget' settings page after that.
Dunno what's causing this for now, but that's not for this issue to fix, and it only shows the importance of getting some tests committed :-p.

My suggestion right now would be:
testAddNewFieldField(): test strictly the 'add new field' feature
testAddExistingField(): test strictly the 'add existing field' feature
testUpdateField(): what's currently in updateField()
testDeleteField(): what's currently in deleteField()

Not all the tests necessarily need to come full-fledged right now (like testing that actual properties and settings are recorded). Getting some initial structure in sooner and adjusting more in-depth tests in followup patches is better that letting this linger IMO - would be extra cool if you're still around for the followups, though ;-)

re #13: I posted a fix at #615866: Field UI redirections broken. Once that fix is committed, we should update the tests to check that 'add new field' goes through the expected steps.

well indeed it failed on my local as-well, I was probably in a denial mode ;)
Current test has "61 passes, 0 fails, 5 exceptions". Exceptions are in:

Undefined index: description Notice field_ui.admin.inc 807
Undefined index: description Notice field_ui.admin.inc 1130

Should the exception fixes be in this patch?

That's probably because field_test_field_info() has no 'description'. Yeah, let's add it here, something like 'Dummy field type used for tests'.

Should be green now.

Alright, let's get this in !

+++ modules/field_ui/field_ui.test
@@ -0,0 +1,149 @@
+  function createField() {

If we don't do it in the first iteration, let's add a "@todo: check that actual settings and properties are saved".

+++ modules/field_ui/field_ui.test
@@ -0,0 +1,149 @@
+    // Assert field settings were saved.
+    $this->drupalGet('admin/structure/types/manage/article/fields/field_ui_test');
+    foreach ($edit as $element => $value) {
+      $this->assertRaw($value, t('%field were saved.', array('%field' => $value)));
+    }

(in updateField) I don't understand how this passes. The $edit array contains values passed on the 'Edit Field' page. Those values are not supposed to show up on the subsequent 'Manage Fields' page, and this is not how we should check that they have been saved (should be done by loading the $field and $instance definitions through field_info_[field|instance] and checking they contain the expected data)

+++ modules/field_ui/field_ui.test
@@ -0,0 +1,149 @@
+    // Check "Add existing field" appears.
+    $this->drupalGet(('admin/structure/types/manage/page/fields'));
+    $this->assertNoText(t('Add existing fields'), t('"Add existing field" was not found'));

(in addExistingField) If we check that "add existing field" appears, the assert should be assertRaw, not assertNoRaw. This passes because there's a typo in the tested string (field should be singular)

+++ modules/field_ui/field_ui.test
@@ -0,0 +1,149 @@
+    // Assert the new field is the "manage fields" page.
+    $this->assertText('field_ui_test', t('Existing field was created in a another content type, and appears in overview page.'));

(in addExistingField) We should check that we are redirected to the 'Edit field page'. Same remark than above, @todo: 'Check that actual field properties and settings can be edited and saved'.

+++ modules/simpletest/tests/field_test.module
@@ -104,6 +104,59 @@ function field_test_entity_info_alter(&$entity_info) {
+  $form['field_test_widget_settings'] = array(
+    '#type' => 'textfield',
+    '#title' => t('Field test field widget settings'),
+    '#default_value' => !empty($settings['field_test_widget_settings']) ? $settings['field_test_widget_settings'] : '',
+    '#required' => FALSE,
+    '#description' => t('A dummy form element to simulate field widget settings.'),
+  );

My remark in #12 about !empty($settings['...']) still stands.
Besides, the actual settings names implemented by test_field type and widget are 'test_field_setting', 'test_instance_setting', 'test_widget_setting' while the code in the various _form hooks in the patch refers to '..._settings' (plural). The form submissions in updateField() also need to be fixed accordingly.

I'm on crack. Are you, too?

@sun: LOL - Well, I learned something. + I can be nonsensically nitpicky too, no pb :-p

There is one todo, to take care after #616742: Can't add existing field is fixed.

Here's another bug that wasn't caught before, and we should have a test for: #617532: field_ui menu paths doesn't translate underscores in content type names

Please mark this issue RTBC as soon as there's something committable here, even if it's not totally complete. I'd like to get a baseline in ASAP so we can stop regressions like this, then work on fleshing it out a bit more over time.

And thanks SO much for your hard work on this!! :D

Only minor stuff now.
I don't think the assertFieldSettings() helper function is the best we can do, but this can be adjusted as the tests are enriched.

+++ modules/field_ui/field_ui.test
@@ -0,0 +1,175 @@
+  function createField() {

#615866: Field UI redirections broken is now in, so we need to check the second page (instance + widget settings) is displayed - this probably explains the test failure.
That function still lacks a @todo note about testing actual properties can bet set in the form and read back in $field and $insatnces.

+++ modules/field_ui/field_ui.test
@@ -0,0 +1,175 @@
+    // "Add new field" isn't a table heading so just test the text.
+    $field = t('Add new field');
+    $this->assertText($field, t('%field field was found', array('%field' => $field)));
+
+    // "Add existing field" isn't there yet, as no field was created.
+    $field = t('Add existing field');
+    $this->assertNoText($field, t('%field field was not found', array('%field' => $field)));

Really, the $field variable is overkill here. + "Add new field field was found" makes a poor success message ;-)

+++ modules/field_ui/field_ui.test
@@ -0,0 +1,175 @@
+    // We are redirected back the "manage fields" page.
+    $this->assertText('field_ui_test', t('Field was created and appears in overview page.'));

This doesn't really test we're back on the 'Manage fields' page...

+++ modules/field_ui/field_ui.test
@@ -0,0 +1,175 @@
+    $field_settings = field_info_field($field_name);

That variable should be named $field.

+++ modules/field_ui/field_ui.test
@@ -0,0 +1,175 @@
+    $this->assertRaw(t('Add existing field'), t('"Add existing field" was not found'));

Incorrect assert message ;-)

It seems git of D7 stopped updating -- http://github.com/mikl/drupal , so yesterday I was working against an older version -- Let's see if it's ok now, and all tests pass.

@webchick, I'll address the issue on #25 after I'll manage to get the first batch of tests right ;)

Instead of testing fields on 'article', should we create a content type with a random name including an underscore, to test for #617532: field_ui menu paths doesn't translate underscores in content type names.

The field name should also probably use a random name consisting of all legal characters, instead of 'field_ui_test'.

re #29: right, we should test that too.

Doing so will break the

+    // "Add existing field" isn't there yet, as no field was created.
+    $this->assertNoText('Add existing field', t('"Add existing field" was not found.'));

test in manageFieldsPage(), though, because the newly added 'node_type_with_underscore' won't have the 'tags' field.

We should remove that check anyway, IMO. It only works because 'article' currently happens to use all the fields currently created by a default install, but that might stop being true at any point in the future. I don't think there's a way to test that the 'Add existing field' row is not displayed when there are no fields to be shared, because setting up such a case is really fragile.

In theory, it would be best to use the 'test_entity' entity type instead of 'node' for those tests - just like we use 'test_field' fields instead of 'text' or 'number'.
Requires adding some UI in field_test.module. I'm working on a patch for this, but this is more refactoring than expected, so we should get those tests with fields first.

So; let's add create node type in setUp() and add this type is tests.

Patch adds a new content type, and makes the field name/ label random.

Works for me. Still many things to be tested, but this is a fine 1st pass.

sigh, I've attached a wrong patch - this one for real, has random field name.

Is there a purpose for this line?:

    $this->type = $type->type;

Other than that, it looks good.

Nevermind, I was totally reading that wrong. It's fine.

HELL YES! Since this was marked RTBC by yched, I committed to HEAD! :D May we not break the critical path of Field UI anymore (we hope)!! Marking needs work for whatever remaining tasks.

Here's one (in addition to matt2000's question):

+++ modules/field_ui/field_ui.test
@@ -0,0 +1,198 @@
+  function testCRUDFields() {
+    $this->manageFieldsPage();
+    $this->createField();
+    $this->updateField();
+    $this->addExistingField();
+    $this->deleteField();
+  }

I don't understand why we have this layer of indirection? Normally these would all be separate test() functions. What do we gain by not doing so here?

Also, that function needs PHPDoc. Perhaps a good place to document why we're doing this. ;)

This review is powered by Dreditor.

Cool thanks, and thanks yched for your great reviews!

Attached patch with PHPdoc explanation.

Cool !

re #36-#37: I'm also not 100% sure this "chain tests in one single test run" approach will be fine in the long run, but I didn't want to delay the initial commit too much.
At any rate, the PHPdoc in patch #37 is in line with the current state, so RTBC for this.

I'll try to come up with a list of remaining test @todos shortly.

Ok, committed that. Back to... needs work, I guess. Or active. Anyway. :)

Tests don't qualify as critical.

This is at least major, and possibly even critical. We have a huge major swath of our user-facing code base that lacks test coverage.

See #878294: Field UI reorder on 'Manage fields' affects ordering on 'Manage display' for a random example of silly problems we currently aren't catching.

#886152-37: All fields are hidden after the administrator newly configures a view mode to not use 'default' display has some initial tests for the 'Manage display' screen
Still missing : test reordering and formatter settings. That's after the refactoring in #616240: Make Field UI screens extensible from contrib - part II lands.

I don't know if posting this in here will help at all but I'm having a really annoying issue.. I created a content type with a bunch of fields, after deleting one and realizing i needed it back i did exactly that and php module eval() was not happy. so in an attempt to rectify this issue i decided to delete the content type and start over. however i did not go through and delete the extra fields first. now when i try to create the content type over again i cannot recreate any of those fields because they 'exist' yet they are not available to me in existing fields.... i wish i knew ANYTHING about how to go about solving this but i'm just a themer with little php knowledge :/

@nomonstersinme : That's #838022: Deleting a content type leaves orphaned fields. Known bug, but not fixed yet.

AH! Thank you so much @yched, i was searching for what felt like forever before i even found this issue! :)

Since we have boilerplate tests added in other issues I'm demoting this from major.