Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
When a BrowserTestBase
test's test method:
- makes a
GET
request, while theBROWSERTEST_OUTPUT_FILE
environment variable is set, its output is logged - The logging of that output includes a mention of the test method that is responsible for that GET request
- That test method is determined by
BrowserTestBase::getTestMethodCaller()
BrowserTestBase::getTestMethodCaller()
fails when the test method lives in a parent class of the test class (e.g.EntityResourceTestBase::testGet()
,NodeResourceTest extends EntityResourceTestBase
-> running theNodeResourceTest
results intestGet()
being executed, but then fails becauseBrowserTestBase::getTestMethodCaller()
is unable to figure out the caller
This code:
// Remove all calls until we reach the current test class.
while (($caller = $backtrace[1]) &&
(!isset($caller['class']) || $caller['class'] !== get_class($this))
) {
// We remove that call.
array_shift($backtrace);
}
results in Undefined offset 1
errors in that case.
Proposed resolution
Support parent classes.
Remaining tasks
None.
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#24 | 8.3.x-24.patch | 3.63 KB | alexpott |
#24 | 22-24-interdiff.txt | 2.22 KB | alexpott |
#22 | 2822387-22.patch | 3.42 KB | Wim Leers |
Comments
Comment #2
jhedstromComment #3
jhedstromWhile this isn't a test, I can reproduce this using tests from Dynamic Entity Reference, against the latest 8.3.x. The
DynamicEntityReferenceTest
itself passes, but all the tests extending that class (DynamicEntityReferenceLocaleTest
for instance) fail with this error.This leads me to think that any class extending another class (and where that base class provides its own test case methods), will result in this error...
Comment #4
jhedstromAlthough things are green on the testbot itself, so there is also some local setup causing issues...
Comment #5
jhedstromThis test fails locally without the fix, we'll see what the testbot thinks.
Comment #6
klausiOn the testbot verbose output is disabled, so getTestMethodCaller() is never called. Can you call it manually from your test class and thereby trigger the PHP notice?
Extending BrowserTestBaseTest is also not a good idea because then it will run all the test method it includes twice. So you should create your own empty class hierarchy.
Comment #7
jhedstromThat explains it! I'll work on calling this manually.
Comment #8
jhedstromHere's an explicit test for
getTestMethodCaller()
. It passes, but the class extending it fails for now (even with the fix), because I'm uncertain what that method *should* return when a test method is called from an extending class. This is the current fail:Comment #11
klausiThis should do it, correctly looking at the class hierarchy and stopping if the current class is not involved.
Comment #12
Wim LeersConfirming. This is exactly what I'm seeing in #2737719-134: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method and #2737719-137: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method's patches against 8.3.x.
The patch fixes the problem.
Marking as critical because this is blocking #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method, which is blocking about a dozen REST patches, as well as #2712647: Update Symfony components to ~3.2.
Comment #13
Wim LeersComment #14
Wim LeersIS updated.
Comment #15
alexpottUnnecessary empty line.
Maybe an example would help make the comment easier to grok.
What happens if $backtrace[2] is not set.
Comment #16
jhedstromWorking on #15.
Comment #17
jhedstromThis should address #15.
I added a new method to the child test class that doesn't exist in the parent, so now both scenarios are tested (eg, non-overridden parent method, and a child-only test method).
Comment #18
Wim LeersThis addresses points 1, 2 and 3.
Now working on point 4.
Comment #19
Wim LeersI actually prefer #17. Much less verbose. RTBC'ing #17. Unless Alex prefers my documentation in #18.
Reuploading @jhedstrom's patch in #17 and RTBC'ing it.
Comment #20
xjmThanks all, glad to see this RTBC!
None of those match the criteria for a critical issue: https://www.drupal.org/core/issue-priority#critical They are definitely a very strong case for it to be major, though.
Comment #21
xjmHow about flipping this around:
"A parent class may provide test methods that are run for every child implementation. In that case, return the child class that was actually run, rather than the parent."
I don't think this follows our coding standards. See https://www.drupal.org/docs/develop/standards/coding-standards#controlst... and https://www.drupal.org/docs/develop/standards/coding-standards#linelength.
We could either just put it all on one line and add an inline comment to it to help people read it, or nest some ifs.
If I say this in words, it's something like: "If this caller's parent was not a class, or if [...], then this is the class that actually ran the method, so stop here."
Grammar nitpick: This is a comma splice. We can fix it by adding the word "or": "or remove the call."
Comment #22
Wim Leers#21:
debug_backtrace()
's output to the appropriate level, and then pass it to\Drupal\Core\Utility\Error::getLastCaller()
and return whatever that returns. So, you get the same value back as\Drupal\Core\Utility\Error::getLastCaller()
would.See #2763401: PHPunit browser tests should log all Mink requests for what output is generated exactly. Specifically, see
\Drupal\Tests\BrowserTestBase::getResponseLogHandler()
.Since this is only addressing comments and whitespace, re-RTBC'ing.
Comment #23
alexpottSo here's the problem for me with this change- we're always looking at next. The point of this loop is to remove everything from $backtrace until $backtrace[1] contains the information you want to be collected by Error::getLastCaller()...
I think we can be much more explicit here and therefore simpler - this works:
Comment #24
alexpottPatch implementing #23 - discussed with @catch and he felt it was simpler too.
Comment #25
Wim LeersThat looks a whole lot simpler, because it refactors the entirety of
\Drupal\Tests\BrowserTestBase::getTestMethodCaller()
. In doing so, it also simplifies the logic in HEAD.Comment #26
catchCommitted/pushed to 8.3.x, thanks!
Comment #27
klausiDid you forget to push?
Comment #28
Wim LeersHurray, unpostponed #2763401: PHPunit browser tests should log all Mink requests + #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method.
Comment #30
Wim LeersPinged catch on IRC, now he pushed. I'll have to retest #2737719 again probably :P