Convert REST tests to Guzzle (would also help with core moving from WTB to BTB)

This is an excellent novice task!

For this we need to use Guzzle\Plugin\Oauth\OauthPlugin or similar plugin

Interesting! Why?

Updated issue summary. The WIP patch was for Guzzle 3 if I recall correctly, and Guzzle 6 handles cookies now.

I do not think that the OAuth1 subscriber is necessary for this test unless @Saphyel's idea is to add the oauth-subscriber dependency to Drupal?

Modernizing the guzzle code.

1. +++ b/core/modules/rest/src/Tests/DeleteTest.php
   @@ -40,6 +41,24 @@ public function testDelete() {
   +      $form = \Drupal::formBuilder()->getForm('Drupal\user\Form\UserLoginForm');
   +      //dpm($form);
   +      $postfields = array(
   +        'name' => $account->name,
   +        'pass' => $account->pass_raw,
   +        'form_build_id' => $form['form_build_id']['#value'],
   +        'form_id' => 'user_login_form',
   +        'op' => 'Log in',
   +      );
   +
   +      //$client = drupal_container()->get('http_default_client');
   +      $url = Url::fromRoute('<front>')->setAbsolute()->toString();
   +      $client = new Client([
   +        'base_uri' => $url
   +      ]);
   +
   +      $response = $client->request('POST', 'user', $postfields);
   

   Looking at the re-roll of the original patch (thanks!), I do not think we are testing trying to do a HTTP POST to the login form. I think that we should refactor RESTTestBase::httpRequest() to use GuzzleHttp\Client instead of Curl.

Well, we need it for the entire method, not just delete requests ...

1. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
   @@ -9,6 +9,7 @@
   +use GuzzleHttp\Client;
   

   This isn't necessary if you're using the global \Drupal object below.

2. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
   @@ -148,13 +149,17 @@ protected function httpRequest($url, $method, $body = NULL, $mime_type = NULL) {
   +        $url = $this->buildUrl($url) . '?_format=json';
   

   This should either be a part of $options below as a query parameter or maybe set the Accept or Content-Type header.

3. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
   @@ -148,13 +149,17 @@ protected function httpRequest($url, $method, $body = NULL, $mime_type = NULL) {
   +        $response = $client->delete($url, $options);
   

   Simpletest's curl stuff adds the session cookie, which is why the test is returning a 403.

   http://docs.guzzlephp.org/en/latest/request-options.html#cookies

   Here's what I got when I output headers from the old test. I don't think we need to add anything other than the cookie.

   Request headers: DELETE /entity_test/1 HTTP/1.1
   User-Agent: simpletest318784;1457399848;56de2828ef7ae0.83008257;_k2P9EqKtkhPA_as87JRrLpt5aSVvYx_T6ib5T7l1I8
   Host: drupal8.dev
   Accept: */*
   Cookie: SESSf695f074497e698a540e6d347d2b0ce9=GSw1k2KM59AZldDRYy4q6tBSRnUmJUc7zcGLAEenAWA
   X-CSRF-Token: iKWznKUykM5U5PI76_tixTGKXHym0Mo0j0IvFXK96dQ
   

Also, RESTTestBase is probably going to need a new assert because assertResponse also depends on curl, and that's in WebTestBase which can't be easily refactored to be backwards-compatible.

I think we need to login with guzzle as mentioned in #1, I don't know if the cookie can be shared with drupallogin()?

Here is a start. It seems to kinda work, but we really need some form of API that the test code stays sort of readable.

.

Let's see how things turn out

Let's see, how much this fixes.

Awesome, I really want to test this out when I get some free coding time. Probably not until next week.

With just a quick glance, should we remove some duplicate code for settings that are shared across request methods? :

+++ b/core/modules/rest/src/Tests/RESTTestBase.php
@@ -86,94 +137,144 @@ protected function httpRequest($url, $method, $body = NULL, $mime_type = NULL) {
+          'http_errors' => FALSE,
...
+          'http_errors' => FALSE,
...
+          'http_errors' => FALSE,

@lokapujya
Yeah indeed, there is quite a bit of overlap I think.

run tests.

@lokapujya
This seems to be for me a bit out of scope of this issue? This is changing WebTestBase quite a bit ...

Well, how else can we update the cookie for DrupalLogout() after changing the password? I think thats the problem.

Also, It's only changing WebTestBase for the Guzzle tests. One option is that we have Guzzle completely manage the cookie from login.

Here is a start on #21. (And reroll for the head support that was added.)

What if we undo the WebTestBase, and instead overwrite the stored curl cookie with the guzzle cookie?

Well, try it out :)

Just needed that motivation.

Bad Patch. Not Rebased.

Different approach. Just logout with Guzzle session, before trying to login again.

I gave it a quick review to the last patch and found some nits.

1. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
   @@ -65,6 +71,51 @@ protected function setUp() {
   +  protected function cookieCurlOptions() {
   +
   +    $cookies = [];
   +    foreach ($this->cookies as $key => $cookie) {
   

   Can we remove this line?

2. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
   @@ -65,6 +71,51 @@ protected function setUp() {
   +    }
   +    debug($curl_options);
   +
   

   This debug method call is on purpose? should we remove it before merge it?

3. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
   @@ -86,105 +137,130 @@ protected function httpRequest($url, $method, $body = NULL, $mime_type = NULL) {
   +            'Content-Type' => $mime_type,
   +            'X-CSRF-Token' => $token
   

   missing comma here.

4. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
   @@ -86,105 +137,130 @@ protected function httpRequest($url, $method, $body = NULL, $mime_type = NULL) {
   +            'Content-Type' => $mime_type,
   +            'X-CSRF-Token' => $token
   

   missing comma.

5. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
   @@ -86,105 +137,130 @@ protected function httpRequest($url, $method, $body = NULL, $mime_type = NULL) {
   +            'Content-Type' => $mime_type,
   +            'X-CSRF-Token' => $token
   

   missing comma.

6. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
   @@ -86,105 +137,130 @@ protected function httpRequest($url, $method, $body = NULL, $mime_type = NULL) {
   +            'X-CSRF-Token' => $token
   

   missing comma.

Also, this doesn't seem a good task for a novice developer anymore, I removed the tags.

Thanks!

Well, actually the remaining points you found on this issue are a good novice task.

Hi,
I solved all the nitpicks remaining on the latest patch. Hope it works correctly.

Thank you for fixing the remaining review points.

I love it how it looks like now, but yeah someone else should review it properly now.

1. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
   @@ -60,6 +66,49 @@ protected function setUp() {
   +  protected function cookieCurlOptions() {
   

   Missing docblock. And missing a lead verb. e.g. setCookieCurlOptions() would be a better name.

2. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
   @@ -60,6 +66,49 @@ protected function setUp() {
   +
   +    foreach ($this->cookies as $key => $cookie) {
   

   Nit: Unnecessary blank line.

3. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
   @@ -60,6 +66,49 @@ protected function setUp() {
   +    $request = \Drupal::request();
   +    $cookie_params = $request->cookies;
   +    if ($cookie_params->has('XDEBUG_SESSION')) {
   +      $cookies[] = 'XDEBUG_SESSION=' . $cookie_params->get('XDEBUG_SESSION');
   +    }
   

   Let's document why we do this. I understand why. But let's document it, just like we did on the other test base classes that do this.

4. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
   @@ -81,105 +130,130 @@ protected function httpRequest($url, $method, $body = NULL, $mime_type = NULL) {
   +      'curl' => $this->cookieCurlOptions() + [
   +          CURLOPT_COOKIEJAR => $this->cookieFile,
   +          CURLOPT_HEADERFUNCTION => [&$this, 'curlHeaderCallback'],
   +        ],
   

   The bottom 3 lines have incorrect indentation: they all have two leading spaces too many.

Finally, I don't understand why we have so many mentions of "CURL" left if we are actually moving from CURL to Guzzle?

Thank you for the review wim!

Finally, I don't understand why we have so many mentions of "CURL" left if we are actually moving from CURL to Guzzle?

Here is a try to use the guzzle native but rather than CURL direct cooke option.

Just a reroll in the meantime.

l

Retesting because the patch applied fine for me locally

1. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
   @@ -72,6 +81,55 @@ protected function setUp() {
   +   * Calculates cookies used by guzzle later.
   ...
   +   *   The used CURL options in guzzle.
   

   Nit: s/guzzle/Guzzle/

2. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
   @@ -72,6 +81,55 @@ protected function setUp() {
   +    // In order to debug web tests you need to either set a cookie, have the
   +    // Xdebug session in the URL or set an environment variable in case of CLI
   +    // requests. If the developer listens to connection on the parent site, by
   +    // default the cookie is not forwarded to the client side, so you cannot
   +    // debug the code running on the child site. In order to make debuggers work
   +    // this bit of information is forwarded. Make sure that the debugger listens
   +    // to at least three external connections.
   

   Can we move this code into a trait, so we can share it with WebTestBase and BrowserTestBase?

It looks like the patch only applies to 8.2.x and the testbot was trying to run 8.1.x still even though Version is set to 8.2.x. I added the 8.2.x test and it is running.

This is blocking #2770921: Feb 21st: Convert chunk of WTB to BTB by just moving classes, changing use statements adding traits in some soft way at the moment. IMHO we should process with that.

Do you mean we should proceed with that other issue before this one, or the other way around?

Well, all I'm saying is that using guzzle would be a improvement not just for the sake of it, which might some people believe in.

Basically we could add to the issue summary that the motivation is not only making the tests more readable, but would improve core developer experience to move to BTB.

Riight! Okay, thanks, that makes sense! :)

I would argue that we should invent an ApiTestBase class as sibling of BrowserTestBase (not a child of it!) which uses Guzzle as a client but does not have all the Mink methods.

Nice idea!

Interesting :) Would that be appropriate to introduce here?

Also, the fact that UpdateTest shows POST requests, not PATCH requests, is maddening. The reason for that seems at least equally maddening:

  /**
   * {@inheritdoc}
   *
   * This method is overridden to deal with a cURL quirk: the usage of
   * CURLOPT_CUSTOMREQUEST cannot be unset on the cURL handle, so we need to
   * override it every time it is omitted.
   */
  protected function curlExec($curl_options, $redirect = FALSE) {
    if (!isset($curl_options[CURLOPT_CUSTOMREQUEST])) {
      if (!empty($curl_options[CURLOPT_HTTPGET])) {
        $curl_options[CURLOPT_CUSTOMREQUEST] = 'GET';
      }
      if (!empty($curl_options[CURLOPT_POST])) {
        $curl_options[CURLOPT_CUSTOMREQUEST] = 'POST';
      }
    }
    return parent::curlExec($curl_options, $redirect);
  }

In other words: 1000 times yes to this issue.

Bumping to major because the brittleness of REST's test coverage has caused major problems, major slowdowns during the 8.2.x cycle and will continue to do so unless we address it. This is one of the key things to make REST's test coverage trustworthy.

Having fought our current tests once again, this time at #2795965-5: REST requests with invalid X-CSRF-Token header get "missing " mesage, I'm dropping everything to do this first. Not using CURL would have made several things there better. Plus, this is pretty much a hard blocker for #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method.

@Wim Leers++

On the other hand its not cool, that you also had that level of pain with the existing test helpers.

First, a straight reroll so it applies to HEAD again. (This conflicted with #2681911: REST requests without X-CSRF-Token header: unhelpful response significantly hinders DX, should receive a 401 response.)

Still fails.

Quoting #1, from November 2012 (!!!!!!!!!!!):

But I even failed at issuing a successful login POST request and I'm giving up for now. Attached is the code I experimented with for future reference. I always got 403 access denied pages as response.

Every single word of this is still true! Incredible. Although apparently that's since the refactor in #44/#46. Still trying to figure out the root cause. I verified that a cookie is correctly retrieved and sent. (By requesting /user, which indeed redirects to the authenticated user's profile page.) I've also ruled out that a missing ?_format=hal_json is the cause. No idea yet what the problem is.

I've tracked the problem down further. RESTTestBase::cookies()'s cookie jar does contain the correct cookie name and value. But it is not sent to the server. So, somewhere along the way, guzzle loses it. Either because we're not using Guzzle in the right way (likely), or Guzzle is not set up in the right way (also somewhat likely, according to the documentation in \GuzzleHttp\Middleware::cookies()), or … Guzzle is broken (very unlikely).

Since I'm writing this on a train, I don't have access to the full Guzzle documentation, but at least we now know exactly where things are going wrong: Guzzle is not sending cookies.

I've tracked the problem down further. RESTTestBase::cookies()'s cookie jar does contain the correct cookie name and value. But it is not sent to the server. So, somewhere along the way, guzzle loses it. Either because we're not using Guzzle in the right way (likely), or Guzzle is not set up in the right way (also somewhat likely, according to the documentation in \GuzzleHttp\Middleware::cookies()), or … Guzzle is broken (very unlikely).

Since I'm writing this on a train, I don't have access to the full Guzzle documentation, but at least we now know exactly where things are going wrong: Guzzle is not sending cookies.

I'm happy to have a look at that this morning, if you don't mind.

Please do! That'd be splendid!

Cool, glad you saw my post in your shaky train internet :)

Let's see

There have been two problems:

• No Domain, see \GuzzleHttp\Cookie\SetCookie::validate
• Values as array

When a cookie is removed (a user is logged out and the session is removed), its send back with value 'deleted'. We set back this cookie, so the page cache believes we have a session opened.
I honestly don't get 100% yet, why this works in WTB, maybe we have a slightly different invoked CURL or so. This patch ensures that we don't send back 'deleted' cookies.

Awesome, great detective work, @dawehner! The remaining fail looks trivial to solve: Guzzle doesn't lowercase all headers like curl does. So these need to be updated to have the proper casing:

      unset($get_headers['date']);
      unset($head_headers['date']);
      unset($get_headers['content-length']);
      unset($head_headers['content-length']);
      unset($get_headers['x-drupal-dynamic-cache']);
      unset($head_headers['x-drupal-dynamic-cache']);

Awesome, great detective work, @dawehner! The remaining fail looks trivial to solve: Guzzle doesn't lowercase all headers like curl does. So these need to be updated to have the proper casing:

Is this WTB which is doing the lowering?

Apparently, yes, in \Drupal\simpletest\WebTestBase::drupalGetHeaders() there is a strtolower().

I'm done for this week, probably. Feel free to bring it home :)

Well, then this should be it.

I think this is ready. This is a big step forward for the REST test coverage. Let's get this in, and then #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method can provide the next round of REST test improvements.

(This issue doesn't change anything about test comprehensiveness, but it improves test debuggability and maintainability. And as a bonus, it helps #2770921: Feb 21st: Convert chunk of WTB to BTB by just moving classes, changing use statements adding traits!)

Oh and IMO this should go in 8.2.x, because #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method is also aimed at 8.2.x (to find any remaining REST bugs in 8.2.x, and to aid overall reliability of REST in 8.2.x), and this is a soft blocker for that issue.

Conflicts with #2795965: REST requests with invalid X-CSRF-Token header get "missing " mesage - which has just landed...

#84's Reroll

Fixed small error in my last reroll.

Thank you for the reroll @gnuget!

<3 gnuget — thanks a lot!

+++ b/core/modules/rest/src/Tests/UpdateTest.php
@@ -259,6 +259,8 @@ public function testUpdateUser() {
+    $this->httpRequest('user/logout', 'GET');
+    $this->loggedInUser = FALSE;

How come this change is necessary?

    // In order to debug web tests you need to either set a cookie, have the
    // Xdebug session in the URL or set an environment variable in case of CLI
    // requests. If the developer listens to connection on the parent site, by
    // default the cookie is not forwarded to the client side, so you cannot
    // debug the code running on the child site. In order to make debuggers work
    // this bit of information is forwarded. Make sure that the debugger listens
    // to at least three external connections.
    $request = \Drupal::request();
    $cookie_params = $request->cookies;
    if ($cookie_params->has('XDEBUG_SESSION')) {
      $cookies[] = 'XDEBUG_SESSION=' . $cookie_params->get('XDEBUG_SESSION');
    }
    // For CLI requests, the information is stored in $_SERVER.
    $server = $request->server;
    if ($server->has('XDEBUG_CONFIG')) {
      // $_SERVER['XDEBUG_CONFIG'] has the form "key1=value1 key2=value2 ...".
      $pairs = explode(' ', $server->get('XDEBUG_CONFIG'));
      foreach ($pairs as $pair) {
        list($key, $value) = explode('=', $pair);
        // Account for key-value pairs being separated by multiple spaces.
        if (trim($key, ' ') == 'idekey') {
          $cookies[] = 'XDEBUG_SESSION=' . trim($value, ' ');
        }
      }
    }

Let's file a new issue to move this into a trait to get the XDEBUG_SESSION from the request and use it here, BrowserTestBase and SimpleTest...

[Edit: complete a sentence]

Running down memory lane (the comment history)...

We were trying to solve the issue of WTB adding session cookies on requests when changing the logged in user's password (#26). One approach was to add a conditional in httpRequest for Guzzle or Curl, or a modification of drupalLogout (up to #33), but that was changed in #38 to the current implementation.

Perhaps a comment based on @lokapujya's comments in #26, #29 and #38?

What about "Log out the user to clear the cookies used by the Guzzle client so that a log in request can be made for the changed user."?

I had time to make a patch of my suggestion above.

#97's Small fix.

Ups. I'm personally happy with this issue. Anyone else?

Looks like all feedback from #93 has been addressed. Thanks all!

Committed d27d6c3 and pushed to 8.3.x. Thanks!

Should we backport this to 8.2.x once 8.2.0 is out? I think we should because test divergence is quite risky.

There is a slightly risk because people might leverage the base class, and well, as we have seen on the UpdateTest, they might be adapted.

Should we backport this to 8.2.x once 8.2.0 is out? I think we should because test divergence is quite risky.

Yes.

The reason I worked on this is because it helps #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method, which aims to bring comprehensive test coverage for REST resources to 8.2.x. And in the process discover bugs (already happening), and then fixing them. Because bug fixes can go in 8.2.x, and we should make REST as reliable as possible in 8.2.x.

This helps with that.

Some documentation and maybe a change record would be nice as well to outline the new capabilities in the test runner.

@mradcliffe you mean for contrib/custom modules that ship with REST resource plugins, and have test coverage via RESTTestBase?

@wimleers, or for someone who might want to use Guzzle in tests in general.

Oh, you mean documentation for the new trait! That makes sense.

Straight reroll.

Test fail for Test #521646 seems to be random. I re-rolled and didn't see any changes to the re-roll, ran the test locally on 8.2.x HEAD and it passed. Then I queued up the tests again and it passed.

Setting back to previous status set by Wim Leers in #113.

Are we sure that's not a new random fail introduced by the guzzle conversion? It's a REST test after all.

I reviewed the patch and re ran the tests and all looks good (also this was already committed to 8.3.x) so I really really think this is RTBC.

#2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method landed, which made the RESTTestBase-based test coverage mostly obsolete. #2824576: Delete old REST test coverage: (Read|Create|Update|Delete)Test, deprecate RESTTestBase then deleted most of that obsolete test coverage.

Because of that, the value of this issue is much smaller now (but the insights I got here helped me a lot during #2737719!). RESTTestBase has been updated in 8.3, but not 8.2. So, the only thing we gain here, is improved consistency between 8.2 and 8.3.

A reroll should be easy: just removing hunks. Then I'll RTBC and a committer can decide.

Considering we've deprecated the class I think we should just leave it alone until we remove it.

It is marked as deprecate already, so yeah, I think we should just close this issue.

Alright, thanks for chiming in, @alexpott & @dawhener!

Actually, this seems more accurate. This did get committed to 8.3.x.