PHP notices are not detected for headless browser-triggered requests in FunctionalJavascript tests

Crossposting from the other issue.

larowlan

:
\Drupal\Core\Test\HttpClientMiddleware\TestHttpClientMiddleware::__invoke

tl;dr the site under test should communicate php errors back to the runner via `X-Drupal-Assertion` headers

xjm:

So this converts warnings etc. into exceptions to raise fails, but how do the headers get set in the first place?

larowlan:

by the error handler

see _drupal_error_header

which is called from _drupal_log_error

so if you could ask him to debug inside _drupal_log_error - there may be something preventing that hunk from running in this scenario, which we need to fix

@Wim Leers:

I'm 90% certain that this simply never worked for functional JS tests. It's based on the user agent that the relevant middleware gets registered — \Drupal\Core\CoreServiceProvider::registerTest() does:

  /**
   * Registers services and event subscribers for a site under test.
   *
   * @param \Drupal\Core\DependencyInjection\ContainerBuilder $container
   *   The container builder.
   */
  protected function registerTest(ContainerBuilder $container) {
    // Do nothing if we are not in a test environment.
    if (!drupal_valid_test_ua()) {
      return;
    }
    // The test middleware is not required for kernel tests as there is no child
    // site. DRUPAL_TEST_IN_CHILD_SITE is not defined in this case.
    if (!defined('DRUPAL_TEST_IN_CHILD_SITE')) {
      return;
    }
    // Add the HTTP request middleware to Guzzle.
    $container
      ->register('test.http_client.middleware', 'Drupal\Core\Test\HttpClientMiddleware\TestHttpClientMiddleware')
      ->addTag('http_client_middleware');
  }

… but when using functional JS tests, drupal_valid_test_ua() never returns TRUE because

  protected function prepareRequest() {
    $session = $this->getSession();
    $session->setCookie('SIMPLETEST_USER_AGENT', drupal_generate_test_ua($this->databasePrefix));
  }

only runs when using $this->drupalGet(…), not when the browser itself makes AJAX requests due to e.g. typing something or clicking something.

That's my theory at least 🙈

The testing subsystem maintainers might be able to help with an alternate solution for this also, but definitely we'll want FM signoff as this significantly impacts yada yada.

Shouldn't the browser accept and send cookies? We're using a real browser here.
If the test UA didn't get set then we wouldn't get logged in sessions in the test and any request the browser makes back to the site under test would punch through the valid test UA and hit the live site (not the prefixed one).

So I don't think the issue is the cookies, I think the issue is that there is no one there to hear the exceptions.

With requests made via the driver, we've got stuff that reads back the headers. But with requests the browser makes we don't. i.e I suspect the responses coming back with the ajax would have the headers - but they're not being read outside of drupalGet.

We've got some prior art here - #3070521: Trigger deprecation notifications on JavaScript deprecations to notify developers that deprecated code is being used - but all of that seems very brittle, sending a header in a child site, to be read in JS, to be stored in session storage, to be read back by the driver on tearDown 😱

Another alternative is something like https://www.previousnext.com.au/blog/making-your-drupal-8-kernel-tests-f... where we'd have a specialised logger that we setup in any functional JS test that we then check in the tear down. We could just have it write to a key value collection or something so it could be used to communicate from the child site to the runner.

The thunder distribution checks the logs on tearDown for this reason - https://github.com/thunder/thunder-distribution/blob/54da56bbf30ef0a3c81... This works for Thunder because we don't have any tests that expect a PHP notice or error - core has such tests and similar tests probably exist in contrib and custom code too. One problem with this approach is that error gets reported in tearDown so it can be very disconnected from where the error actually occurs.

I do think that if #3070521: Trigger deprecation notifications on JavaScript deprecations to notify developers that deprecated code is being used is working then leveraging is probably the best bet. If it is brittle then we need to change the way that works too.

We can split off child issues for the individual failures listed in https://www.drupal.org/pift-ci-job/2154378.

Tests that don't fail when they should is a major bug IMO.

@chr.fritsch pointed me to #3061251: Expose Ajax errors in Javascript tests which is related. When ajax does fail it can be really hard to work out what is wrong.

This is related to #2400477: Drupal never displays non-fatal error messages to the screen during Ajax requests and these issues almost feel like dupes - different solutions though.

There's also the wider issue about reporting JS errors - see #1419652: JavaScript logging and error reporting

I ended up here from [3225381] which is a regression caused by [3084436]. We unset a render array key which caused a array key undefined notice in a template function. but the test didn't catch the notice, for reasons described here.

Because the standard error handler hides these from the ajax response, the forms generally work in real life. I got stuck on this because the ajax call above was failing for me on my local version, and I couldn't export a config. Took some digging, but long story short, I had devel installed and had the error handler set to none, which reverts back to the standard php handler, plus error_reporting E_ALL for local development. All this together means the notice is printed in the response body, above the JSON. All that to say, with a local setup like this, those failures become real, and those forms/buttons don't work. Easy enough to re-enable the drupal error handler in devel for me, but it confused me for a bit.

We can split off child issues for the individual failures listed in https://www.drupal.org/pift-ci-job/2154378.

The one thing they found in [3225381] is that it turns out to be difficult to write a failing test for those bugs without having some framework in the test system to detect them. I posted a proof of concept test in that issue which disables the drupal error handler for that test, but it would be much better to globally expose the notices and warnings, rather than changing the error handler for each of the tests. I haven't gone through all the failures here, but if they are easy enough to fix, it might be worth considering doing them all at once along with a system to prevent future warnings and notices from cropping up in Ajax requests.