SimpleTest has gotten to the point where you can no longer simply use drupal_set_message
to output debug information. This is partly due to batch API implementation.
I propose adding some helper functions for debugging, something simple.
Either a way to collect debug messages to display on results page (could just use assertions, but not a nice), or just a output console of sorts which dumps messages to a file.
$this->debug('debug information');
Comments?
Comment | File | Size | Author |
---|---|---|---|
#60 | 296574-drupal-debug.patch | 11.46 KB | boombatower |
#59 | 296574-drupal-debug.png | 4.91 KB | boombatower |
#59 | 296574-drupal-debug.patch | 23.62 KB | boombatower |
#53 | 296574-drupal-debug.patch | 11.13 KB | boombatower |
#51 | 296574-drupal-debug.patch | 11.13 KB | boombatower |
Comments
Comment #1
boombatower CreditAttribution: boombatower commentedComment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedWhen I create my own debugging routines I output the debug info to watchdog. This allows me to capture quite a bit of information on and view it on a different window.
Comment #3
boombatower CreditAttribution: boombatower commentedMore talk: #314112: Add an easy way to write debug statements
Comment #4
boombatower CreditAttribution: boombatower commentedPer other issue focusing on unit test debugging facility.
I'll work on a patch.
Comment #5
Damien Tournoud CreditAttribution: Damien Tournoud commentedHere is my current state of thoughts for tonight (copied from the other issue):
It would be useful to have additional data when test fails. Those include full page dump for all assertions that check stuff on the page:
From a code perspective, this is not too hard to do (the first step is to add an additional_data column to the test result table). But there is a big UI issue. Maybe we should postpone this until we made up our mind on the new test result page?
Comment #6
boombatower CreditAttribution: boombatower commentedI think postponing it would be best...I'll focus on the two UI patches...lets get something figured out.
Comment #7
andreiashu CreditAttribution: andreiashu commentedsubscribing... is this going to be implemented ?
Comment #8
RobLoach#5: Dear god, yes.
Comment #9
moshe weitzman CreditAttribution: moshe weitzman commentedYeah, #5 seems very useful still.
Comment #10
moshe weitzman CreditAttribution: moshe weitzman commented@boombatower - i think we should unpostpone this. code freeze cometh. any thoughts?
Comment #11
boombatower CreditAttribution: boombatower commentedMarked #314112: Add an easy way to write debug statements as duplicate.
Comment #12
boombatower CreditAttribution: boombatower commentedI have begun.
Comment #13
boombatower CreditAttribution: boombatower commentedI think I will split this up into two patches:
1) Verbose mode
2) Debug output function.
This issue will focus on the latter this the first item is a new idea.
EDIT: Other issue created: #500280: Provide a verbose mode for SimpleTest
Comment #14
boombatower CreditAttribution: boombatower commentedAlso created: #500334: SimpleTest: color debug messages
Comment #15
moshe weitzman CreditAttribution: moshe weitzman commentedcrucial for getting contrib authors on board with testing.
Comment #16
boombatower CreditAttribution: boombatower commentedIt seems like this could take advantage of the framework put in place by #443154: Fatal errors in tests not reported as failures.
The patch pulls PHP fatal errors out of the log since it know nows what was the last database prefixed used. Could also pull things out of the prefixed database for debugging.
Something like watchdog() might even make sense:
Probably best to provide SimpleTest function so people are aware of the functionality.
I'll work on this soon.
Comment #17
boombatower CreditAttribution: boombatower commentedThe biggest problem with providing a function is that on say code running due to a drupalGet() SimpleTest code is not loaded. So the function won't be available.
If we use watchdog() then at least the code is there, but that seems like something hard to document in code itself, considering not too many ppl may read documentation say at d.o/simpletest.
Comment #18
Damien Tournoud CreditAttribution: Damien Tournoud commentedI believe we need a dsm() equivalent function in core. This function would:
* outside of simpletest, do what dsm() does
* in simpletest, simply throw a PHP warning, this warning will be caught by our error handler (from both the parent side and the client side), and could be displayed properly in the simpletest log.
Comment #19
boombatower CreditAttribution: boombatower commentedI would have to agree, I like that solution the best. The question is how far to take it. Make a simple dpm() that does some sort or print_r()/var_export() like stuff (maybe even syntax highlight), and allow devel to override with krumo, or include the krumo stuff in core.
Since Drupal is largely a developer platform and has lots of things useful to developers (not to mention SimpleTest itself) it would seem to make sense to include something like this. Usually the only feature I use from devel.
Comment #20
boombatower CreditAttribution: boombatower commentedSeems like there is a negative reactions to providing devel like functionality in core (from IRC) and I wasn't sure about it myself. So we won't go there.
I do think that providing a debug method in core that is very simple and work both during SimpleTest and regular context is the only clean, simple, and documentable way to do this.
The key is should it integrate with devel so that when enabled when you use it you get the pretty krumo output or should they use different function names. The advantage of integrating with devel is that the extra code in this function to allow it to work when using simpletest would not need to be duplicated in devel...and developers would only need to remember one debug function.
Integrating isn't necessarily simple/pretty. I don't like the idea of:
Seems like if we do implement such a function that is overrideable it should be done with a hook that registers debug functions. That starts to seem like overkill so them I am left thinking that we choose to not integrate or integrate only with devel.
Once I get a consensus and a name for the function I'll go ahead and write it.
Comment #21
boombatower CreditAttribution: boombatower commentedComment #22
boombatower CreditAttribution: boombatower commentedJust to get things off the ground, this is a very basic patch. No SimpleTest integration yet. drupal_register_debug_function() was DamZ's idea...so that solves our overrideable problem in a simple manor.
Comment #23
boombatower CreditAttribution: boombatower commentedWill also be effected by #518404: Lock down DB config based on simpletest UA headers
Comment #24
boombatower CreditAttribution: boombatower commentedThis seems to do the trick.
Comment #26
boombatower CreditAttribution: boombatower commentedI think this is a very simple base implementation that we can expand on.
What I would like to see is:
Comment #27
boombatower CreditAttribution: boombatower commenteddocumentation.
Comment #29
boombatower CreditAttribution: boombatower commentedCan we remove that bugged test already??
Comment #30
boombatower CreditAttribution: boombatower commentedPart of that test has been removed. Lets see what happens.
Comment #31
kwinters CreditAttribution: kwinters commentedCross-referencing with possible exception handling change: #522746: Drupal Exception Wrapper Class
If a test throws an exception rather than just doing a watchdog and return or trigger_error, you could potentially pull additional info out of the DrupalException object.
Comment #32
boombatower CreditAttribution: boombatower commentedI think that is separate, but interesting none the less.
Comment #33
boombatower CreditAttribution: boombatower commentedThis will integrate with #443154: Fatal errors in tests not reported as failures
Comment #34
boombatower CreditAttribution: boombatower commentedProvides debug facility for general core use and the SimpleTest integration display debug messages in assertions table with group debug.
#500334: SimpleTest: color debug messages will color the messages so they are easy to distinguish.
The current method is insufficient since SimpleTest reader assumes each message in on a single line and that may not always be so. The question begged, is should they be inserted directly into SimpleTest assertions table or, should we store them in a more complex text format or database.
Eventually I see the general debug statements being displayed via drupal_set_message() in a similar manor to devel, but without krumo.
Comment #35
boombatower CreditAttribution: boombatower commentedMuch better format output with backtrace details, thus eliminating the multi-line issue above. I'll work on designing and implementing the drupal_set_message() portion for regular debugging.
Note: the screenshot has the debug color patch applied.
Code in block.test:
Code in block.admin.inc
This also demonstrates that the backtrace works properly when using the wrapper dd() or drupal_debug().
Comment #36
boombatower CreditAttribution: boombatower commentedDocumented and added drupal_set_message() when error_level is set to ERROR_REPORTING_DISPLAY_ALL. Since the default reporting level for Drupal 7 HEAD is ERROR_REPORTING_DISPLAY_ALL I used that as well, but that means it will need to be changed along with the all other defaults when a stable release is made. I am not sure if that needs to be documented somewhere or what.
EDIT: missed the SimpleTest functions when documenting...will fix.
Comment #37
boombatower CreditAttribution: boombatower commentedFully documented.
NOTE: This will break devel 7.x, so once this gets in we should place an issue in devel queue to use the register function to override drupal_debug() behavior. Devel should define devel_debug() or devel_drupal_debug() to be properly namespaced instead of dd() anyway.
Comment #38
boombatower CreditAttribution: boombatower commentedAfter a very long conversation with chx in #drupal-dev we decided to take advantage of the error handling built into PHP through trigger_error() and simply provide a wrapper. The wrapper will server to provide documentation and dump the data passed to it so that dd() is still enough (short and sweet).
The debug messages are now concerted exception which we agreed will ensure that they are removed before committing. I will convert the verbose output to exceptions in a followup patch.
Comment #39
boombatower CreditAttribution: boombatower commentedFurther reduction, remove drupal_debug() and only provide dd().
Comment #40
boombatower CreditAttribution: boombatower commentedMore minification. :)
Comment #41
chx CreditAttribution: chx commentedNow, that's better. Small patches++
Comment #42
boombatower CreditAttribution: boombatower commentedFollowup: #539114: Update verbose messages to be exceptions
Comment #43
webchickWhy?
I thought we removed drupal_debug? If so, we no longer need it here.
/for/ outputting, probably.
Also, is it possible to write tests for this?
23 days to code freeze. Better review yourself.
Comment #44
chx CreditAttribution: chx commentedEdited the patch file itself so it's good to go.
Comment #45
webchickOk, some feedback after having tried this out.
1. Debug messages are currently coming up as exceptions. They shouldn't be. It muddles the results of the test:
2. When using dd() outside of the SimpleTest context, the results are pretty nasty:
It'd be nice if we could at least replace that "User notice" with "Debug" and use a neutral grey colour here as well.
3. Core's dd() does nothing like what Devel's dd() does. Yes, I know core takes precedence over contrib, and APIs change on a whim, etc. but it still seems like we're needlessly introducing a major developer WTF here. I'd suggest just using d() for debug, like we have t() for translate and l() for link. There are no core functions that I'm aware of that are two characters, except st() which is an alternate form of t(). Alternately, what the heck is wrong with just plain old "debug()"? :P
Comment #46
boombatower CreditAttribution: boombatower commentedThe latter part about what it does outside of SimpleTest context is due to the changes made after #37. Chx and I changed it to use trigger_error() instead of my custom logging utility, but as webchick noted it doesn't work so well outside of the context. #37 should work like we want...perhaps we should try that out and we can minimize from there.
Comment #47
boombatower CreditAttribution: boombatower commentedAlright.
This should fix all outstanding issue...and even adds a test to simpletest.test.
Comment #49
boombatower CreditAttribution: boombatower commentedI must say this is a good one:
error_test_generate_warnings() contains:
trigger_error("Drupal is awesome", E_USER_NOTICE);
Notice anything weird there? (no pun intended)
Fixed in patch and solve test issue.
Comment #51
boombatower CreditAttribution: boombatower commentedMissed another instance of User notice.
Comment #53
boombatower CreditAttribution: boombatower commentedSmall update for t() $args.
Comment #54
chx CreditAttribution: chx commentedI think this addressed webchick's concerns.
Comment #55
Dries CreditAttribution: Dries commentedI'll leave it up to webchick to drive this patch home. My personal take is that the way we mess around with PHP error types (e.g. 'User notice') is a bit of a hack.
The code still seems to use 'User notice' instead of 'User warning' but maybe I misunderstood parts of it. Doesn't this patch prevent other code from using 'User warning' and 'User notices'? That would be bad, IMO.
Comment #56
chx CreditAttribution: chx commentedTo clarify, while we are using user notices for our purposes, it's not like others can't. Currently if you throw a trigger_error it will show up as a Drupal error message after this patch it will be a normal message. It's still there, just the class has changed. And instead of "User notice:" it will say "Debug:". To show an error, everyone uses drupal_set_message, that's why it's there, otherwise you error message includes the "User notice:" which is not cool so I doubt throw_error saw a lot of usage in Drupal (never met anyone using it). And yes using trigger_error is a bit of a hack, however we get a (almost) free ride on the back of the existing error handling infrastructure, otherwise we need to add quite a bit of code -- to get the same infrastructure. That's a waste for debug.
Comment #57
chx CreditAttribution: chx commentedAnd to clarify further, only trigger_error produces E_USER* stuff, that's not something normally comes around.
Comment #58
chx CreditAttribution: chx commentedThat probably was not clear enough. What trigger_error does is always specified by the error handler running. Drupal's error handler already changes what PHP's error handler does. It's not specified anywhere at all what Drupal's error handler should do with the messages passed through trigger_error. So far, almost as an afterthought we put the displayed drupal message in the error class and prefixed the message with "User notice:". We now give it another class and prefix with "Debug:". As we have no specs neither is more correct than the other. The message is displayed, logged etc all the same.
Comment #59
boombatower CreditAttribution: boombatower commentedUpdate patch per request by webchick to use warning icon for debug messages. I have attached screenshot.
This patch is ready to go and would be useful to have available during the testing sprint.
Comment #60
boombatower CreditAttribution: boombatower commentedUgh on laptop...not setup...this is teh patch.
Comment #61
webchickOk, I've committed this to HEAD.
Like Dries, I too worry about the hackishness of taking over trigger_error, and am concerned about the WTF this brings to PHP developers new to Drupal. But chx does have a good point at #58 that Drupal is defining its own error handler which takes precedence over trigger_error() anyway.
With the commit of this patch, we can satisfy the code freeze requirement to get all function changes in, yet clean-up the implementation later if necessary.
This change needs to be documented in the upgrade docs.
Comment #62
boombatower CreditAttribution: boombatower commentedGreggles added documentation, but it used the format from the initial post which is incorrect. I correct: http://drupal.org/node/394976.
Comment #64
mgiffordUnfortunately the documentation gives no guidance about when/where to insert a debug statement like:
It's all rather circular.
Can we have a clear example of how one might use a debug feature for simpleTest?
Comment #65
boombatower CreditAttribution: boombatower commentedhttp://blog.boombatower.com/drupal-7-debug-and-simpletest-verbose
Comment #66
mgiffordThanks!
Comment #67
gupta324 CreditAttribution: gupta324 commentedComment #68
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedDrupal treating all E_USER_NOTICE messages as if they were debug messages means you can't simply call
trigger_error('some text')
in your code and have it behave like you expect it would. This is causing specific problems in #2844716: "Missing/moved modules" PHP warnings should be PHP notices instead].I have posted a patch at #2844732: Drupal treats all user-level PHP notices as debug messages to make it so only messages triggered via the debug() function get treated this way, rather than all E_USER_NOTICE messages.