I finally figured out what I didn't like in all of our attempts to fix assertMollomWatchdogMessages():

Whenever we changed it, we basically destroyed the entire meaning of passing FALSE to assertMollomWatchdogMessages(); i.e., in order to not get false positive test results due to additional, non-severe messages being logged (e.g., server redirects and refreshes), we made any log message pass the assertion.

But if we use assertMollomWatchdogMessages(FALSE), then we have a unique expectation that must be verified: There must have been at least one severe log message, because we expect one.

CommentFileSizeAuthor
#9 mollom-DRUPAL-6--1.assert-watchdog.9.patch10 KBsun
#7 mollom-HEAD.assert-watchdog.7.patch9.99 KBsun
#4 mollom-HEAD.assert-watchdog.4.patch4.93 KBsun
#2 mollom-HEAD.assert-watchdog.2.patch3.67 KBsun
mollom-HEAD.assert-watchdog.0.patch1.94 KBsun

Comments

Status: Needs review » Needs work

The last submitted patch, mollom-HEAD.assert-watchdog.0.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new mollom-HEAD.assert-watchdog.2.patch3.67 KB

Glad to see that this patch already revealed an invalid assertion.

dries’s picture

dries commented

This is certainly a clean-up. I was thinking; instead of $positive why not a $max_severity so we can do 

  if ($row->severity <= $max_severity) {
    $this->pass();
  }
  else {
    $this->fail();
  }

Just a thought to explore. I think the word $positive is a bit funny in this context, and that a $max_severity would provide a bit more flexibility/expressiveness. I think that extra flexibility is required if we want to move this function to core per the @todo.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new mollom-HEAD.assert-watchdog.4.patch4.93 KB

Good idea! Though I'm not sure whether we always want to test for an actual/precise watchdog message severity -- for most code, it only matters that there has been a severe message, and the severity used by the code should be able to be changed freely, as long as it remains severe.

So what we could do is to use $max_severity, but additionally make it accept a Boolean TRUE or FALSE, in case it only matters for the caller to distinguish between severe and non-severe messages (for which the logic could be changed or advanced later).

dries’s picture

dries commented

Good idea! Though I'm not sure whether we always want to test for an actual/precise watchdog message severity -- for most code, it only matters that there has been a severe message, and the severity used by the code should be able to be changed freely, as long as it remains severe.

Wouldn't that mean we pass WATCHDOG_NOTICE in that case? Everything above WATCHDOG_NOTICE is considered to be severe, not? I'm not sure the extra boolean makes a difference.

Could you try a patch with the extra boolean?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

#4: mollom-HEAD.assert-watchdog.4.patch queued for re-testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new mollom-HEAD.assert-watchdog.7.patch9.99 KB

I guess you meant "without the extra boolean".

However, what I tried to say is that most tests will be fine by merely differing between "severe" (FALSE) and "non-severe" (TRUE) watchdog messages. By removing the Boolean logic, all tests have to pass an actual severity level constant; i.e., WATCHDOG_EMERGENCY instead of FALSE.

dries’s picture

dries commented
Version: 7.x-1.x-dev » 6.x-1.x-dev
Status: Needs review » Patch (to be ported)

Got it, and looks good now. Committed to CVS HEAD. Thanks.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new mollom-DRUPAL-6--1.assert-watchdog.9.patch10 KB

Straight backport to D6. Fails for me locally. Testbot won't work, as usual.

Problem space being that I'm currently getting the following error sequence:

Refreshed servers: 'http://174.37.205.152, http://67.228.84.11'
Server 'http://174.37.205.152' redirected to: 'http://67.228.84.11'.
Server 'http://67.228.84.11' redirected to: false.
All servers unreachable or returning errors. The server list was emptied.
Mollom servers can be contacted and testing API keys are valid.

- There are only 2 servers in the server list.

- All servers redirect.

- No server is left and so the request fails.

While the client-side server fallback handling could certainly be improved, this problem seems to affect all clients right now. The last server in the server list must never return MOLLOM_REDIRECT. It either needs to handle the request, or it needs to respond with MOLLOM_REFRESH. Preferably the former.

sun’s picture

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

Thanks for reporting, reviewing, and testing! Committed to D6.

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 227a760 on master, fai6, 8.x-2.x, fbajs, actions by Dries: 
    - Patch #918440 by sun: severe watchdog messages are not asserted.

  • Commit 227a760 on master, fai6, 8.x-2.x, fbajs, actions by Dries: 
    - Patch #918440 by sun: severe watchdog messages are not asserted.