This took a while to track down.

If you use the SimpleTest internal browser to click a link with a query string - for example the test in http://drupal.org/node/248436#comment-834256 you get a php warning like this in your testing report:

Unexpected PHP error [sprintf() [function.sprintf]: Too few arguments] severity [E_WARNING] in [/modules/simpletest/test_case.php line 272]

Thanks to chx and Freso who helped me work out that it wasn't my .test causing the problem.

Here's clickLink() from drupal_web_test_case.php, it has it's own internal assertion which appears to be where the problem is.

  function clickLink($label, $index = 0) {
    $url_before = $this->getUrl();
    $ret = FALSE;
    if ($this->parse()) {
      $urls = $this->elements->xpath('//a[text()="'. $label .'"]');
      if (isset($urls[$index])) {
        $url_target = $this->getAbsoluteUrl($urls[$index]['href']);
        $curl_options = array(CURLOPT_URL => $url_target);
        $ret = $this->curlExec($curl_options);
      }
      $this->assertTrue($ret, t('Clicked link !label (!url_target) from !url_before', array('!label' => $label, '!url_target' => $url_target, '!url_before' => $url_before)), t('Browser'));
    }
    return $ret;
  }
CommentFileSizeAuthor
#11 simpletest_clicklink.patch1.64 KBboombatower
#10 simpletest_clicklink.patch1.3 KBboombatower
#4 simpletest_query_string_decode.patch1.62 KBboombatower
#2 simpletest_query_string_decode.patch1.61 KBboombatower

Comments

catch’s picture

catch
he/him
Primary language English
commented
Title: clickLink() and appended ?foo=bar gives warnings » clickLink() and query string gives warnings
Priority: Normal » Critical

see also: http://drupal.org/node/260495

boombatower’s picture

boombatower commented
Assigned: Unassigned » boombatower
Status: Active » Needs review
StatusFileSize
new simpletest_query_string_decode.patch1.61 KB

Decided to give this a wirl and found that the query strings contain %20 which sprintf thinks is are arguments. Just required that all the urls be decoded before being sent to sprintf.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Removes the exceptions in statistics.test and all other tests using clickLink() still pass.

boombatower’s picture

boombatower commented
StatusFileSize
new simpletest_query_string_decode.patch1.62 KB

Bump, still applies after batch API with fuzz. Re-rolled to remove fuzz.

dries’s picture

dries commented

I ran all tests with and without this patch, and it doesn't seem to make a difference. The same amount of tests fail/pass.

catch’s picture

catch
he/him
Primary language English
commented

On my system, I now get no exceptions (since Batch API went in) - so I'm wondering if this was fixed in another issue somewhere.

boombatower’s picture

boombatower commented

This is still an issue that can occur since the urls can have %20 and such. sprintf interprets the % a a variable and tries to insert it.

The only possibility would be if the urls are now decoded somewhere else, which I will check into.

boombatower’s picture

boombatower commented

Doesn't appear that the urls are decoded anywhere. Based on that I would say this still applies, not sure why the test doesn't give exception. Was it changed?

catch’s picture

catch
he/him
Primary language English
commented
Priority: Critical » Normal
Status: Reviewed & tested by the community » Needs work

This no longer applies cleanly. The test wasn't changed so I'm not sure where else it got fixed. Downgrading to normal, might be unreproducable now.

boombatower’s picture

boombatower commented
Status: Needs work » Needs review
StatusFileSize
new simpletest_clicklink.patch1.3 KB

Well, it doesn't give warnings for the old reason as that does seem to be fixed, rather interesting. I think that is due to the reporting moving to Drupal code instead of SimpleTest library.

Eitherway this patch makes the clickLink method use drupalGet instead of its own cURL request which outputs assertions and such.

It throws notice when no link is found, which it shouldn't, fixed that.

boombatower’s picture

boombatower commented
Title: clickLink() and query string gives warnings » SimpleTest: Update clickLink() to use drupalGet() and clean-up code
StatusFileSize
new simpletest_clicklink.patch1.64 KB

This a slightly updated version of the above patch, with comment update as well.

Since the original bug has been fixed with new framework the scope has changed.

Ran blog.test which has a call to clickLink which and it works fine.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs review » Needs work

It appears this needs tests in simpletest.test. I couldn't quite follow the logic of what exact bugs this change fixes. It would be much easier with a test case. And that way, we know if we broke it again later.

boombatower’s picture

boombatower commented
Status: Needs work » Needs review

This no longer fixes anything, merely change clickLink() to use drupalGet() and cleans code.

It used to fix a bug, but when changed from the SimpleTest PHP library that bug was removed.

The clickLink function is used in several tests so it confirms it works, as it was never broken. Just it should use drupalGet for consistency.

Can add tests, but I think they are unnecessary?

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs review » Fixed

OK cool. I get it now. Thanks, committed.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

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