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.
CommentFileSizeAuthor
#15 simpletest-backend-rework.patch15.22 KBchx
#6 simpletest-backend-rework-6.patch15.01 KBdamien tournoud
#5 new-backtracing.png176.3 KBdamien tournoud
#5 simpletest-backend-rework-5.patch10.69 KBdamien tournoud
#4 simpletest-backend-rework-4.patch10.74 KBdamien tournoud
#3 before-rework.png156.87 KBdamien tournoud
#3 after-rework.png159.86 KBdamien tournoud
#2 simpletest-backend-rework-3.patch9.77 KBdamien tournoud
#1 simpletest-backend-rework-2.patch9.08 KBdamien tournoud
simpletest-backend-rework.patch8.89 KBdamien tournoud

Comments

damien tournoud’s picture

damien tournoud commented
StatusFileSize
new simpletest-backend-rework-2.patch9.08 KB

After a discussion with chx on the IRC, I rewrote the backtracing logic a bit and added useful comments.

damien tournoud’s picture

damien tournoud commented
StatusFileSize
new simpletest-backend-rework-3.patch9.77 KB

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.

damien tournoud’s picture

damien tournoud commented
StatusFileSize
new after-rework.png159.86 KB
new before-rework.png156.87 KB

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.

damien tournoud’s picture

damien tournoud commented
StatusFileSize
new simpletest-backend-rework-4.patch10.74 KB

Adds an exception handler for tests. Essentially, this is #293521: Simpletest should catch exceptions but adapted to the context of this patch.

damien tournoud’s picture

damien tournoud commented
StatusFileSize
new simpletest-backend-rework-5.patch10.69 KB
new new-backtracing.png176.3 KB

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).

damien tournoud’s picture

damien tournoud commented
StatusFileSize
new simpletest-backend-rework-6.patch15.01 KB

Here a new patch that removes two unneeded lines, and adds tests of the new backtracing code to simpletest.test.

chx’s picture

chx commented
Status: Needs review » Reviewed & tested by the community

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.

boombatower’s picture

boombatower commented

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.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs review

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.

boombatower’s picture

boombatower commented
Status: Needs review » Reviewed & tested by the community

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. :)

boombatower’s picture

boombatower commented

Would probably be a good idea if someone ran all the tests.

maartenvg’s picture

maartenvg commented

I ran all the test and everything passes, 5390 passes to be exact. :)

boombatower’s picture

boombatower commented

Great, looks like it is ready to go.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

I'll be taking a look at this after supper and committing it providing I don't find any remaining issues.

chx’s picture

chx commented
StatusFileSize
new simpletest-backend-rework.patch15.22 KB

Just PHPdoc and indent fixes. Do not credit me.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

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.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed
damien tournoud’s picture

damien tournoud commented

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.

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.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

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