Simpletest does not allow assigning actions to triggers

That's a good one.

I have RTBC'd above 'cos I knew that the bot can set to CNW should a test fail. I am still content with the patch and now the bot is too.

+++ modules/trigger/trigger.test	11 Oct 2009 15:13:19 -0000
@@ -228,7 +228,31 @@ class TriggerOtherTestCase extends Drupa
+    // Verify that the trigger's hook has assigned action.

Looks like there is a word missing to make this sentence 'flow'.

Should be easy to fix!

Change

+ // Verify that the action assigned to correct hook.
+ $actions = trigger_get_assigned_actions($hook);

to

+ // Verify that the action has been assigned to the correct hook.
+ $actions = trigger_get_assigned_actions($hook);

OK then back to the green queue.

Shouldn't we go one step further and ensure that the action actually fires when the user logs in?

+          case 'hidden':
+            if ($edit[$name] == $value) {
+              unset($edit[$name]);
+            }
+            break;

I have no idea what this is doing. Please explain and add comments.

This still needs documentation, and I'm not really sure that the solution makes sense. I would expect that hidden fields need to be treated like text, textarea and password. We need to pass them to post anyway.

Just explain what you just explained in an inline comment for that case statement and be done with it.

+++ modules/simpletest/drupal_web_test_case.php	12 Oct 2009 21:22:55 -0000
@@ -1621,6 +1621,12 @@ class DrupalWebTestCase extends DrupalTe
+          case 'hidden':
+            // Ensure that element exists.

uhm, replace that with maybe: "Normally, hidden fields are considered read-only. However, to allow to submit multiple forms on a single page that only differ in a hidden value, we allow their value to be overridden."

I'm on crack. Are you, too?

uhm, I just wanted to fix the wrapping of that inline comment, but after looking at the actual code and how the processed values of handleForm() are used afterwards, that inline comment is utterly wrong.

Anyway, since I did it, you get it.

that inline comment is utterly wrong.

Hidden fields are not read only. Like, not read only at all.

Poor andypost.

Could someone who is more confident in their English skills (this isn't a slag on andypost at all; he told me he doesn't feel English is his strong suit even though it seems fine to me :)) please rephrase the comment?

This is not critical for API freeze.

Not critical. Please read and understand Priority levels of Issues, thanks.

IMO, we should address this in a more generic fashion. The problem with the triggers page is that each form has the same Submit and the same values in the select. The problem is that the only way we have to choose among them is the value of the button, which is pretty weak. If we could just select with an xpath expression, I think that would do it.

I opened #709852: Impossible to write a test where duplicate buttons/forms exist as a more generic statement of the same problem (encountered in the same place). I'm marking it as a dup of this one.

I don't know if this causes us pain in other areas, but the assignment of actions to triggers is basically not being tested because of this limitation. You can only assign the first item on the page.

I spent quite a lot of time with this tonight, and andypost's approach will work. There were a couple of problems due to the passage of time, but this patch works.

I took such delight in the fact that we can now test triggers and actions that this fleshes out Andypost's original test, adds another one that triggers and advanced (email) action - which we couldn't do before.

I also took the liberty of adding two tiny email helper functions to drupal_web_test_case(). One allows assertion on a *pattern* in an email message, and the other allows verbose output of a mail message. Mail hasn't been supported as well as page views, but when it's better supported, we'll have better tests.

Finally, the trigger_test mock module for trigger had not been properly updated to D7, so was creating warnings about a missing label field. I took the liberty of fixing that, since it's in the line of trigger test and all.

The bot will find 2 exceptions (hopefully only two): The new test uncovered #714190: system_send_email_action uses $account without initializing it , which had already been reported.

Well, unfortunately these test failures demonstrate that this approach does *not* work.

The idea was a workaround to try to get simpletest to find the correct form based on a passed-in hidden field. If the hidden field matches what's there, it helps select the correct form on the page.

The problem is that forms like imagefield's forms submit hidden fields, which are *different* from what's already in the form. So I don't think we're going to be successful piggy-backing on the hidden field idea. This was my original reservation in #45.

IMO we have to change the interface to drupalPost() somehow to give it the ID or name of the form we want to submit.

I will roll a proposed alternate. I hope an interface change will fly, because we really need this testing capability.

This patch changes the signature of drupalPost to add a $form_css_id argument: the CSS ID of the form we want to submit. That completely resolves the ambiguity we've been dealing with, and does not break existing usage of hidden fields. And I think the code is far clearer. Less WTF!

The same set of peripheral issues are dealt with as in #48: There are a couple of assertions, the triggertest module was updated, the trigger tests filled out, and a new one added.

This patch removes the test that exercises system_send_email_action, so we can decouple this issue from #714190: system_send_email_action uses $account without initializing it. I will move the test over to the patch on that issue.

The test using system_send_email_action moved to #721086: Create tests for system actions, clean up token replacement, clean up triggers

+++ modules/simpletest/drupal_web_test_case.php
@@ -1533,8 +1533,12 @@ class DrupalWebTestCase extends DrupalTestCase {
+   * @param $form_css_id
+   *   The optional CSS id of the form to be submitted. On some pages there
+   *   are many identical forms, so just using the value of the submit button
+   *   is not enough.
    */
-  protected function drupalPost($path, $edit, $submit, array $options = array(), array $headers = array()) {
+  protected function drupalPost($path, $edit, $submit, array $options = array(), array $headers = array(), $form_css_id = NULL) {

If anything this should be just $form_id. It doesn't have anything to do with CSS.

+++ modules/simpletest/drupal_web_test_case.php
@@ -2711,6 +2719,31 @@ class DrupalWebTestCase extends DrupalTestCase {
   /**
+   * Look for a regex in the body of the email.
+   * @param string $name
+   *   Name of the portion of the body to search. Most common: 'body'
+   * @param string $regex
+   *   Text to search for.
+   * @param string $message
+   *   Message for simpletest.
+   */

Fails coding standards.
- Needs newline between function summary and @params
- We aren't documenting types in @param or @returns yet.

+++ modules/trigger/trigger.test
@@ -298,4 +322,36 @@ class TriggerOtherTestCase extends DrupalWebTestCase {
+  /**
+   * Configure an advanced action.
+   * @param string $hook
+   *   The name of the hook. For example: 'user_presave', 'user_login'
+   * @param array $action_configure_edit
+   *   The $edit array for the form to be used to configure.
+   *   Example members would be 'actions_label' (always), 'message', etc.
+   * @return integer
+   *   the aid (action id) of the configured action, or -1 if none.
+   */

Fails multiple coding standards.
- See above points, same apply here.
- Why not just use $edit instead of $action_configure_edit?

+++ modules/trigger/trigger.test
@@ -298,4 +322,36 @@ class TriggerOtherTestCase extends DrupalWebTestCase {
+    // Now we have to find out the action ID of what we created.
+    $cell_to_find = "//div/table/tbody/tr/td[2][. = '{$action_configure_edit['actions_label']}']/../td[3]/a[@href]";
+    $this->assertFieldByXPath($cell_to_find, t('configure'), t('Found the configure link'));
+    $atag = $this->xpath($cell_to_find);
+    $this->assertTrue(is_object($atag[0]), t("Found the cell with 'configure' in it"));
+    if (is_object($atag[0])) {
+      $href = (string)$atag[0]->attributes()->href;
+      $aid = preg_replace('/^.*\//', '', (string)$href);
+      $this->verbose(t('The action ID for configured action hook=@hook is aid=@aid.', array('@hook' => $hook, '@aid' => $aid)));
+      return $aid;
+    }

Wow. Just wow. This is going to be a fun chunk to maintain. Is there any better way to accomplish this? Can we just load the action from the database and use that data to find the specific link instead of this voodoo? :)

Powered by Dreditor.

I think it's fantastic. Much improved. I really like what you did with the form id. That really helps bridge the gap between the Drupal and the HTML world.

This no longer applies after today's spring cleaning.

Edit: #60 is the same patch as #56, rerolled. The two pieces removed belonged with #721086: Create tests for system actions, clean up token replacement, clean up triggers so I moved them over there.

This should go in.

Ok, great! Took a look through here and all looks good. I imagine that extra $form_id parameter would also come in handy for testing something like the Fivestar module. Nice to have expanded test coverage for this stuff, too.

Committed to HEAD!

Also, wow, Trigger module is weird, with that md5() stuff everywhere. I questioned rfay about this and he said this was probably to disambiguate the forms from one another, which makes sense, although also seems totally unnecessarily fancy. Maybe we can add a general sanity clean-up to trigger module to the todo list for D8. ;)

We rather want to remove that call to drupal_html_id() entirely here.

yes, tests should pass a valid form_id already. After all, test authors will look up the form_id on the actual page and copy+paste that into their test. drupal_html_id() is totally wrong here.

Please wait for the bot to come back green.

Since we're going back to the HTML ID, I'd like to make the signature really clear about that. This reroll addresses that.

Dang. I loved $form_id in that function. :(

Also this?

-      $this->drupalPost('admin/structure/trigger/node', $edit, t('Assign'));
+      $this->drupalPost('admin/structure/trigger/node', $edit, t('Assign'), array(), array(), 'trigger-node-presave-assign-form');

Ugh. So very, very ugly. And an API change to boot.

Since the reason for changing this according to #65 is "I found that drupal_html_id() uses drupal_static() and there's no ability to post the same form," can we not fix this with #684568: drupal_html_id() should use $drupal_static_fast pattern instead?

@webchick, the API change, with its ugliness, was the only way we could figure out to solve this problem. It was thoroughly reviewed (and accepted and committed already, by you, in #62). The ugliness was introduced there, but only for callers that need to use the HTML ID to find the proper form. So the many instances of that you see here are not required of all callers of drupalPost(), just the callers that need to use the ID to find their way in the test.

@sun: I liked using $form_id as a parameter (instead of the HTML ID of the form) as well. It looks like this was done at your request?

Sorry. I meant the fact that test writers who used to be able to do:

      $this->drupalPost('admin/structure/trigger/node', $edit, t('Assign'));

now have to do:

      $this->drupalPost('admin/structure/trigger/node', $edit, t('Assign'), array(), array(), 'trigger-node-presave-assign-form');

This was not the case in the original patch that was committed.

Sorry, quite some confusion here.

1. The optional form id was already introduced in a (committed) patch earlier in this issue. It is entirely optional, and only required in cases you have the same form more than once on the page.
2. The earlier patch introduced $form_id as parameter, specifying the internal form_id. This is wrong for two reasons:
   
   1. When using the internal $form_id, we need to invoke drupal_html_id() to translate the $form_id into a HTML id, and, assume that we get the translated form_id we want. Since the sole purpose of drupal_html_id() is to ensure that ids in Drupal's HTML output are unique, it's easily possible that drupal_html_id() will not return what you expected, due to other tests/assertions running before yours.
   2. The entire SimpleTest functionality and helper functions work with values from the HTML output. We do not specify an multi-dimensional array of form values to post, but instead input names like foo[bar], as visible in the output. We also specify form button names using t('Save'), as visible in the output. Therefore, it is wrong to specify an internal $form_id, because this is not what we see in the output.

+++ modules/simpletest/drupal_web_test_case.php
@@ -1608,8 +1611,8 @@ class DrupalWebTestCase extends DrupalTestCase {
       // Let's iterate over all the forms.
       $xpath = "//form";
...
+      if (!empty($form_html_id)) {
+        $xpath .= "[@id='" . $form_html_id . "']";
       }
       $forms = $this->xpath($xpath);

Note, however, that we could use the internal form id, if someone comes up with a xpath expression that filters //form via //form/input[@name='form_id',@value='$form_id'], but still makes it return the matched forms, and not that input element. (i.e. like $('form:has(input[name=form_id][value=$form_id])))

138 critical left. Go review some!

Actually, I just got bit by the fact that drupal_html_id() is clearly the wrong tool for this job, as it generated an incorrect HTML ID for me (with a -2 on it). I think we probably have to use the HTML ID to make this work right.

Another +1 from me for this as is.

Ah, got it. Thanks, folks.

Committed to HEAD.