The comment tests should be enhanced to ensure that

  1. Users can post a comment and solve a CAPTCHA in one shot.
  2. Moderators can update an existing comment without errors.
CommentFileSizeAuthor
#21 mollom.data-update.21.patch4.98 KBsun
#17 mollom.data-update.17.patch4.93 KBsun
#17 mollom.data-update.17.tests-only.patch4.46 KBsun
#15 mollom.data-update.15.patch5.92 KBsun
#14 mollom.data-update.14.patch5.74 KBsun
#12 mollom.data-update.12.patch5.39 KBsun
#4 mollom.test-comments.4.patch2.63 KBsun
#2 mollom.test-comments.2.patch2.69 KBsun
mollom.test-comments.0.patch2.68 KBsun

Comments

Status: Needs review » Needs work

The last submitted patch, mollom.test-comments.0.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new mollom.test-comments.2.patch2.69 KB

Sorry, wrong patch format. Still need to fix all of my scripted automations.

Status: Needs review » Needs work

The last submitted patch, mollom.test-comments.2.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new mollom.test-comments.4.patch2.63 KB

Alright. Windows 7 keeps on to drive me nuts.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community
dries’s picture

dries commented

This looks good to me. It wasn't clear how you identified these missing tests though. Any particular motivation?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Cause for this was a bug report filed in http://mollom.zendesk.com/tickets/2159

dries’s picture

dries commented
Version: 7.x-1.x-dev » 6.x-1.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed to master (7.x-1.x) and 7.x-rest. Thanks sun!

I guess we could consider back-porting this patch to D6.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Version: 6.x-1.x-dev » 7.x-1.x-dev
Component: Tests » Code
Category: task » bug
Priority: Normal » Major
Status: Patch (to be ported) » Needs work

Back to the drawing board. Further analysis on the support issue revealed:

Translating that DB error to make sure we don't miss something:

Integrity constraint violation: Duplicate entry 'comment-39' for key 'PRIMARY':
UPDATE {mollom} SET
entity = comment,
session_id = 1103229a15fcf8426c,
form_id = comment_node_forum_form,
changed = 1300877320,
moderate = NULL,
spam = 0, quality = 1, profanity = 0, languages = NULL
WHERE id = 39;
in drupal_write_record()

...ouch, hold on... the where clause only contains the id, not the entity, so this query tries to update _all_ rows with id 39, regardless of entity type. The where clause should contain both entity and id, as that is the combined primary key.

Need to figure out how to properly catch this situation in a test...

AimAdvantage’s picture

AimAdvantage commented

subscribe

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Work on the tests revealed a quite major problem/bug with the node form integration - we're not storing any Mollom data for nodes, and Node API/Form API doesn't allow us to fix it.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new mollom.data-update.12.patch5.39 KB

Attached patch cleanly reproduces the bug.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review

Let's make the testbot confirm that the tests in this patch fail.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new mollom.data-update.14.patch5.74 KB

And attached patch additionally fixes the bug.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new mollom.data-update.15.patch5.92 KB

Slight comment tweaks.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

I've extracted that essential node form integration fix into #674692: Node module integration tests

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new mollom.data-update.17.tests-only.patch4.46 KB
new mollom.data-update.17.patch4.93 KB

Since we've learned that this is a bug in Mollom module's low-level CRUD handling of session data, I've revamped the tests to remove the dependency on Node and Comment modules, and instead, directly test the API functions.

Much cleaner now.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, mollom.data-update.17.tests-only.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Reviewed & tested by the community
dries’s picture

dries commented
Status: Reviewed & tested by the community » Needs work 
+++ b/tests/mollom.test
@@ -2892,6 +2861,91 @@ class MollomDataTestCase extends MollomWebTestCase {
+    // Update the first data record.
+    mollom_data_save($data1);

This looks good. My only comment is that we never actually change $data1 so it might not be properly updating the existing record. The test can probably be improved in that regard.

I think the patch can be committed as is though -- it's an improvement over what we currently have so we shouldn't hold this up. Good work.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Fixed
StatusFileSize
new mollom.data-update.21.patch4.98 KB

Indeed, that was a bit sloppy. Fixed in attached patch.

Thanks for reporting, reviewing, and testing! Committed to all branches.

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

Status: Fixed » Closed (fixed)

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

  • Commit 31434a5 on master, fai6, 8.x-2.x, fbajs, actions by Dries: 
    - Patch #1117298 by sun: verify that comments can be updated.
  • Commit 4b1c56b on master, fai6, 8.x-2.x, fbajs, actions by sun:
    Issue #1117298 by sun: Fixed database error when trying to update posts...

  • Commit 31434a5 on master, fai6, 8.x-2.x, fbajs, actions by Dries: 
    - Patch #1117298 by sun: verify that comments can be updated.
  • Commit 4b1c56b on master, fai6, 8.x-2.x, fbajs, actions by sun:
    Issue #1117298 by sun: Fixed database error when trying to update posts...