I have made a patch that introduces Simpletest tests for the Image CAPTCHA module.

This required an small tweak on the base (abstract) CaptchaBaseWebTestCase::setUp() method, so that this very same class can be subclassed to implement tests for other modules.

The introduced tests only check that the source image URL is being generated correctly by actually trying to fetch it. Checking that an image is correct is a tough task, so the test just check if any data were generated at all. This helped to confirm the #2462479: Captcha - No image displayed bug and fix.

CommentFileSizeAuthor
#19 captcha-7.x-1.x-image_captcha_simpletest-2468539-19.patch3.12 KBAohRveTPV
#15 captcha-7.x-1.x-image_captcha_simpletest-2468539-15.patch3.12 KBAohRveTPV
#13 interdiff.txt1.92 KBflaviovs
#13 captcha-7.x-1.x-image_captcha_simpletest-2468539-13.patch3.12 KBflaviovs
#9 interdiff.txt2.11 KBAohRveTPV
#9 captcha-7.x-1.x-image_captcha_simpletest-2468539-9.patch2.96 KBAohRveTPV
#6 captcha-7.x-1.x-image_captcha_simpletest-2468539-6.patch3.17 KBflaviovs
#4 captcha-7.x-1.x-image_captcha_simpletest-2468539-3.patch3.25 KBflaviovs
#1 captcha-7.x-1.x-image_captcha_simpletest-2468539-1.patch2.93 KBflaviovs
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

flaviovs’s picture

flaviovs CreditAttribution: flaviovs commented
Status: Active » Needs review
FileSize
captcha-7.x-1.x-image_captcha_simpletest-2468539-1.patch2.93 KB

Status: Needs review » Needs work

The last submitted patch, 1: captcha-7.x-1.x-image_captcha_simpletest-2468539-1.patch, failed testing.

AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented

Very cool.

The func_get_args() is a little different from what seems to be used throughout core:

    $modules = func_get_args();
    if (isset($modules[0]) && is_array($modules[0])) {
      $modules = $modules[0];
    }

I wonder if it might be better to use that code for consistency. (I don't know if you got your func_get_args() from somewhere?) A difference with the code in the patch is the patch will allow a module to be passed as a string for the first argument.

flaviovs’s picture

flaviovs CreditAttribution: flaviovs commented
Status: Needs work » Needs review
FileSize
captcha-7.x-1.x-image_captcha_simpletest-2468539-3.patch3.25 KB

Lest try again with some fixes to please testbot:

  • 5b67580 Use getAbsoluteUrl() in Image CAPTCH test to help testbot
  • 0bb53dd Change Image CAPTCHA test to cope with testbot not using clean URLS by default
  • feb422c Rename ImageCaptchaUnitTestCase -> ImageCaptchaWebTestCase

Status: Needs review » Needs work

The last submitted patch, 4: captcha-7.x-1.x-image_captcha_simpletest-2468539-3.patch, failed testing.

flaviovs’s picture

flaviovs CreditAttribution: flaviovs commented
FileSize
captcha-7.x-1.x-image_captcha_simpletest-2468539-6.patch3.17 KB

@AohRveTPV,

I understand your point. Here is a final patch that implement the change more concisely (using the pattern you suggested).

BTW, I think also I fixed the issues that prevented testbot to run the tests as expected. Now, the test is doing the right thing, though it won't pass past testbot until the fix in #2462479: Captcha - No image displayed is tested and committed.

flaviovs’s picture

flaviovs CreditAttribution: flaviovs commented
Status: Needs work » Needs review

Please review.

Status: Needs review » Needs work

The last submitted patch, 6: captcha-7.x-1.x-image_captcha_simpletest-2468539-6.patch, failed testing.

AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented
Status: Needs work » Needs review
FileSize
captcha-7.x-1.x-image_captcha_simpletest-2468539-9.patch2.96 KB
interdiff.txt2.11 KB

- Shorten function short description to <=80 characters per coding standards.
- Make few other changes to comply with coding standards.
- Remove now-incorrect @see tags.

Status: Needs review » Needs work

The last submitted patch, 9: captcha-7.x-1.x-image_captcha_simpletest-2468539-9.patch, failed testing.

AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented

Not sure why assertNonEmptyImage() is public instead of protected? I am not good at OOP so maybe I am missing why it should be public.

Otherwise, patch in #9 looks good to me. Having these tests would be much better than not.

flaviovs’s picture

flaviovs CreditAttribution: flaviovs commented

Yep, assertNonEmptyImage() was originally a test*() function, so that's why it is (forgotten) public. It should be protected for sure.

BTW, there are other improvements related to the change I mentioned. The assert function is simply doing too many things. As an assert function, it should not include any test code in the "act" test phase, but it currently does. I will send an improved patch in a couple hours or so.

flaviovs’s picture

flaviovs CreditAttribution: flaviovs commented
Status: Needs work » Needs review
FileSize
captcha-7.x-1.x-image_captcha_simpletest-2468539-13.patch3.12 KB
interdiff.txt1.92 KB

Here it goes.

  • Move code that setup and fetch CAPTCHA form from assertNonEmptyImage() to getImageCaptchaForm(). This change makes the methods much more flexible for adding new tests in the future.
  • Adjust existing tests to reflect the change above.
  • Make non-test*() methods "protected"
  • Minor tweak (remove unnecessary temp variable)

Status: Needs review » Needs work

The last submitted patch, 13: captcha-7.x-1.x-image_captcha_simpletest-2468539-13.patch, failed testing.

AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented
Status: Needs work » Needs review
FileSize
captcha-7.x-1.x-image_captcha_simpletest-2468539-15.patch3.12 KB

Change to one blank line between functions for consistency.

Two lines were used between functions in two places. If it was intentional, please disregard this patch.

Status: Needs review » Needs work

The last submitted patch, 15: captcha-7.x-1.x-image_captcha_simpletest-2468539-15.patch, failed testing.

AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented
Status: Needs work » Reviewed & tested by the community
wundo’s picture

wundo CreditAttribution: wundo commented
Status: Reviewed & tested by the community » Needs review
AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented
FileSize
captcha-7.x-1.x-image_captcha_simpletest-2468539-19.patch3.12 KB

Change imperative function short descriptions to third-person singular per coding standards.

Also, I think it would be clearer if the functions read from top to bottom, but I did not make this subjective change.

One test fail expected, due to #2462479: Captcha - No image displayed.

Status: Needs review » Needs work

The last submitted patch, 19: captcha-7.x-1.x-image_captcha_simpletest-2468539-19.patch, failed testing.

AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented
Status: Needs work » Needs review

Expected fail.

wundo queued 19: captcha-7.x-1.x-image_captcha_simpletest-2468539-19.patch for re-testing.

wundo’s picture

wundo CreditAttribution: wundo commented

Now that the other issue is committed, re-testing

  • wundo committed 1661765 on 7.x-1.x authored by flaviovs 
    Issue #2468539 by flaviovs, AohRveTPV: Adding Image CAPTCHA test suite
wundo’s picture

wundo CreditAttribution: wundo commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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