ResourceTestBase should not have a timeout

+++ b/core/modules/rest/tests/src/Functional/ResourceTestBase.php
@@ -136,9 +136,13 @@ public function setUp() {
-    // Set up a HTTP client that accepts relative URLs.
+    // Set up a HTTP client that accepts relative URLs and turns off curl
+    // timeouts.
     $this->httpClient = $this->container->get('http_client_factory')
-      ->fromOptions(['base_uri' => $this->baseUrl]);
+      ->fromOptions([
+        'base_uri' => $this->baseUrl,
+        'timeout' => NULL,
+      ]);

Couldn't this get the HTTP client from the mink instance, which is somewhere in $this->getSession()->client->getClient() or so ...

1. +++ b/core/modules/rest/tests/src/Functional/ResourceTestBase.php
   @@ -344,7 +344,8 @@ protected function grantPermissionsToTestedRole(array $permissions) {
   -    return $this->httpClient->request($method, $url->toString(), $request_options);
   +    $uri = UriResolver::resolve(uri_for($this->baseUrl), uri_for($url->toString()));
   +    return $this->httpClient->request($method, $uri, $request_options);
   

   Uhm… wow.

   So this is the alternative for not setting base_uri on the Guzzle HTTP client apparently. Wow.

   Let's at least document it.

   Also… ::resolve() expects the base URL second, not first! So AFAICT this can't work.

2. +++ b/core/modules/rest/tests/src/Functional/ResourceTestBaseTest.php
   @@ -0,0 +1,60 @@
   +class ResourceTestBaseTest extends ResourceTestBase {
   

   I don't think this test is very useful…?

Also, I don't see how this is critical?

Wait, what?!

The IS in #2784159: Remove CURL timeout in BTB says:

While this is not a problem in normal test running, it is a problem when you debug the site using xdebug.

It does not mention random failures. If it was, it'd have been critical too!

I'm confused :(

In #2844509: Random fails in BrowserTestBase tests with Operation timed out after 30001 milliseconds with 0 bytes we finally figured out the 0 byte intermittent was cause by the 30 sec CURL timeout, and we already had an issue about that.

Thanks for doing that issue queue archeology! Confirming critical status then.

I have seen this new issue cause a random failure, https://www.drupal.org/pift-ci-job/637415

Thanks, that then explicitly confirms this is not just theoretical. Further confirmation of criticalness.

1. +++ b/core/modules/rest/tests/src/Functional/ResourceTestBase.php
   @@ -344,7 +344,9 @@ protected function grantPermissionsToTestedRole(array $permissions) {
   +    // Set up a HTTP client that accepts relative URLs.
   +    $uri = UriResolver::resolve(uri_for($this->baseUrl), uri_for($url->toString()));
   +    return $this->httpClient->request($method, $uri, $request_options);
   

   I don't get why we don't force the $url to be absolute. I assume that we then no longer have to do this dance here?

2. +++ b/core/modules/rest/tests/src/Functional/ResourceTestBaseTest.php
   @@ -0,0 +1,60 @@
   +    $this->assertTrue(30 < $time_end - $time_start);
   

   Can't you use assertLessThan?

#16.1: the original intent was for us to be able to do $this->httpClient->request('/node/5?_format=json') for example. Because that'd improve legibility of the tests. But I think we're already generating absolute URLs everywhere. So perhaps we can just remove this support for relative URLs altogether. IOW: +1 :)

#16.1: the original intent was for us to be able to do $this->httpClient->request('/node/5?_format=json')

If we want to support such things we would ideally change the central guzzle client. I believe for now its a more important to fix the critical issue, right?

Thank you!

+++ b/core/modules/rest/tests/src/Functional/ResourceTestBaseTest.php
@@ -0,0 +1,60 @@
+   * Ensures that HTTP client turns off curl timeout.

curl => cURL

Can be fixed on commit.

#18: absolutely! That's what I implied in #17, didn't I? :)

I wanted to confirm RTBC, but I found something small yet questionable:

1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/User/UserResourceTestBase.php
   @@ -213,7 +213,7 @@ public function testPatchDxForSecuritySensitiveBaseFields() {
   -    $response = $this->httpClient->request('POST', Url::fromRoute('user.login.http')->setRouteParameter('_format', 'json')->toString(), $request_options);
   +    $response = $this->request('POST', Url::fromRoute('user.login.http')->setRouteParameter('_format', 'json'), $request_options);
   

   So here we're removing $this->httpClient…

2. +++ b/core/modules/rest/tests/src/Functional/ResourceTestBase.php
   @@ -344,7 +342,7 @@ protected function grantPermissionsToTestedRole(array $permissions) {
   -    return $this->httpClient->request($method, $url->toString(), $request_options);
   +    return $this->httpClient->request($method, $url->setAbsolute(TRUE)->toString(), $request_options);
   

   Then why don't we do the same here?

3. +++ b/core/modules/rest/tests/src/Functional/ResourceTestBase.php
   @@ -136,9 +136,7 @@ public function setUp() {
   -    // Set up a HTTP client that accepts relative URLs.
   -    $this->httpClient = $this->container->get('http_client_factory')
   -      ->fromOptions(['base_uri' => $this->baseUrl]);
   +    $this->httpClient = $this->getSession()->getDriver()->getClient()->getClient();
   

   Then we could simply delete these lines altogether!

Yep!

Reduced API surface. Less chance of breaking contrib/custom tests.

+++ b/core/modules/system/tests/modules/test_page_test/src/Controller/Test.php
@@ -108,4 +108,15 @@ public function renderPipeInLink() {
+    sleep(42);

I don't think we should be adding this test in this issue. If the main test http client has a timeout of 30 secs then we'd have way more test fails. The fault here was creating a new client in ResourceTestBase - swapping to use the main test http client is the fix.

Re #26 - it is great that you've proved the fix in this issue.

Back to RTBC

Committed and pushed 3e20814 to 8.4.x and f72d600 to 8.3.x. Thanks!

I already raised my concerns about this test in #9.2, I'm glad it was not committed :)

Glad to have this in!