Need tests for AJAX Commands

I can confirm that the patch applies cleanly, and the AJAX tests run fine: 59 passes, 0 fails, and 0 exceptions. Not sure about the PHP syntax error.

It would be cool to make the AJAXPost() method available in DrupalWebTestCase. Field tests, and IIRC, poll tests, both use a hacked-in method to test their own AHAH behaviors currently.

Well, if the function is self contained and generic enough, it does belong to DrupalWebTestCase. Forcing test classes to subclass AJAXTestCase just to gain access to that function is fairly restrictive, we don't have multiple inheritance in PHP.

I think this is great. My only nit is:

+++ modules/simpletest/drupal_web_test_case.php
@@ -1500,6 +1500,82 @@ class DrupalWebTestCase extends DrupalTestCase {
+   *   TRUE to be checked and FALSE to be unchecked. Note that when a form
+   *   contains file upload fields, other fields cannot start with the '@'
+   *   character.

You removed uploaded file support in copying this from drupalPost(). PHPDoc should be updated accordingly.

If someone wants to RTBC this (after the above nit is fixed), I'm in favor. Otherwise, I'd like to give it a little more time for other reviewers to see it, before I RTBC it.

I'm on crack. Are you, too?

I'm not familiar enough with the guts of drupalPost() to have an opinion on the implementation :-(, but I'd suggest renaming AJAXPost() to drupalPostAJAX() for consistency (not sure about the capitalization, too).

+ I see AJAXTestCase has a drupalGetAJAX(). Should this be moved to DrupalWebTestCase to mirror drupalGet() ? Or is this unrelated ?

I'd suggest renaming AJAXPost() to drupalPostAJAX() for consistency (not sure about the capitalization, too).

Capitilization suggested and implemented in #14 is correct. AJAX is an acronym, and this is PHP's convention. Same as DOMDocument::loadHTML()

And the drupalGetAJAX() predates this work so I didn't want to fiddle with it.

Please move it as yched suggests. It makes sense to keep it with drupalPostAJAX(); moving it to the base class won't hurt regressions; and having both of these functions available to more test cases will help, especially as we write more AJAX related tests for field.module.

I'm being hasty with this ...

Ok. Once the above is in, I'll happily mark it RTBC.

awesome.

Here are some comments from an initial read-through. I'm being extra anal about documentation here because the AJAX framework is one of those areas that is voodoo mystery sauce to most PHP developers, and they really need to be able to understand what's going on here if tests in here start breaking when they're patching some other part of the system.

The biggest thing I have a problem with is drupalPost() being copy/pasted in two places. I'd rather us either extend drupalPost to let it test AJAX callbacks as well, or see what else we can do to minimize the amount of 100% duplicated code.

+++ modules/simpletest/drupal_web_test_case.php
@@ -1500,6 +1500,95 @@ class DrupalWebTestCase extends DrupalTestCase {
+   * Perform an HTTP GET on a path, and then json_decode the result.
...
+   * Execute a POST request on an AJAX path (normally system/ajax).
+   * It will be done as usual POST request with SimpleBrowser.
+   *

It'd be nice to get some unification of these comments, since these functions are basically opposite, no?

Also, could we include a bit of info on why I might need to call these functions from my test?

+++ modules/simpletest/drupal_web_test_case.php
@@ -1500,6 +1500,95 @@ class DrupalWebTestCase extends DrupalTestCase {
+   *   Optional query string for the path.

Maybe note that this is passed into drupalGet().

+++ modules/simpletest/drupal_web_test_case.php
@@ -1500,6 +1500,95 @@ class DrupalWebTestCase extends DrupalTestCase {
+   *   JSON-decoded contents of the GET.

Contents of the GET? That doesn't make sense. :)

+++ modules/simpletest/drupal_web_test_case.php
@@ -1500,6 +1500,95 @@ class DrupalWebTestCase extends DrupalTestCase {
+   *   Multiple select fields can be set using name[] and setting each of the
+   *   possible values. Example:
+   *   $edit = array();
+   *   $edit['name[]'] = array('value1', 'value2');

Please put this between @code and @endcode. (see drupal_execute() for an example)

+++ modules/simpletest/drupal_web_test_case.php
@@ -1500,6 +1500,95 @@ class DrupalWebTestCase extends DrupalTestCase {
+      // Let's iterate over all the forms.
+      $forms = $this->xpath('//form');
+      foreach ($forms as $form) {
+        // We try to set the fields of this form as specified in $edit.

Um. Wow. Is all of this hand-holding necessary? Can't we just give an error if they specified values that didn't make sense?

...oh, I see. You've basically copy/pasted the contents of drupalPost() here.

Would it not make more sense to extend the parameters of drupalPost() to take whatever extra stuff is needed to do what you're trying to do here? Having that much tweaky-ass code duplicated in two places gives me the heebie-jeebies.

+++ modules/simpletest/tests/ajax.test
@@ -76,5 +71,95 @@ class AJAXCommandsTestCase extends AJAXTestCase {
+    /**
+     * 'after' command.
+     */
...
+    /**
+     * 'alert' command.
+     */

(etc.)

Please don't use /** PHPdoc comments */ inline. Use // Inline instead. It's really annoying if you're trying to quickly comment out a test function to figure out where errors are coming from.

I'd use // Tests the 'xxx' command. for consistency with the way these are documented in the test module.

+++ modules/simpletest/tests/ajax_forms_test.module
@@ -0,0 +1,306 @@
+ *   Simpletest mock module for AJAX forms testing.

No need to indent this. Should be directly below @file.

+++ modules/simpletest/tests/ajax_forms_test.module
@@ -0,0 +1,306 @@
+function ajax_forms_test_menu() {

Needs PHPDoc.

+++ modules/simpletest/tests/ajax_forms_test.module
@@ -0,0 +1,306 @@
+    'title' => 'AJAX Forms Simple Form Test',
...
+    'title' => 'AJAX Forms AJAX Commands Test',

(minor) We use sentence capitalization, Not Proper Case.

+++ modules/simpletest/tests/ajax_forms_test.module
@@ -0,0 +1,306 @@
+ * @param $form
+ * @param $form_state
+ * @return unknown_type
...
+ * @param $form
+ * @param $form_state
+ * @return unknown_type

Remove these; it's enough to document that this is a form.

+++ modules/simpletest/tests/ajax_forms_test.module
@@ -0,0 +1,306 @@
+    '#options' => array('red' => 'red', 'green' => 'green', 'blue' => 'blue'),

Separate lines.

+++ modules/simpletest/tests/ajax_forms_test.module
@@ -0,0 +1,306 @@
+    '#suffix' => "<div id='ajax_selected_color'>No color yet selected</div>",
...
+    '#suffix' => "<div id='after_div'>Something can be inserted after this</div>
+                  <div id='after_status'>'After' Command Status: Unknown</div>"
...
+    '#suffix' => "<div id='append_div'>Append inside this div</div>",

(etc.)

It seems odd that you're single-quoting the div id, when normally we use double-quotes for that. I notice you don't do this on "test checkbox", so I assume these are oddities and should be fixed.

+++ modules/simpletest/tests/ajax_forms_test.module
@@ -0,0 +1,306 @@
+    '#title' => 'Test checkbox',
...
+    '#value' => 'submit',

These should go in t(), I think.

+++ modules/simpletest/tests/ajax_forms_test.module
@@ -0,0 +1,306 @@
+function ajax_forms_test_simple_form_select_callback($form, $form_state) {
...
+function ajax_forms_test_simple_form_checkbox_callback($form, $form_state) {

These need PHPDoc.

+++ modules/simpletest/tests/ajax_forms_test.module
@@ -0,0 +1,306 @@
+/***** --------- For testing CTools-style AJAX Commands --------- *****/

There is no such thing as "CTools-style AJAX commands." This is testing code in core. I would remove this.

+++ modules/simpletest/tests/ajax_forms_test.module
@@ -0,0 +1,306 @@
+  // Shows the 'after' command with a callback generating commands.
+
+  $form['after_command_example'] = array(

(minor) Remove extra line between comment and form element definition.

+++ modules/simpletest/tests/ajax_forms_test.module
@@ -0,0 +1,306 @@
+  // Note that this won't work until http://drupal.org/node/623320 lands.
...
+    // This does not yet exercise the 2nd arg (asterisk) so that should be added
+    // when it works.

Please spell these out as explicit @todos so we don't lose track of them.

+++ modules/simpletest/tests/ajax_forms_test.module
@@ -0,0 +1,306 @@
+    '#suffix' => "<div id='prepend_div'>Something can be prepended to this div... </div>",

The others don't have three dots, so please don't put three dots here.

+++ modules/simpletest/tests/ajax_forms_test.module
@@ -0,0 +1,306 @@
+function ajax_forms_test_advanced_commands_after_callback($form, $form_state) {
...
+function ajax_forms_test_advanced_commands_alert_callback($form, $form_state) {
...
+function ajax_forms_test_advanced_commands_append_callback($form, $form_state) {
...
+function ajax_forms_test_advanced_commands_before_callback($form, $form_state) {
...
+function ajax_forms_test_advanced_commands_changed_callback($form, $form_state) {
...
+function ajax_forms_test_advanced_commands_css_callback($form, $form_state) {

(etc.)

These all need a line of PHPDoc.

This review is powered by Dreditor.

@rfay: I hope I'm not out of line here, but I agree with webchick that duplicate drupalPost() code will be harder to maintain than a single one that's AJAX compatible. Also, even though the drupalGetAJAX() function is tiny and not yet used much, I do think it belongs in the web test case, for symmetry if for no other reason, and I suspect once it's there, we'll find uses for it. This patch includes those changes. Obviously, feel free to post disagreements. If you don't have disagreements, RTBC it, so it gets in front of webchick for her to verify that her concerns have been addressed.

@webchick: Since rfay concedes on my changes, and since he indicated a desire on your part for this portion of an AJAX testing framework to land in HEAD quickly, I'm RTBC'ing it for your review. The implementation is straightforward and sound and low risk to HEAD. But your thorough review on documentation and grokability is most welcome.

This patch is same as #21 except for these nits:

+++ modules/simpletest/drupal_web_test_case.php	17 Nov 2009 17:15:03 -0000
@@ -1401,6 +1401,14 @@ class DrupalWebTestCase extends DrupalTe
+   * Retrieves a Drupal path or an absolute path and JSON decode the result.

s/Retrieves/Retrieve/

+++ modules/simpletest/drupal_web_test_case.php	17 Nov 2009 17:15:03 -0000
@@ -1449,8 +1478,15 @@ class DrupalWebTestCase extends DrupalTe
+          // handleForm() functions, it's not currently a requirement.

s/functions/function/

I'm on crack. Are you, too?

Awesome work. There are a few more things I could nitpick here (like drupalGet needs PHPdoc for its params) but this is definitely a solid base and getting this in ASAP will help enable other testing.

Committed to HEAD.

Followup: convert field and poll tests that currently use their own hacked helper.
Posting this here, because this required enriching drupalPostAJAX() a little.

Setting needs review.

@rfay: patch *is* rolled against HEAD ;-)

That one should fix the warnings. drupalPostAJAX() cannot assume the structure of the json result array, so it cannot populate $this->content itself. Each consumer has to extract the HTML data from where it expects it.

Maybe drupalPostAJAX() could loop over the $result and put the first $result[n]['data'] it finds in $this->content ? dunno if that's too much assumption already.

Excellent cleanup. I have a request, but if it can't happen due to needing to be strict about API freeze, then I'm fine with the patch as-is.

+++ modules/simpletest/drupal_web_test_case.php	18 Nov 2009 14:31:08 -0000
@@ -1405,7 +1405,9 @@ class DrupalWebTestCase extends DrupalTe
   function drupalGetAJAX($path, array $options = array(), array $headers = array()) {
     $out = $this->drupalGet($path, $options, $headers);
-    return json_decode($out, TRUE);
+    // The response is drupal_json_output, so we need to undo some escaping
+    // before decoding.
+    return json_decode(str_replace(array('\x3c', '\x3e', '\x26'), array("<", ">", "&"), $out), TRUE);
   }
 
   /**
@@ -1546,7 +1548,9 @@ class DrupalWebTestCase extends DrupalTe
@@ -1546,7 +1548,9 @@ class DrupalWebTestCase extends DrupalTe
    */
   protected function drupalPostAJAX($path, $edit, $triggering_element, $ajax_path = 'system/ajax', array $options = array(), array $headers = array()) {
     $out = $this->drupalPost($path, $edit, array('path' => $ajax_path, 'triggering_element' => $triggering_element), $options, $headers);
-    return json_decode($out, TRUE);
+    // The response is drupal_json_output, so we need to undo some escaping
+    // before decoding.
+    return json_decode(str_replace(array('\x3c', '\x3e', '\x26'), array("<", ">", "&"), $out), TRUE);
   }

Is it too late to add a drupal_json_decode() function to common.inc, and have these lines call it? If we have a drupal_json_encode() function as part of core because PHP's json_encode() is lame, then it seems like we should include the decode function right along with it.

This review is powered by Dreditor.

re #30: would make sense I guess.
Let's push this to RTBC to have a core committer feedback.

Agreed.

webchick/Dries: Please see #30. If you agree that it makes sense to add a drupal_json_decode() function to common.inc right below drupal_json_encode() despite it being an API change to core, please either set this back to "needs work", or commit this patch and add a comment that the follow-up cleanup would be okay with you. As far as API changes go, adding a new function that starts with "drupal_" seems about as low risk as can be, and would have the benefit that if someone needs to change the implementation of drupal_json_encode(), then they see the matching function right there below it, instead of having the decoding buried somewhere within a test case class.

Hm. I think this would be acceptable. It doesn't stop anyone from using the old way and those lines of code are damn nasty as-is.

Great! Here it is. I also added tests, so that if someone changes the implementation of the encode function without also changing the decode function or vice versa, bot will catch it.

@yched: please RTBC it if you like it.

Sweet :-)

I asked webchick to wait after #638356: Reorganize field_test.module lands, though. I'll do the reroll ;-)

Reroll after #638356: Reorganize field_test.module.

Looks good! Committed to HEAD.

Without being protected, assertion messages will be logged with drupalGetAJAX() as source.

Wasn't aware of that. Does drupalPostAJAX need the same fix ?

Without being protected, assertion messages will be logged with drupalGetAJAX() as source.

hm? There is no such thing. But yes, it is a good idea to mark those as protected.

drupalPostAJAX() already is protected in HEAD, so #40 is good to go.

@Damien: There is no such what? Do you mean drupalGetAJAX() is a bad function name for what the function does? rfay had pointed the same thing out and suggested renaming it to drupalGetAndJSONDecode(). The advantage to drupalGetAJAX() is when looked at as a group, drupalGet(), drupalGetAJAX(), drupalPost(), and drupalPostAJAX(), provide a symmetrical set of functionality. But if you have a recommendation, please submit one.

No need to hold off the fix in #40. If we do end up renaming the function at some point, it's no harder to rename a protected function than a public one.

Committed to HEAD. Thanks!