Problem/Motivation

When a BrowserTestBasetest's test method:

  1. makes a GET request, while the BROWSERTEST_OUTPUT_FILE environment variable is set, its output is logged
  2. The logging of that output includes a mention of the test method that is responsible for that GET request
  3. That test method is determined by BrowserTestBase::getTestMethodCaller()
  4. BrowserTestBase::getTestMethodCaller() fails when the test method lives in a parent class of the test class (e.g. EntityResourceTestBase::testGet(), NodeResourceTest extends EntityResourceTestBase -> running the NodeResourceTest results in testGet() being executed, but then fails because BrowserTestBase::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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhedstrom created an issue. See original summary.

jhedstrom’s picture

Status: Active » Needs review
FileSize
622 bytes
jhedstrom’s picture

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

jhedstrom’s picture

Although things are green on the testbot itself, so there is also some local setup causing issues...

jhedstrom’s picture

This test fails locally without the fix, we'll see what the testbot thinks.

klausi’s picture

Status: Needs review » Needs work

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

jhedstrom’s picture

Assigned: Unassigned » jhedstrom

On the testbot verbose output is disabled, so getTestMethodCaller() is never called.

That explains it! I'll work on calling this manually.

jhedstrom’s picture

Assigned: jhedstrom » Unassigned
Status: Needs work » Needs review
FileSize
1.53 KB
2.13 KB

Here'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:

1) Drupal\FunctionalTests\GetTestMethodCallerExtendsTest::testGetTestMethodCaller
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    'file' => '.../core/tests/Drupal/FunctionalTests/GetTestMethodCallerTest.php'
-    'line' => 18
-    'function' => 'Drupal\FunctionalTests\GetTestMethodCallerTest->testGetTestMethodCaller()'
-    'class' => 'Drupal\Tests\BrowserTestBase'
-    'object' => Drupal\FunctionalTests\GetTestMethodCallerExtendsTest Object (...)
-    'type' => '->'
+    'file' => '-'
+    'line' => 100
+    'function' => 'main()'
     'args' => Array ()
 )

The last submitted patch, 8: 2822387-08-TEST-ONLY.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 8: 2822387-08.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
2.58 KB
1.74 KB

This should do it, correctly looking at the class hierarchy and stopping if the current class is not involved.

Wim Leers’s picture

Priority: Normal » Critical
Status: Needs review » Reviewed & tested by the community
Wim Leers’s picture

Wim Leers’s picture

Issue summary: View changes

IS updated.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
    @@ -1850,7 +1850,19 @@ protected function getTestMethodCaller() {
    +
    

    Unnecessary empty line.

  2. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
    @@ -1850,7 +1850,19 @@ protected function getTestMethodCaller() {
    +      // The call might be done in a parent class. Stop if the next call does
    +      // not go back to our test class.
    

    Maybe an example would help make the comment easier to grok.

  3. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
    @@ -1850,7 +1850,19 @@ protected function getTestMethodCaller() {
    +        $next = $backtrace[2];
    

    What happens if $backtrace[2] is not set.

  4. Nice tests - I wonder if both classes should implement a method that is not overridden and contains a similar test.
jhedstrom’s picture

Assigned: Unassigned » jhedstrom

Working on #15.

jhedstrom’s picture

Assigned: jhedstrom » Unassigned
Status: Needs work » Needs review
FileSize
2.19 KB
3.08 KB

This should address #15.

I wonder if both classes should implement a method that is not overridden and contains a similar test.

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

Wim Leers’s picture

FileSize
1.58 KB
3.52 KB

This addresses points 1, 2 and 3.

Now working on point 4.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
3.08 KB

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

xjm’s picture

Priority: Critical » Major

Thanks all, glad to see this RTBC!

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

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.

xjm’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
    @@ -1850,7 +1850,19 @@ protected function getTestMethodCaller() {
    +      // The call might be done in a parent class. Stop if the next call does
    +      // not go back to our test class. This can occur when a test class is
    +      // extending another test class that provides its own test methods.
    

    How 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."

  2. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
    @@ -1850,7 +1850,19 @@ protected function getTestMethodCaller() {
    +        if (empty($next['class']) || (!is_subclass_of($this, $next['class'])
    +          && $next['class'] !== get_class($this))
    +        ) {
    

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

  3. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
    @@ -1850,7 +1850,19 @@ protected function getTestMethodCaller() {
    +      // Otherwise we have not reached our test class yet, remove that call.
    

    Grammar nitpick: This is a comma splice. We can fix it by adding the word "or": "or remove the call."

  4. Does this output meaningful line numbers and method names? E.g. if the method was in fact provided by a parent class, what line number and method name will it say?
Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
3.42 KB
1.65 KB

#21:

  1. Ok. But your proposed text is wrong. Used your suggestion, but amended it for correctness. Hope it's still clear.
  2. +1 for this. But the existing code in this class was already doing such strange things (presumably to stay within 80 cols).
  3. Your proposed text change was wrong. Replaced the comma by a colon.
  4. Well, we unravel 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.

alexpott’s picture

+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -1850,7 +1850,18 @@ protected function getTestMethodCaller() {
     while (($caller = $backtrace[1]) &&
...
+        $next = $backtrace[2];

So 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:

  protected function getTestMethodCaller() {
    $backtrace = debug_backtrace();

    // Find the test class that has the test method. In the case that the test
    // method is implemented by a parent then the class name of $this will not
    // be part of the backtrace. In that case we process the backtrace until
    // the caller is not a subclass of $this and return the previous caller.
    while ($caller = Error::getLastCaller($backtrace)) {
      if (isset($caller['class'])) {
        if ($caller['class'] === get_class($this)) {
          break;
        }
        elseif (isset($last_caller) && !is_subclass_of($this, $caller['class'])) {
          // The current caller is not part of the test class inheritance
          // therefore we want the last caller since that has to be the test
          // class.
          $caller = $last_caller;
          break;
        }
      }
      $last_caller = $caller;
      array_shift($backtrace);
    }

    return $caller;
  }
alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.22 KB
3.63 KB

Patch implementing #23 - discussed with @catch and he felt it was simpler too.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

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

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.3.x, thanks!

klausi’s picture

Status: Fixed » Reviewed & tested by the community

Did you forget to push?

Wim Leers’s picture

  • catch committed d498a39 on 8.3.x
    Issue #2822387 by jhedstrom, Wim Leers, klausi: Undefined offset 1 in...
Wim Leers’s picture

Status: Reviewed & tested by the community » Fixed

Pinged catch on IRC, now he pushed. I'll have to retest #2737719 again probably :P

Status: Fixed » Closed (fixed)

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