PHPunit browser tests should log all Mink requests

In general this looks really nice!

1. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -365,6 +375,43 @@ protected function getDefaultDriverInstance() {
   +   *   The callablke handler that will do the logging.
   

   nitpick: typo in callablke

2. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -641,7 +688,9 @@ protected function drupalGet($path, array $options = array()) {
   +    // Log only for non Goutte drivers because for those log with
   +    // ::getResponseLogHandler.
   
   @@ -944,7 +993,10 @@ protected function submitForm(array $edit, $submit, $form_html_id = NULL) {
   +    // Log only for non Goutte drivers because for those log with
   +    // ::getResponseLogHandler.
   

   Just a suggestion: I would just mention JavascriptTestBase for convencience

3. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -1636,19 +1689,24 @@ protected function htmlOutput($message) {
   -  protected function getHtmlOutputHeaders() {
   

   we could do a formatHtmlOutputHeaders

Thank you for addressing the feedback!

Looks good to me, just one small improvement, for less lines of code :)

+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -1561,15 +1614,28 @@ protected function htmlOutput($message) {
+    $flattened_headers = array_map(function($header) {
+      if (is_array($header)) {
+        return implode(';', array_map('trim', $header));
       }
       else {
...
+        return $header;

This could just be return return implode(';', array_map('trim', (array) $header));

Neat fix. Committed 761657a and pushed to 8.3.x. Thanks!

Setting to patch to be ported for eventual cherry-pick to 8.2.x once 8.2.0 is released.

+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -396,6 +406,43 @@ protected function getDefaultDriverInstance() {
+  protected function getResponseLogHandler() {

@@ -1561,15 +1614,28 @@ protected function htmlOutput($message) {
+  protected function formatHtmlOutputHeaders(array $headers) {

@@ -1754,4 +1820,24 @@ protected function getConfigSchemaExclusions() {
+  protected function getTestMethodCaller() {

If we want this in 8.2.x then we need to prefix these methods with an underscore. Alternatively if we are happy to have this fix in 8.3.x only we can just mark it as fixed.

Given that we do most of the conversions hopefully in 8.3.x the win is probably not that high. On the other hand we want to make things easier for contrib and custom modules.

I'd like to see this in 8.2.x. I have client projects with test suites that could benefit from this.

I haven't seen internal class methods prefixed with an underscore or where this is documented? You'd think that's what private was for? In any case if that's the approach, patch attached.

This passes against 8.2.x. Back to review.

This looks alright for me

@alexpott: Why do we prefix those methods with an underscore? Then they are different in 8.3.x and 8.2.x and cause headaches for people working with them in both branches?

This is our BC policy, see https://www.drupal.org/core/d8-bc-policy and Underscore-prefixed functions and methods

When @catch and @alexpott raised this issue this morning, my thought was that the underscore prefixes were only a workaround to allow us to backport fixes for (e.g.) major bugs when we could not do so otherwise. I don't think it was ever intended as a way to backport API additions to patch releases. But worth discussing the case of this issue in #2773087: [policy, maybe patch] Clarify backport policy for testing framework changes, including new BTB tests.

For me #22 alone as a reason to do this violates semver in principle, and there is a risk of disruption from it because BTB is intended as an extension point. On the other hand, I think we all consider BTB experimental in our heads, which means it's a pre-release version that we can add API to. But adding protected methods to a base class that is intended to be directly subclassed does risk breaking people's BTB tests.

Let's discuss more on #2773087: [policy, maybe patch] Clarify backport policy for testing framework changes, including new BTB tests?

+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -1766,4 +1832,24 @@ protected function getConfigSchemaExclusions() {
+    while (($caller = $backtrace[1]) &&
+      (!isset($caller['class']) || $caller['class'] !== get_class($this))
+    ) {

I haven't pinned down the exact cause yet, but this logic can lead to Undefined offset: 1 errors, but only on certain GET requests.

I added #2822387: Undefined offset 1 in BrowserTestBase::getMethodCaller() for the issue mentioned in #32.

Confirming what @jhedstrom wrote in #32. 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.

Postponing this on #2822387: Undefined offset 1 in BrowserTestBase::getMethodCaller(), so that the same bug is not introduced in 8.2.x.

#2822387: Undefined offset 1 in BrowserTestBase::getMethodCaller() landed. This can now continue for 8.2.x. But it'll need a reroll, to incorporate the fixes from #2822387: Undefined offset 1 in BrowserTestBase::getMethodCaller().

Remove "Needs reroll" tag.

We probably won't get this into 8.2.x, so we on the other hand don't need underscores then.