Problem/Motivation

Working on #2870458: Convert web tests to browser tests for quickedit module I realized there's no direct replacement for WTB::drupalPost().

This got me searching through core for 'x-www-form-urlencoded' in order to see if I was missing something.

It turns out a number of tests make direct calls with curl, which is what WTB::drupalPost() does.

For instance here's a WTB: Drupal\node\Tests\Views\NodeContextualLinksTest::renderContextualLinks().

And here's a BTB: Drupal\Tests\history\Functional\HistoryTest::getNodeReadTimestamps().

There are many outlier WTB tests awaiting conversion to BTB, and it seems that they're outliers because there's no direct conversion for drupalPost(). And there are also many tests which are rolling their own curlExec() in order to accomplish this goal.

Proposed resolution

Make sure all tests have a consistent way to use the Mink client's guzzle client. See HistoryTest::getHttpClient() for an example of how this might work. http://cgit.drupalcode.org/drupal/tree/core/modules/history/tests/src/Fu...

Then tests can perform POST or other requests as needed using the client.

Remaining tasks

Add a change record about drupalPost(), drupalPostAjaxForm(), etc. conversions.

Follow-up to make existing tests consistent in their use of getHttpClient().

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#43 2911915-18.patch6.01 KBAnonymous (not verified)
#40 interdiff-36-40.txt13.12 KBAnonymous (not verified)
#40 2911915-40.patch22.14 KBAnonymous (not verified)
#37 interdiff-29-37.txt1.96 KBAnonymous (not verified)
#37 2911915-37.patch11.13 KBAnonymous (not verified)
#36 2911915-34-36.txt708 bytesAnonymous (not verified)
#36 2911915-36.patch13.59 KBAnonymous (not verified)
#34 interdiff-18-34.txt8.04 KBAnonymous (not verified)
#34 interdiff-29-34.txt8.33 KBAnonymous (not verified)
#34 2911915-34.patch13.59 KBAnonymous (not verified)
#27 interdiff-18-27.txt4.97 KBAnonymous (not verified)
#27 2911915-27.patch10.52 KBAnonymous (not verified)
#18 2911915-18.patch6.01 KBWim Leers
#18 interdiff.txt919 bytesWim Leers
#13 interdiff.txt3.64 KBMile23
#13 2911915_13.patch5 KBMile23
#11 2911915_11.patch3.28 KBMile23
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mile23 created an issue. See original summary.

Mile23’s picture

Issue summary: View changes
dawehner’s picture

I agree with @Mile23 we should have a trait, called, ApiTestTrait maybe which contains this method for now ... and maybe some helpful assertions or so later.

dawehner’s picture

Issue tags: +phpunit initiative
nlisgo’s picture

Assigned: Unassigned » nlisgo
Issue tags: +Vienna2017
nlisgo’s picture

Assigned: nlisgo » Unassigned

I'm not convinced there is value in having a getHttpClient method. In the example HistoryTest::getHttpClient(), the cookies are being prepared so that when a request is made with the client we can pass the cookies as an option in the request. We would need to call getHttpClient each time we switched users.

The expectation is that drupalPost will perform it's operations as the user who has authenticated with BrowserTestBase::drupalLogin. Therefore, it would be cleaner to just prepare the cookies within drupalPost.

I don't know what getHttpClient would do other than return \Drupal::httpClient(). I have no idea what value return $this->getSession()->getDriver()->getClient()->getClient(); adds above \Drupal::httpClient().

If all getHttpClient does is return \Drupal::httpClient() is it worth creating a Trait for?

Mile23’s picture

My assumption is that we want a consistent way to get whichever HTTP client we want people to use. Some existing tests resolve \Drupal::httpClient() and some use the mink client, so which should it be?

I think HistoryTest::getHttpClient() would be the most useful thing to copy, because it populates the session as if we're logged in and so forth. This should really be a method on BrowserTestBase. If someone knows why they need to not use that client they can then get \Drupal::httpClient() or just make a new Guzzle.

Then if we want to support drupalPost() that could be on a legacy trait of some type or another.

dawehner’s picture

The main problem of using \Drupal::httpClient() is that XDEBUG doesn't work with them, unless you copy the cookies over. Given that having a helper method is totally worth doing.

Mile23’s picture

Title: No direct conversion for WTB::drupalPost(), add trait » Add getHttpClient() to BrowserTestBase
Issue summary: View changes

OK rescoping, because if there's a consistent way to get http client, then performing a POST isn't that tricky.

dawehner’s picture

I agree with the scope change.

We basically have drupalGet/drupalPostForm/click ... which controls a browser and on the other side we have a pure HTTP client, which does GET/POST/DELETE/... requests.

Mile23’s picture

Status: Active » Needs review
FileSize
3.28 KB

A patch.

mpdonadio’s picture

  1. +++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
    @@ -633,4 +634,16 @@ public function testCheckForMetaRefresh() {
    +      $this->fail('getHttpClient() threw: ' . $e->getMessage());
    

    Think we need the class name in this message?

  2. +++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
    @@ -633,4 +634,16 @@ public function testCheckForMetaRefresh() {
    +  }
     }
    

    Nit. Need newline a class end.

  3. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
    @@ -547,6 +547,33 @@ public function getSession($name = NULL) {
    +    throw new \RuntimeException ('The Mink client type ' . get_class($mink_driver) . ' does not support getHttpClient().');
    +  }
    

    Is this work having coverage for?

Those are fairly minor. I like the patch. I like the comments.

Leaving at NR so other people see and can chime in, but the IS mention a few items to do before this can RTBC.

Mile23’s picture

Turned the test into a unit test. With lots, and lots, of mocking. :-) But it follows the exception code path.

Lendude’s picture

Love the test.

+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -626,11 +653,13 @@ protected function buildUrl($path, array $options = []) {
-   *   testing REST APIs it is recommended to directly use an HTTP client such
-   *   as Guzzle instead.
+   *   testing REST APIs it is recommended to obtain a separate HTTP client
+   *   using getHttpClient() and performing requests that way.

Does this mean we need a change record for this since we basically recommend a different way of doing this now? Not sure.

    Nittiest of nitpicks, feel free to ignore:

  1. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
    @@ -547,6 +547,33 @@ public function getSession($name = NULL) {
    +   * @throws \RuntimeException
    +   *   If the Mink driver does not support a Guzzle HTTP client, throw an
    +   *   exception.
    

    "Thrown when the Mink driver does not support Guzzle HTTP client."
    The wording is inconsistent in core but that seems the go-to wording.

  2. +++ b/core/tests/Drupal/Tests/Core/Test/BrowserTestBaseTest.php
    @@ -0,0 +1,82 @@
    +  protected function mockBrowserTestBaseWithDriver($driver) {
    

    all methods need a docblock... :)

Mile23’s picture

Issue tags: +Needs change record

Does this mean we need a change record for this since we basically recommend a different way of doing this now? Not sure.

Well we don't have a recommended way of doing this currently, so we're not changing behavior. :-)

Tho a change record would probably be nice.

"Thrown when the Mink driver does not support Guzzle HTTP client."
The wording is inconsistent in core but that seems the go-to wording.

getHttpClient() always returns a guzzle object, or it throws the exception. I'm not sure all Mink drivers use Guzzle, or use it in a way that allows us to get an arbitrary HTTP request through it. For instance GastonJS uses Guzzle to send requests to PhantomJS, but not to send requests to the HTTP server itself.

Lendude’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record
larowlan’s picture

Updating issue credits and queuing for 8.5

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +API-First Initiative, +DX (Developer Experience)
FileSize
919 bytes
6.01 KB

+1

But I think we should consider updating ResourceTestBase to use this instead of it having similar code. It's the first (and perhaps only) case of this in core, and therefore helps justify this patch.

Wim Leers’s picture

Also, quoting @dawehner in #8:

The main problem of using \Drupal::httpClient() is that XDEBUG doesn't work with them, unless you copy the cookies over. Given that having a helper method is totally worth doing.

… AFAICT this patch is not yet doing that, even though that's the #1 justification?

I should be able to remove \Drupal\Tests\rest\Functional\ResourceTestBase::decorateWithXdebugCookie(), which was added in #2851746: Support xdebug header in ResourceTestBase and move htttpClient property to right place.

dawehner’s picture

But I think we should consider updating ResourceTestBase to use this instead of it having similar code. It's the first (and perhaps only) case of this in core, and therefore helps justify this patch.

There are a LOT of testcases to be updated ... I was hoping we don't start this here.

+++ b/core/modules/rest/tests/src/Functional/ResourceTestBase.php
@@ -335,7 +335,7 @@ protected function request($method, Url $url, array $request_options) {
     $request_options = $this->decorateWithXdebugCookie($request_options);
-    $client = $this->getSession()->getDriver()->getClient()->getClient();
+    $client = $this->getHttpClient();

I'm quite sure you no longer have to decorate with xdebug cookies ...

Wim Leers’s picture

There are a LOT of testcases to be updated ... I was hoping we don't start this here.

Oh, I'm sorry, I didn't realize that. AFAICT there's less than 15 though? Still, would be good to update at least one clear example, I think?

I'm quite sure you no longer have to decorate with xdebug cookies ...

Ah? Then why does \Drupal\Tests\BrowserTestBase still have use XdebugRequestTrait;?

dawehner’s picture

Oh, I blindly assumed things ... Is there a reason we don't add the xdebug cookies here? This would make it more in parallel to for example the previous drupalPostForm

Wim Leers’s picture

I’m not sure what you’re saying/asking exactly. Can you clarify?

Anonymous’s picture

#23: Perhaps, in #22 @dawehner is wondering can we also add xdebug cookies as global to BrowserTestBase (maybe in prepareRequest()). Like example, he pointing on WebTestBase::curlExec(), where these cookies are added here:

    foreach ($this->extractCookiesFromRequest(\Drupal::request()) as $cookie_name => $values) {
      foreach ($values as $value) {
        $cookies[] = $cookie_name . '=' . $value;
      }
    }

In addition to the main benefits for debugging, this will provide additional compatibility between WTB and BTB.

dawehner’s picture

Sorry for not commenting in a while :(

@vaplas
Yeah, that's exactly what I meant. It would be nice if these HTTP requests would have the xdebug cookies, so people have an easy time to debug those requests.

Wim Leers’s picture

Status: Needs review » Needs work

For #22/#24/#25.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
10.52 KB
4.97 KB

Added XDEBUG_SESSION for debugging + test.

Status: Needs review » Needs work

The last submitted patch, 27: 2911915-27.patch, failed testing. View results

Anonymous’s picture

Oh, fixed. + test-only with only new test.

Anonymous’s picture

Status: Needs work » Needs review

The last submitted patch, 29: 2911915-29-test-only.patch, failed testing. View results

dawehner’s picture

  1. +++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
    @@ -639,4 +639,41 @@ public function testGetDefaultDriveInstance() {
    +   * @dataProvider providerTestGetHttpClientXdebugOn
    +   * @dataProvider providerTestGetHttpClientXdebugOff
    

    Oh, I had no idea one can provide multiple data providers, nice!. I love how this encapsulates the putenv bit!

  2. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
    @@ -296,11 +297,24 @@ protected function initMink() {
    +      // peer verification so that testing under HTTPS always works. Include
    +      // cookies with XDEBUG_SESSION in the debugging.
    +      $request = \Drupal::request();
    +      $cookies_params = $this->extractCookiesFromRequest($request);
    +      if (isset($cookies_params['XDEBUG_SESSION'])) {
    +        $cookies = new CookieJar(TRUE, [
    +          [
    +            'Name' => 'XDEBUG_SESSION',
    +            'Value' => current($cookies_params['XDEBUG_SESSION']),
    +            'Domain' => $request->getHost(),
    +          ],
    

    I'm curious, don't we need a way to pass cookies from one request to the next one? It feels like creating a new cookie jar prevents this? Sorry, this might be an utter stupid question.

Anonymous’s picture

Thanks, @dawehner!

#32.2 is very good question! But unfortunately, I did not find a way to avoid overlapping. Here is my research:

1. GuzzleHttp\Client hasn't public method for set default config after created. Therefore, the only chance to do this while creating the instance. How to avoid the loss of values when combining with the custom parameters like

$custom_parameters['cookies'] = $cookies;
$client->request($method, $url, $custom_parameters);

Cookies can store many values. Maybe if we create a default value with a large index, it will remain when we merge:

$default_cookies = new CookieJar(TRUE, [
      [stub_cookie],
      [stub_cookie],
...
      [xdeubg], // index 99
      ],
]);

Not. It is not works, because GuzzleHttp\Client::prepareDefaults() combines this with custom values like:

        // Shallow merge defaults underneath options.
        $result = $options + $defaults;

So, we lost default cookies key, if we have custom cookies key :(


2. What else? Can be used default 'headers', like
'headers' => ['Cookie' => 'XDEBUG_SESSION=value;'],
At first it seemed to me that there are chances here, since the processing of headers in GuzzleHttp\Client::prepareDefaults() is more intricate.
Example, it is copy default headers to '_conditional' and than combine with result headers. But I miscalculated.
headers['Cookie'] is not decomposed into parts, but it is also completely replaced.

In addition, 'cookies' key have more prior, than headers['Cookie'].

So, we lost default headers['Cookie'] key too :(


3. Then what to do?
More radical solutions:
  1. Сreating own class DrupalClient extends GuzzleHttp\Client, and return it in getHttpClient()
  2. Creating own methods in BTB, like drupalRequest(), drupalPost(), drupalGet(), and use them instead of $client->request(), $client->post(), $client->get().
  3. To do nothing, if someone wants custom cookies and xdebug, let him do it himself. For the rest developers it will be out of the box.
  4. The previous version, but plus additional methods to simplify the decoration task. For example, to steal ResourceTestBase::decorateWithXdebugCookie() to BrowserTestBase::decorateWitCookie() (as-is version) and to XdebugRequestTrait::decorateWithXdebugCookie() (stripped-down version, based on XDEBUG_SESSION from \Drupal::request()).
Anonymous’s picture

I was so intoxicated with the joy of successful tests, that I did not check some things. Now I suffered the damage for it. @vaplas----.

PHPUnit is really not supports multi @dataProvider, just ignore all except first provider (I definitely got it mixed up with something else). And this is only half the trouble.

Before starting the test, all dataProviders are processed (even if they are associated with another test). As result, if one dataProvider call putenv(); this will affect all tests in the class 😱

For this reason, the testing of the Xdebug mode moved into a separate class.

Also added XdebugRequestTrait::decorateWithXdebugCookieFromRequest(). Not sure about name.

By default, it works with headers['Cookie'] key, because it has a weaker priority, and will be replaced by a custom 'cookies' key.

If someone wants to work with 'xdebug' and own 'cookies':

// Combine options.
$request_options = $this->decorateWithXdebugCookieFromRequest($request_options, NULL, TRUE);
// Only Xdebug options. 
$request_options = $this->decorateWithXdebugCookieFromRequest();

Status: Needs review » Needs work

The last submitted patch, 34: 2911915-34.patch, failed testing. View results

Anonymous’s picture

Status: Needs work » Needs review
FileSize
13.59 KB
708 bytes
Anonymous’s picture

How did such a tasty #29 test turned into a tasteless #36 puttee?( Sorry again.

Multiple data providers supports only from PHPUnit 5.7 (pull request 2223).

While we can emulate the multiple data providers by calling one test from another.

We can also set putenv() in the setUp() instead of dataProvdier (because all dataProviders are processed before test start).

Done as a challenge. Nevertheless, perhaps we should continue #36.

Review:

+++ b/core/tests/Drupal/Tests/XdebugRequestTrait.php
@@ -45,4 +48,52 @@ protected function extractCookiesFromRequest(Request $request) {
+  protected function decorateWithXdebugCookieFromRequest(array $request_options = [], $request = NULL, $use_headers = TRUE) {
...
+    if ($xdebug_cookies) {
...
+          $request_options[RequestOptions::HEADERS]['Cookie'] .= '; ' . $cookies;
...
+      else {
...
+        $request_options[RequestOptions::COOKIES] = new CookieJar(TRUE, $cookies);

What if we decouple this method on two methods:

  1. decorate via cookies (jar cookies)
  2. decorate via headers['Cookie'] (string)

and add both decoration to initial client instance, like:

+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -296,12 +296,15 @@ protected function initMink() {
       $client = $this->container->get('http_client_factory')->fromOptions([
         'timeout' => NULL,
         'verify' => FALSE,
-      ]);
+      ]
+      + $this->decorateWithXdebugCookieJarFromRequest()
+      + $this->decorateWithXdebugHeadersCookieFromRequest()
+    );

+ add decoration method with all cookies (not only with XDEBUG)?

dawehner’s picture

@vaplas
At least for me the really important thin is that getHttpClient

add decoration method with all cookies (not only with XDEBUG)?

That sounds sensible, can you show how the end result would look like?

Status: Needs review » Needs work

The last submitted patch, 37: 2911915-37.patch, failed testing. View results

Anonymous’s picture

Sure. But my implementation is probably obese 😶

+++ b/core/tests/Drupal/Tests/Traits/DecorateCookiesTrait.php
@@ -0,0 +1,156 @@
+trait DecorateCookiesTrait {
...
+  protected function decorateRequestOptions(array $request_options = [], $force_regim = FALSE) {
...
+  protected function decorateHeadersCookie(array $request_options = [], $force_regim = FALSE) {
...
+  protected function decorateCookie(array $request_options = [], $force_regim = FALSE) {

Separate trait with decoration methods.
decorateRequestOptions() for the intellectual determination of the desired decorator (based on check existing keys).

It might be also useful converter between ['headers']['Cookie'] and ['cookies']?

+++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
@@ -639,4 +642,77 @@ public function testGetDefaultDriveInstance() {
+  public function testDecorationCookies($request_options, $is_force, $expected) {

Special test for these decorative methods.

+++ b/core/tests/Drupal/Tests/XdebugRequestTrait.php
@@ -55,19 +55,21 @@ protected function extractCookiesFromRequest(Request $request) {
-  protected function decorateWithXdebugCookieFromRequest(array $request_options = [], $request = NULL, $use_headers = TRUE) {
+  protected function decorateWithXdebugCookieFromRequest(array $request_options = [], $request = NULL) {

Remove $use_headers and now also determines the desired decorator based on the key.


I'm a little afraid to add extra weight to the issue. Maybe to move works about Xdebug and decoration-helpers to follow-up, and revert to #18?

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Anonymous’s picture

I suggest splitting this issue into several parts, and to solve each one as well as possible, without delaying other parts.

  • Part 1: Add getHttpClient() (current issue, just reupload #18);
  • Part 2: Use Xdebug in HTTP client by default (new issue with a piece of #36);
  • Part 3: Add API for Cookies/Headers options decoration (new issue with a piece of #40).
  • Part 4: add getMinkClient() (by analogy with getHttpClient(), for cases when the flexibility of Mink client is sufficient, because this makes it possible to work with pages as usual)
  • Part 5: Replace chains for get HTTP client on API. (like \Drupal::httpClient() and $this->getSession()->getDriver()->getClient()->getClient() to getHttpClient()/getMinkClient() where applicable).

I'd love to make these parts, after approval.

Anonymous’s picture

Completed points 1-3 (4-5 are not worth attention now):

So all the useful work after #18 patch was transferred to separate issues. So we can focus on solving the main task of this issue.

Reupload #18. It seems it already has everything we need!

Lendude’s picture

Status: Needs review » Reviewed & tested by the community

This still makes sense to me. Also, off loading the cookie stuff to follow ups makes sense. Little steps.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed f54efcf and pushed to 8.6.x. Thanks!
Committed 511ea6a and pushed to 8.5.x. Thanks!

Backported 8.5.x because even if a test method has a method called getHttpClient the only chance it would break is if it was public and there would be no need for that.

diff --git a/core/tests/Drupal/Tests/BrowserTestBase.php b/core/tests/Drupal/Tests/BrowserTestBase.php
index 22c450e0a1..290be1a5cf 100644
--- a/core/tests/Drupal/Tests/BrowserTestBase.php
+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -616,7 +616,7 @@ protected function getHttpClient() {
     if ($mink_driver instanceof GoutteDriver) {
       return $mink_driver->getClient()->getClient();
     }
-    throw new \RuntimeException ('The Mink client type ' . get_class($mink_driver) . ' does not support getHttpClient().');
+    throw new \RuntimeException('The Mink client type ' . get_class($mink_driver) . ' does not support getHttpClient().');
   }
 
   /**

Extra space crept in from somewhere - fixed on commit.

  • alexpott committed 511ea6a on 8.5.x
    Issue #2911915 by vaplas, Mile23, Wim Leers, dawehner, Lendude, nlisgo,...

  • alexpott committed f54efcf on 8.6.x
    Issue #2911915 by vaplas, Mile23, Wim Leers, dawehner, Lendude, nlisgo,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

quietone’s picture

publish change record