Currently, there are two tests (one in form.test, one in ajax.test) that use drupalPostAJAX() for "real" AJAX testing: i.e., asserting something about the page after it's been modified by the AJAX response. This isn't nearly enough test coverage. For example, if you add an image to an article, and then edit that article, and click "remove" for the image, it does not get removed. And yet, this has slipped by bot, because file.test doesn't have a test for this AJAX interaction.

This patch moves the tricky stuff out of the two existing tests and into drupalPostAJAX(), which will make writing additional AJAX tests easy.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, simpletest-ajax.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
16.44 KB

Oops. Included a stray file. Trying again.

rfay’s picture

subscribe

rfay’s picture

#2: simpletest-ajax.patch queued for re-testing.

rfay’s picture

I think this is a fantastic addition to our testing repertoire.

Here's a summary of my understanding of what's going on here.

We all know we need to test AJAX forms more thoroughly and have tried to find ways to do it. Sure, the long-term solution is a real in-browser testing solution.

But right now, effulgentsia has created a PHP-based simulation of what ajax.js does on the page. As a result, we can come closer to having confidence in AJAX activities, at least with very standard traditional AJAX forms. What this does is take the JSON content returned by the server, apply it to the Simpletest view of the page, and allow validation of the result. Yay!. It's not as good as actually testing what the javascript does, but it's a big step toward better-quality tests. And since we're still stymied on real in-browser testing, this is a great step forward.

This patch is also quite low-risk, as it is testing-side only, and gives us great options for improving our tests.

If it comes back green, I'll RTBC.

rfay’s picture

Status: Needs review » Reviewed & tested by the community

Per #5.

klausi’s picture

#2: simpletest-ajax.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, simpletest-ajax.patch, failed testing.

rfay’s picture

Status: Needs work » Needs review

#2: simpletest-ajax.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, simpletest-ajax.patch, failed testing.

rfay’s picture

I guess this needs the attention of the master, @effulgentsia. Unfortunately, this sat in the queue too long. I took a look at the merge, but didn't have confidence that I'd recreate your original intent.

rfay’s picture

Status: Needs work » Needs review
FileSize
16.47 KB

Thanks to @marvil07's rebase skills here is a rebase from #2 to current HEAD. This is untested, but perhaps it can get us a step forward.

Status: Needs review » Needs work

The last submitted patch, drupal.ajax_testing_789186_12.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
18.45 KB

Thanks for the #12 re-roll. This fixes the test failures.

People keep making claims on critical issues that tests can't be added because AJAX is involved. See #896162: Write tests for the Field UI formatter settings form. Let's get this in and change that mindset.

rfay’s picture

Status: Needs review » Reviewed & tested by the community

This improvement will let us do far better AJAX testing, and with this we can say "AJAX Form testing is mandatory" instead of "Sorry, we can't test that".

This affects testing only, not core functionality, so is extremely low risk.

RTBC.

rfay’s picture

rfay’s picture

@webchick, @dries, Placing another vote to get this committed. (Can I vote over and over again?) It's a drag that this one didn't go in way back when, but the fact that it's not in now means that some of the AJAX patches are getting committed with poorer tests than they would otherwise have. This patch allows for far higher quality tests and will encourage us to do *something* about AJAX tests. There are still some issues in the queue that can be improved by this, and it will also affect contrib testing over the life of D7.

webchick’s picture

Oh, actually, I meant to get back to this but it fell off my radar. Please ping me about it tomorrow because I'm going to bed now. ;P

effulgentsia’s picture

chx’s picture

Priority: Normal » Critical

Given that the commit of a critical give borne to another critical bug and effulgentsia said this would have prevented , moving it over to the critical queue so it surely gets in.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Alright. Let's get this in then. Committed to CVS HEAD. Thanks.

rfay’s picture

The only problem is that now we have to write tests :-) Oh, I guess that's a good thing!

grendzy’s picture

Status: Fixed » Needs work
Issue tags: +Needs documentation

re: #15, making ajax tests mandatory: Surely you're joking.

This function takes 8 parameters and has no documentation whatsoever. I thought this might even be a record number for Drupal, but it's edged out by field_default_form().

Anyway, without docs, I think AJAX is still firmly in the "untestable" territory.

catch’s picture

Priority: Critical » Normal
rfay’s picture

@effulgentsia, People are very happy to try to build a test using the techniques added here, but are clueless. And looking at the patch and the issue I'm clueless too. In #913846: Image/file field breaks after uploading two files there are a few people willing to write a test as the issues on that one get sorted out, but they can't do it.

Can you give us pointers on how you've done this? Just some quick tips here might be good enough to start. I'm afraid we're missing the most important period for this improvement because nobody knows how to take advantage of it.

effulgentsia’s picture

Hey rfay, grendzy, et al. Sorry for being MIA so much lately. Writing AJAX tests for #913846: Image/file field breaks after uploading two files is still challenging, because they deal with file uploads, which drupalPostAJAX() doesn't handle yet, as per this @todo in FileFieldWidgetTestCase::testWidget():

* @todo This function currently only tests the "remove" button of a single-
*   valued field. Tests should be added for the "upload" button and for each
*   button of a multi-valued field. Tests involving multiple AJAX steps on
*   the same page will become easier after http://drupal.org/node/789186
*   lands. Testing the "upload" button in AJAX context requires more
*   investigation into how jQuery uploads files, so that drupalPostAJAX() can
*   emulate that correctly.

I haven't looked in depth on that issue yet. It might be that the bugs there surface when JS is disabled as well, in which case regular tests can accomplish what is needed. If the bugs only occur when JS is enabled, then yeah, that raises the urgency of figuring out how to make drupalPostAJAX() handle file uploads. I'll try to get to that issue soon and help out.

Candidates for better AJAX test coverage now possible that don't require figuring out file uploads include Book module and Field UI's Manage Display page, both of which have had recent regressions. I'll post in the appropriate issues for those.

Documenting the parameters and example usages of drupalPostAJAX() is definitely needed. I'll see if I can put something together.

Meanwhile, here's some quick pointers:

You can still use drupalPostAJAX() the same way you were able to prior to this issue, which is to get back a commands array, and then assert stuff that should be there. This works fine if you only need to test a single action. An example is in PollJSAddChoice::testAddChoice().

What this issue enabled is testing consecutive iterations. An example is AJAXMultiFormTestCase::testMultiForm(), where each call to drupalPostAJAX() leaves $this in a similar state as when you do a regular drupalPost(). You can assert based on its contents, which now reflect any emulated AJAX update, and can call drupalPostAJAX() again to keep updating the page iteratively, and asserting what you want from each iteration. Hope that's helpful, but yeah, I get that it might not be as helpful as getting drupalPostAJAX() arguments properly documented.

eojthebrave’s picture

Status: Needs work » Needs review
FileSize
2.35 KB

Was going to play around with file uploads via AJAX but realized I have no idea what this function does or how it works.

Here's a first pass at documenting the arguments for this beast. Note that I haven't actually used the function yet so this is based just on me reading the code and trying to figure out what is going on.

eojthebrave’s picture

Fix wrapping of comments.

rfay’s picture

eojthebrave++ !!!

There is one example in core. Effulgentsia demonstrated this use for testing in #926016-49: Several bugs when trying to remove files from a multivalued file/image field.

rfay’s picture

Status: Needs review » Reviewed & tested by the community

This seems great to me.

rfay’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 789186-28-drupalpostajax_documentation-eojthebrave.patch, failed testing.

rfay’s picture

Status: Needs work » Needs review
Issue tags: +Needs documentation
rfay’s picture

Status: Needs review » Reviewed & tested by the community

Don't know how a docs patch can get those errors.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for the awesome docs improvements. That should really help people down the line to use this function.

Committed to HEAD.

Status: Fixed » Closed (fixed)

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

effulgentsia’s picture

Status: Closed (fixed) » Reviewed & tested by the community

I don't see this in HEAD or in a CVS commit log. Am I missing something? I agree that the docs are great.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Oops! :)

Committed to HEAD for reals this time!

Status: Fixed » Closed (fixed)
Issue tags: -Needs documentation

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