If verbose logging is enabled on admin/config/development/testing/settings, the SimpleTest > SimpleTest error collector tests fail.

Comments

Status: Needs review » Needs work

The last submitted patch failed testing.

c960657’s picture

Status: Needs work » Needs review
StatusFileSize
new2.34 KB

Reroll.

carlos8f’s picture

Status: Needs review » Reviewed & tested by the community

Works for me, thanks :)

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Let's get an inline comment to explain that change related to verbose messages.

I'm also confused about the API change to assertError(). If anything I'd think you'd change it to $message = '', since that's what most of the other assertion messages seem to be doing.

c960657’s picture

Status: Needs work » Needs review
StatusFileSize
new2.55 KB

I updated the comments.

assertError() is only called from the DrupalErrorCollectionUnitTest class that always supplies all arguments, so there is no need for default arguments.

retester2010’s picture

#5: simpletest-verbose-3.patch queued for re-testing.

Stevel’s picture

Status: Needs review » Reviewed & tested by the community

To catch other debug messages as well, we could test for $group == 'User notice', which is the group used for debug and verbose messages.

... but as it's only used locally, why bother :)

sun’s picture

Status: Reviewed & tested by the community » Needs work

#839068: DrupalErrorCollectionUnitTest fails in Verbose mode has been marked as duplicate, but seems to contain a simpler approach. This patch adds proper comments, so it's preferred. However, whichever approach makes more sense needs discussion. To be solved in this issue.

dww’s picture

Status: Needs work » Needs review
StatusFileSize
new1.15 KB

We were also confused by this same bug at #930812: Testing is stalled due to failing simpletest tests, where justinrandell posted a nearly identical patch to DamZ's in #839068...

Anyway, yeah, DamZ's approach for this is cleaner than #5, so I re-rolled it to remove fuzz and get a fresh bot result for RTBC and commit...

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

this is a simple patch, all simpletest tests pass for me locally, RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

It looks like the last time I reviewed this patch, i marked it cnw for comments, and lo and behold I am doing the same now. ;)

+    if ($group != 'User notice') {

Why explicitly exempt "User notice"?

+    else {
+      parent::error($message, $group, $caller);
+    }

A one-liner explaining what happens in the "else" case would be nice, since the if branch has it.

dww’s picture

Assigned: Unassigned » dww
Status: Needs work » Needs review
StatusFileSize
new2.09 KB

one-liner?! as if. ;) to actually explain WTF is happening here requires a small novel...

dww’s picture

StatusFileSize
new2.01 KB

Sorry, emacs failure -- I accidentally capitalized "Protected" without meaning to. Also, the collectedErrors array is protected, not private, to this class.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Yeah, yeah....

dww’s picture

StatusFileSize
new2.01 KB

Fixed a typo in one of the comments and added that debug() is treated the same way as verbose(). Still RTBC, IMHO. ;)

webchick’s picture

Well! That was way more than I was asking for, but YAY! :) Nicely done.

Committed to HEAD.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

And by that I mean...

dww’s picture

@webchick: Cool, thanks. ;)

Status: Fixed » Closed (fixed)

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