After implementing some massive tests elsewhere, which failed for entirely invisible reasons, I had to invent something.
The fails were caused by invalid XML-RPC data, which resulted in a error response from the XML-RPC server, but the module that is tested implements a failover mechanism to not expose such errors.
But of course, those errors were logged. But when running web tests, you can't look into the logs.
So why don't we look always into the logs?
If your test expects a failing watchdog message, then it can pre-emptively invoke $this->assertWatchdogMessages(FALSE), which applies the identical assertion logic reversed (fails will pass).
Subsequent calls will only assert new watchdog messages.
So let's see what failures we have under the hood. :)
Comment | File | Size | Author |
---|---|---|---|
#27 | drupal.assert-watchdog.27.patch | 16.7 KB | sun |
#25 | drupal.assert-watchdog.25.patch | 16.61 KB | sun |
#22 | drupal.assert-watchdog.22.patch | 9.02 KB | sun |
#8 | drupal.assert-watchdog.8.patch | 4.22 KB | sun |
#5 | drupal.assert-watchdog.6.patch | 4.17 KB | sun |
Comments
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedsubscribe.
Comment #3
sunAlrighty - to make any sense out of those fails, we also need the $type of each message.
Comment #4
sunCatch all. :)
Comment #5
sunFixed the docs.
Comment #7
sunCreated #652420: Fix errors uncovered using aggressive watchdog assertion to fix the actual failures, so we can focus on this thingy in here.
Comment #8
sunerrr. Fixed the incomplete logic for checking whether {watchdog} exists before trying to query it ;)
Comment #10
Dries CreditAttribution: Dries commentedTypo:
a ... messages
-- singular/plural mix up.Instead of $pass_expected, I was expecting a string to match. Like:
assertWatchdogMessage('hello world', 'comment');
The API is a bit unexpected for me, and not as intuitive as the example above.
This seems to be scope creep to me, and pollutes the assertResponse() API. It is nicer if the assertResponse() API wasn't inter tangled with watchdog. I'd keep them separate.
Comment #11
sunNote, this aggressive watchdog logging proposal is not to be confused with #500394: Add new log fetcher, which attempts to allow to retrieve individual log messages.
Comment #14
sunI'm continuing to fix patch failures in #652420: Fix errors uncovered using aggressive watchdog assertion. Just wanted to mention once again:
This is totally awesome.
One of the failing Locale tests revealed an utterly wrong test. :)
Will this pass?
Sure it will! An "Access denied" page contains no language selector! :-D
Comment #17
sunCreated #653940: Clean-up: Tests do not report all errors to fix SimpleTest's error reporting.
Comment #18
sunSome code by Damien Tournoud, which might fix those wrong source function identifiers in the test result:
Comment #19
carlos8f CreditAttribution: carlos8f commentedwow. subscribe
Comment #20
catchsubscribing.
Comment #21
sunActually, the nice thing about this patch is that the fails reported in the test results are entirely different to the notices, warnings, and errors that #653940: Clean-up: Tests do not report all errors revealed.
Hence, combining both will make our tests *really* bullet-proof. I really look forward to that.
I'll continue here, when #653940: Clean-up: Tests do not report all errors has landed.
Comment #22
sunDiscussed this some more with smk-ka and carlos8f... we actually think that every request in the internal browser should directly catch any severe messages.
Attached patch implements this as prototype.
Comment #24
sunSo the nice thing about this approach is that you see the actual + real location of the test code that triggered the watchdog message:
So you go in there and you see:
However, I can perfectly see how this new approach will trigger uneasy feelings... but OTOH, I can't think of a better location and way to do it.
Of course, the name of the key I added to $options is highly debatable, and of course, the inlined logic in assertResponse() is no longer required. And we likely want to replace the value for the $options key with a string that's passed as $type to assertWatchdogMessages(), so we don't do a catch-all style assertion in case we expect a certain fail.
Comment #25
sunAnd this is how it would look like.
I didn't update everything, because I need to know whether we want to go with this approach first.
Comment #27
sunerrr, I really hate my copy + paste mistakes :P
Comment #32
sunWe need an agreement on this approach to move forward here.
Note that this was implemented in a contributed module already, which revealed a bug: $seen_ids cannot be cached statically within the function, because the {watchdog} table is thrown away for each test that is run.
Comment #33
sun.core CreditAttribution: sun.core commentedThis is badly needed and still can be considered, but it's Not critical. Please read and understand Priority levels of Issues, thanks.
Comment #34
Dave ReidI've also implemented this in my own contrib module and it's exposed a core bug already: #775012: Theme key watchdog errors on node pages. We can't allow core to ship with random error messages on every singe node edit view or something as commonly viewed. I don't think this should be linked to drupalGet() however.
Comment #35
Dave Reid@33 LOL at giving yourself the lashing since no one else but 'sun' set this to critical. :)
Comment #36
sunThe idea is that we do not expect any severe watchdog messages on any GET/POST request, unless explicitly specified. Therefore, this approach automatically covers any possible scenario and reveals unexpected messages in any request that is performed during testing.
Comment #37
sunComment #38
valthebaldA-ha! Smells the same as #393538: Document that check_plain() can issue PHP messages on invalid UTF-8 input
Comment #39
smartango CreditAttribution: smartango commentedI am sorry to be not exactly on topic, but I experiencing this and make all site break.
For example when I try to search for an invalid utf8 string, system raise exception and site give awful page (innodb exception). Also I read #393538: Document that check_plain() can issue PHP messages on invalid UTF-8 input thread and I realized that I have to add @ on front of check_plain if I want url with utf8 works.
Why does a keyboard character is not utf8 valid code? there is a problem with html encode (http header, html header..)? should be fixed there? what the test would to say and what actions are to be taken is not clean, not here, neither in other report ..
Incidentally I found dblog_watchdog do not call check_plain on location field (nor in referer) I added utf8_encode() there, it seems to work, it seems, in fact this is broken when there are result, so I have to add utf8_encode to search.module (search_simplify() now).
I understand it is very important to define a good test, but at this time I could not go online with drupal 7.14 (who can?) I can disable watch dog, maybe most site want to use solr, it solve? who can use it in a entry hosting?
Finally I ask if at least against dblog and search modules should be filed a bug report, and if those have to call check_plain, give error, translate, have some kind of behaviour, or what. Error on search string is not acceptable. From user prospective it is fail, customer would not pay if a site give error on search, and you know what? he is right on this.
I am just going to make a patch that works, apply it on current drupal version and maintain it internally for web company I am working for .. is this the way opensource is supposed to work?
I appreciate the TDD, defining test is important, and I appreciate the work is done here, I just want to give a wider view of what invalid utf8 error means.
Comment #40
Damien Tournoud CreditAttribution: Damien Tournoud commented@smartango: I assume you are using IIS. See how to configure IIS not to mangle the encoding of server variables. This is a bug in IIS, not Drupal.
Comment #41
valthebald#40: controversial behavior of htmlspecialchars() is the same on IIS and Apache/nginx.
It just behaves opposite to desired: when display_errors is on, it is silent (but other functions throw messages); when display_errors is off, it throws messages.
It's a proper #phpwtf, and I think the only way to overcome it is to suppress htmlspecialchars() call output with @
Comment #42
smartango CreditAttribution: smartango commented@Damien why this bug is absent in Drupal 6? (same IIS)
Comment #43
Damien Tournoud CreditAttribution: Damien Tournoud commented@smartango: Drupal 6 is very lenient. You probably have the same bug but never actually noticed it.
Comment #52
quietone CreditAttribution: quietone as a volunteer commentedTriaging issues in simpletest.module to determine if they should be in the Simpletest Project or core.
This looks like it a Phpunit issue, changing component.
Comment #53
jonathanshawClosing in favour of #2903456: Allow kernel tests to fail or expect logged errors which is newer, but the discussion here is outdated.