As mentioned in the roadmap for 1.x (#2384241), we need tests for MVW.

Although this module would need for the biggest part JS/Browser tests, some fundamental expectations can be tested using Simpletest tests.

Here is a list of tests we want to implement and their current state:

  • The theme registry holds the expected entries; @see multiple_value_widget_theme_registry_alter()
  • The UI for managing the configuration of MVW on a specific field; @see multiple_value_widget_form_field_ui_field_edit_form_alter()
  • Generated HTML markup; @see theme_multiple_value_widget_group()
  • API functions, at the moment only hook_multiple_value_widget_element_title_alter()
  • Widget is present after failed validation.
  • Adding a field item is reflected after form rebuild.
  • Removing a field item (where appropriate) is reflected after form rebuild.
  • Multiple widgets on the same form, different fields: Adding a field item is handled properly.
  • MVW for file fields.
  • Multiple instances of the same field (e.g. in Paragraphs items): Adding a field item (#ajax callback) is handled properly.
CommentFileSizeAuthor
#7 interdiff-5-7.txt13.82 KBstefanos.petrakis
#7 multiple_value_widget-basic_tests-2901830-7.patch15.28 KBstefanos.petrakis
#5 interdiff-2-5.txt7.64 KBstefanos.petrakis
#5 multiple_value_widget-basic_tests-2901830-5.patch13.95 KBstefanos.petrakis
#2 multiple_value_widget-basic_tests-2901830-2.patch10.08 KBstefanos.petrakis

Comments

stefanos.petrakis created an issue. See original summary.

stefanos.petrakis’s picture

stefanos.petrakis
Primary language English
Location Biel, Switzerland
commented
StatusFileSize
new multiple_value_widget-basic_tests-2901830-2.patch10.08 KB

Here is a patch for this.

stefanos.petrakis’s picture

stefanos.petrakis
Primary language English
Location Biel, Switzerland
commented
Component: Code » Automated Testing
ciss’s picture

ciss commented

Awesome, thanks for this! I'd suggest we add a list of test cases we want to cover to the issue summary so that we can check those off.

Since various scenarios should be covered for each widget type individually it might make sense to create a base class and extent it for each widget type. I assume you have more experience with Web test cases than I do, so that decision is yours.

Tests that I think we should cover (bucket list, not necessarily all in one go):

  • Widget is present after failed validation.
  • Adding a field item is reflected after form rebuild.
  • Removing a field item (where appropriate) is reflected after form rebuild.
  • Multiple widgets on the same form, different fields: Adding a field item is handled properly.
  • MVW for file fields.
  • Multiple instances of the same field (e.g. in Paragraphs items): Adding a field item (#ajax callback) is handled properly.
stefanos.petrakis’s picture

stefanos.petrakis
Primary language English
Location Biel, Switzerland
commented
Issue summary: View changes
StatusFileSize
new multiple_value_widget-basic_tests-2901830-5.patch13.95 KB
new interdiff-2-5.txt7.64 KB

Here is a patch that covers some of the wanted test cases (see updated OP).
This is still WIP, reviews are welcome though along the way.

ciss’s picture

ciss commented
Status: Needs review » Needs work

Thanks, this looks like a good start! What follows is a very superficial review (I didn't review the details of the individual tests). I hope to find some time to do an in-depth review later this week.

  1. +++ b/tests/multiple_value_widget.test
@@ -0,0 +1,330 @@
+  public function configureContentType() {

    Should be protected. 

  2. +++ b/tests/multiple_value_widget.test
@@ -0,0 +1,330 @@
+    $this->drupalPost('admin/structure/types/manage/page/fields/body', $edit, t('Save settings'));
+    $this->assertRaw(t('Saved %field configuration.', array('%field' => 'Body')), t('Body field settings have been updated.'));
+
+    // Check if the setting works.
+    $this->drupalGet('node/add/page');
+    $this->assertFieldById('edit-body-und-add-more', t('Add another item'), t('Add another item button found.'));

    Shouldn't assertions be limited to test* methods? If this is a setup method, it should probably focus on setting things up? 

  3. +++ b/tests/multiple_value_widget_test/multiple_value_widget_test.info
@@ -0,0 +1,8 @@
+files[] = multiple_value_widget_test.module

    Why is this neccessary? 

  4. +++ b/tests/multiple_value_widget_test/multiple_value_widget_test.module
@@ -0,0 +1,10 @@
+define("MULTIPLE_VALUE_WIDGET_TEST_MODIFIED_TITLE", "Hello world.");

    Minor nitpick: These should be single quotes.

In general all asserts should have a proper message added, and t() should not be used for assertion messages.

As discussed on IRC I still believe we should test the various widgets individually. This should also help us pinpoint and add corner cases where features are specific to a single widget type.

stefanos.petrakis’s picture

stefanos.petrakis
Primary language English
Location Biel, Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new multiple_value_widget-basic_tests-2901830-7.patch15.28 KB
new interdiff-5-7.txt13.82 KB

Here is an update, based on the last very helpful review.

Regarding the 2. point about

Shouldn't assertions be limited to test* methods? If this is a setup method, it should probably focus on setting things up?

; although this is a valid point, the practice in core is deviating from that, and for good reason I believe. For example, TranslationTestCase::addLanguage uses asserts even though it's a configuration/helper method. And I understand this, as it is also checking some hypotheses about the configuration interface and functionality. In that sense, I think assertions in our test suite may occasionally appear inside configuration/helper methods.

Regarding the

we should test the various widgets individually

remark; I can only agree, I am wondering if we are able to do that using Simpletest; testing the Javascript rendering+functionality is not really possible using Simpletest. Thoughts?