Blacklisting test [#716250]

Comment	File	Size	Author
#8	mollom-DRUPAL-6--1.blacklist.8.patch	10.96 KB	sun

#5	mollom-HEAD.blacklist-test.5.patch	11.58 KB	sun

#3	mollom-HEAD.blacklist-test.3.patch	11.12 KB	sun

#1	mollom-HEAD.blacklist-test.1.patch	10.49 KB	sun

Comment #1

sun

German

Karlsruhe

CreditAttribution: sun commented 16 February 2010 at 22:24

Status:

Active

» Needs review

File	Size
mollom-HEAD.blacklist-test.1.patch	10.49 KB

Holy cow. That base64_encode() munging really killed my nerves. Quite inconsistent...

- When adding a URL, only the domain name is added, i.e. "spam.com".

- When deleting a URL, it is a fully-qualified domain name as URI, i.e. "http://spam.com/". Note the trailing slash, and also note that all URLs are stored lower-case.

This patch makes the Blacklisting test basically work, but...

I somehow get the impression that text blacklist API does not properly work.

It seems like the tests passed previously, since the testing text contained the word "spam", but "spam" is already a hard-coded keyword.

Log in or register to post comments

Comment #2

sun

German

Karlsruhe

CreditAttribution: sun commented 16 February 2010 at 22:30

+++ tests/mollom.test	16 Feb 2010 22:22:17 -0000
@@ -1092,55 +1104,63 @@ class MollomBlacklistTestCase extends Mo
+    $term = $this->randomName();
     $result = mollom('mollom.addBlacklistText', array(
-      'text' => 'unicorn',
+      'text' => $term,

errr, please scratch what I said above. It was "unicorn", not "spam".

So I really wonder what the problem is.

Powered by Dreditor.

Log in or register to post comments

Comment #3

sun

German

Karlsruhe

CreditAttribution: sun commented 16 February 2010 at 23:01

Status:

Needs review

» Reviewed & tested by the community

File	Size
mollom-HEAD.blacklist-test.3.patch	11.12 KB

Hey, who said that blacklisted text needs to be lower-case and don't contain numbers? :P

Log in or register to post comments

Comment #4

Dries CreditAttribution: Dries commented 17 February 2010 at 13:51

+++ tests/mollom.test	16 Feb 2010 22:42:23 -0000
@@ -1054,36 +1054,48 @@ class MollomBlacklistTestCase extends Mo
     // Although we only added one element, we're using a loop because we want to
     // reset the blacklist in case it got somehow polluted.
+    // @todo This breaks in concurrency and when test runs do not finish.
+    /*
     foreach ($blacklist as $entry) {

I don't see how this can be fixed without removing the foreach. Removing the foreach has the drawback that we wouldn't clean up left-overs on the server-side. Though problem.

+++ tests/mollom.test	16 Feb 2010 22:42:23 -0000
@@ -1092,55 +1104,74 @@ class MollomBlacklistTestCase extends Mo
+    // @todo As of now, only non-numeric, lower-case text seems to be supported.
+    $term = drupal_strtolower(preg_replace('/[^a-zA-Z]/', '', $this->randomName()));

- Blacklists words/strings can't be more than 100 characters right now. Not sure how long randomName() can be.

- On the server-side, we convert the blacklist to lowercase.

+++ tests/mollom.test	16 Feb 2010 22:42:23 -0000
@@ -1092,55 +1104,74 @@ class MollomBlacklistTestCase extends Mo
-    $this->assertEqual($result['spam'], MOLLOM_ANALYSIS_SPAM, t('The message was blocked.'));
+    $this->assertEqual($result['spam'], MOLLOM_ANALYSIS_SPAM, t('Total identical match was blocked.'));
 
     $result = mollom('mollom.checkContent', array(
-      'post_body' => "When match is 'contains', the word can be surrounded by other text: abcunicorndef",
+      'post_body' => "When the term is present, the post should get blocked: " . $term,
     ));
-    $this->assertEqual($result['spam'], MOLLOM_ANALYSIS_SPAM, t('The message was blocked.'));
+    $this->assertEqual($result['spam'], MOLLOM_ANALYSIS_SPAM, t('Identical match was blocked.'));

The difference between 'total identical' and 'identical' is confusing. Not sure I understand from reading the patch.

+++ tests/mollom.test	16 Feb 2010 22:42:23 -0000
@@ -1092,55 +1104,74 @@ class MollomBlacklistTestCase extends Mo
+    // @todo This breaks in concurrency and when test runs do not finish.
+    // $blacklist = mollom('mollom.listBlacklistText');
+    // $this->assertEqual(count($blacklist), 1, t('The server-side blacklist has one entry.'));

There is probably no solution but to remove this.

+++ tests/mollom.test	16 Feb 2010 22:42:23 -0000
@@ -1164,34 +1198,40 @@ class MollomBlacklistTestCase extends Mo
-    $this->clickLink(t('delete'));
+    $delete_url = 'admin/config/content/mollom/blacklist/delete/text/' . base64_encode($text);
+    $this->assertLinkByHref(url($delete_url));
+    $this->drupalGet($delete_url);

That is an improvement but shouldn't explain why the tests failed, IMO. There shouldn't have been more than one 'delete' link in the old tests unless we consistently ran into concurrency issues (i.e. extremely unlikely). I haven't run the tests yet but it would be wonderful if this fixed 'em.