Fix AssetBuilderTest when running on WordPress. Use Guzzle.

[seamuslee001]

Overview

This fixes the AssetBuilder E2E test testInvalid when running on wordpress

Before

Test fails on wordpress

After

Test passes

ping @totten @eileenmcnaughton

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[seamuslee001]

Jenkins re test this please

[totten]

@seamuslee001 This is doing two things, right?

1. Changing the HTTP client used for the test (file_get_contents() -> =GuzzleHttp)
2. Relaxing the assertion to not involve HTTP content

On general aesthetic grounds, changing the client to Guzzle is fine. 👍. I'm not clear if the change is needed for the fix, but it's fine either way.

Regarding the assertEmpty() claim, here's a bit of context: one of the original/main use-cases for asset-builder was loading a JSON document (via AJAX) which aggregated various strings needed for Angular apps. When writing the frontend code which receives the AJAX response, one should include error-handling logic.

Personally, I really dislike when an AJAX end-point delivers its error in a format that can't be read easily -- e.g. when it responds with a fully-formed HTML web-page. (Basically anything is better - short plain-text, short impromptu XML, impromptu JSON, empty string, etc.) But regardless of the format, the main thing is to be consistent - given the same request and the same underlying error-condition, the error should be reported in the same way. This makes it easier to reproduce bugs, to write error-handling code, to educate/train people in debugging, etc. The assertEmpty() basically locked in the behavior I saw on D7.

Now, maybe I'm misinterpreting... but in removing assertEmpty(), there seems to be an implicit claim that D7 and WP are reporting error-conditions in different formats? That's a bug IMHO.

NOTE 1: I realize this is a bit persnickity, and I wouldn't be as verbal if we were talking about one random end-point, but this particular end-point is meant to handle a lot of different assets, so it's more important than average.

NOTE 2: Maybe D7+WP are really dead-set on reporting errors differently. But I think we should take a stab at reconciling the error-formats before saying it can't be done.

[seamuslee001]

@totten locally it seemed the Wordpress site was kicking back more error than on drupal side. Maybe that is just Wordpress. Changing to guzzle fixed it. Also Guzzles’s excepton message doesn’t include the thing HTTP only the status code and not found iirc

[seamuslee001]

@totten i did some more debugging and i put in error_reporting() call in to see what values we were playing with on my wp-test install the value was

4983 = a combo of E_ERROR, E_WARNING, E_PARSE, E_CORE_ERROR, E_CORE_WARNING, E_COMPILE_ERROR, E_USER_ERROR, E_USER_WARNING, E_RECOVERABLE_ERROR

on Drupal it was 1 = E_ERROR

When i make the change like this

diff --git a/tests/phpunit/E2E/Core/AssetBuilderTest.php b/tests/phpunit/E2E/Core/AssetBuilderTest.php
index 19df9a3c1c..16f6c0752d 100644
--- a/tests/phpunit/E2E/Core/AssetBuilderTest.php
+++ b/tests/phpunit/E2E/Core/AssetBuilderTest.php
@@ -162,9 +162,11 @@ class AssetBuilderTest extends \CiviEndToEndTestCase {
   public function testInvalid() {
     \Civi::service('asset_builder')->setCacheEnabled(FALSE);
     $url = \Civi::service('asset_builder')->getUrl('invalid.json');
+    $originalReportingLevel = error_reporting(1);
     $this->assertEmpty(file_get_contents($url));
     $this->assertNotEmpty(preg_grep(';HTTP/1.1 404;', $http_response_header),
       'Expect to find HTTP 404. Found: ' . json_encode(preg_grep(';^HTTP;', $http_response_header)));
+    error_reporting($originalReportingLevel);
   }

 }

Instead of what is in the PR I get this failure

There was 1 failure:

1) E2E\Core\AssetBuilderTest::testInvalid
Expect to find HTTP 404. Found: ["HTTP\/1.0 404 Not Found"]
Failed asserting that an array is not empty.

/home/seamus/buildkit/build/wp-test/wp-content/plugins/civicrm/civicrm/tests/phpunit/E2E/Core/AssetBuilderTest.php:168
/home/seamus/buildkit/extern/phpunit5/phpunit5.phar:598

FAILURES!
Tests: 1226, Assertions: 1456, Failures: 1, Skipped: 404.

[seamuslee001]

the original failure is

file_get_contents(http://wp-test/?page=CiviCRM&q=civicrm%2Fasset%2Fbuilder&an=invalid.json&ap=&ad=d1370334f089e6b5c0123e48857cdba3): failed to open stream: HTTP request failed! HTTP/1.0 404 Not Found

[totten]

(@seamuslee001) tl/dr it seems WordPress has a different error reporting level to Drupal and when i apply Drupal's one it is giving similar but different output

If that's the case.. then maybe the page-controller for the asset builder should do something to normalize the error-reporting?

(Note: I'm not talking about all routes - it makes sense for normal user-facing routes to follow some policy/mechanism dependent on the CMS. But the asset-builder route is unusual in that it's not user-facing; the content is loaded indirectly -- via <script> or AJAX -- and therefore doesn't have the same UX/DX.)

[eileenmcnaughton]

@totten @seamuslee001 is this better in than out? If it's fixing a false test fail then I would say yes - even if it's not the 'best' methodology

[seamuslee001]

I would want to try and pin @totten down on which he thinks is better to go with.

[eileenmcnaughton]

@totten ....

[seamuslee001]

I have logged the underlying problem in the lab at https://lab.civicrm.org/dev/wordpress/issues/29

[totten]

We tried the simpler test approach in #14212, but it didn't actually work out well when running the full matrix in a clean environment.

It appears that AssetBuilderTest should have been failing all along -- i.e. this code specifically says it should return error text, and this check specifically asserts that it should not. So the test-failure on WP was a proper indication of a contradiction in the code. Which flips the question around: why was it passing on D7?

I don't have a clear answer to that, but... if we update the test a bit (use Guzzle as the HTTP client; check for the intended+actual output), then the test passes on both D7+WP. The problem seems to be when the test-mechanism uses file_get_contents() as an HTTP client on D7 (i.e. file_get_contents() on D7 doesn't return the error-body).

The discrepancy could certainly confuse people who use file_get_contents() on D7 and WP, but it doesn't speak to the correctness of the AssetBuilder's server-side code, so it's not really in scope. IMHO, as long as the test has proper assertions, it's fair-game to side-step the wonky HTTP clients by switching to Guzzle.