Rework simpletest backend
Damien Tournoud - September 7, 2008 - 12:23
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | simpletest.module |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
Description
As #250047: Rework the SimpleTest results interface and clean-up of backend code became a monster patch, here is an extract of the patch that only rework the backend.
Highlights:
- Fix the backtracing logic of
_assert() - Remove the artificial limit on the size of the 'message' field (once again a regression of the DB:TNG patch: we changed that field to be a TEXT in #293500)
- Fixed error handling inside tests
- Rename {simpletest}.caller to {simpletest}.function for consistency
- Improvements to code comments and documentation
Compared to boombatower's patch:
- I used a slightly different backtracing logic, in order to get the good caller name: for an assertion, we know return the name of the function that called the assertion, not the name of the assertion function itself.
- I kept $custom_caller (renamed $caller), because (1) we need it for errorHandler() because we don't want to backtrace in case there is an error in an assertion function, (2) I need it for #304924: Extend drupal_error_handler to manage exceptions.
- I revised several comments and corrected typos.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| simpletest-backend-rework.patch | 8.89 KB | Ignored | None | None |
#1
After a discussion with chx on the IRC, I rewrote the backtracing logic a bit and added useful comments.
#2
After new dicussions with chx, I simplified the
getCallerContext()method a moved it to common.inc with the name_drupal_get_last_caller(). This is for future extensibility, as it will be useful in #304924: Extend drupal_error_handler to manage exceptions.#3
Here are results before and after the patch. Notice how file and line numbers were wrong: there were the line and file number of the call to the function, not those of the call to the assertion inside the function.
#4
Adds an exception handler for tests. Essentially, this is #293521: Simpletest should catch exceptions but adapted to the context of this patch.
#5
Again on chx' suggestion, here is a slight change in the backtracing logic: we now ignore all methods from DrupalWebTestCase, not only the assertions. The result actually looks great (see attached screenshot).
#6
Here a new patch that removes two unneeded lines, and adds tests of the new backtracing code to
simpletest.test.#7
I run out of ideas of what to add / remove / fix and I even channeled Dries to DamZ on IRC by asking for a new test. So, I believe this is ready.
#8
Errrr....can I review this...I worked on the other patch that this stems from and I not really sure I like what is going on. Don't have time at the moment, but should later.
#9
Yes, I'd like to hear your thoughts. I do agree with splitting this from the UI changes, however. That other patch is currently in discussion about buttons and placement and descriptions, and this patch looks like it fixes a lot of underlying things that will help us fix other testing system stuff in the meantime.
#10
The patch looks good, simpletest.test passes. Just includes the back-end changes from my patch.
Once this goes in I'll work on re-rolling the other patch to reflect this, could be fun with the large overlap and slight differences. :)
#11
Would probably be a good idea if someone ran all the tests.
#12
I ran all the test and everything passes, 5390 passes to be exact. :)
#13
Great, looks like it is ready to go.
#14
I'll be taking a look at this after supper and committing it providing I don't find any remaining issues.
#15
Just PHPdoc and indent fixes. Do not credit me.
#16
Committed this, with a couple minor PHPDoc tweaks. Thanks!
Now let's get #250047: Rework the SimpleTest results interface and clean-up of backend code in. ;)
Btw, I talked over with chx that _drupal_get_last_caller($backtrace) looks like it could be generalized a bit and re-used in multiple places, if it accepts an index of how far back to parse... _db_query() does (or used to before DBTNG?) a similar thing where it grabs the "true" calling function rather than db_query() for more sensible debug messages.
Can be handled in a follow-up patch, but I'm curious about Damien's thoughts on that.
#17
#18
Of course, that's exactly the plan. That's also why it is placed just below
drupal_error_handler()in common.inc :) I already have a mockup of this in #304924: Extend drupal_error_handler to manage exceptions, but it will require a reroll and some more testing.#19
Automatically closed -- issue fixed for two weeks with no activity.