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
Comment | File | Size | Author |
---|---|---|---|
#43 | 2911915-18.patch | 6.01 KB | Anonymous (not verified) |
#40 | interdiff-36-40.txt | 13.12 KB | Anonymous (not verified) |
#40 | 2911915-40.patch | 22.14 KB | Anonymous (not verified) |
#37 | interdiff-29-37.txt | 1.96 KB | Anonymous (not verified) |
#37 | 2911915-37.patch | 11.13 KB | Anonymous (not verified) |
Comments
Comment #2
Mile23Comment #3
dawehnerI 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.Comment #4
dawehnerComment #5
nlisgo CreditAttribution: nlisgo commentedComment #6
nlisgo CreditAttribution: nlisgo commentedI'm not convinced there is value in having a
getHttpClient
method. In the exampleHistoryTest::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 callgetHttpClient
each time we switched users.The expectation is that
drupalPost
will perform it's operations as the user who has authenticated withBrowserTestBase::drupalLogin
. Therefore, it would be cleaner to just prepare the cookies withindrupalPost
.I don't know what
getHttpClient
would do other than return\Drupal::httpClient()
. I have no idea what valuereturn $this->getSession()->getDriver()->getClient()->getClient();
adds above\Drupal::httpClient()
.If all
getHttpClient
does is return\Drupal::httpClient()
is it worth creating a Trait for?Comment #7
Mile23My 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 onBrowserTestBase
. 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.Comment #8
dawehnerThe 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.Comment #9
Mile23OK rescoping, because if there's a consistent way to get http client, then performing a POST isn't that tricky.
Comment #10
dawehnerI 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.
Comment #11
Mile23A patch.
Comment #12
mpdonadioThink we need the class name in this message?
Nit. Need newline a class end.
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.
Comment #13
Mile23Turned the test into a unit test. With lots, and lots, of mocking. :-) But it follows the exception code path.
Comment #14
LendudeLove the test.
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:
"Thrown when the Mink driver does not support Guzzle HTTP client."
The wording is inconsistent in core but that seems the go-to wording.
all methods need a docblock... :)
Comment #15
Mile23Well 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.
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.Comment #16
LendudeCR is here: https://www.drupal.org/node/2914601
Comment #17
larowlanUpdating issue credits and queuing for 8.5
Comment #18
Wim Leers+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.Comment #19
Wim LeersAlso, quoting @dawehner in #8:
… 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.Comment #20
dawehnerThere are a LOT of testcases to be updated ... I was hoping we don't start this here.
I'm quite sure you no longer have to decorate with xdebug cookies ...
Comment #21
Wim LeersOh, 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?
Ah? Then why does
\Drupal\Tests\BrowserTestBase
still haveuse XdebugRequestTrait;
?Comment #22
dawehnerOh, 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
Comment #23
Wim LeersI’m not sure what you’re saying/asking exactly. Can you clarify?
Comment #24
Anonymous (not verified) CreditAttribution: Anonymous commented#23: Perhaps, in #22 @dawehner is wondering can we also add xdebug cookies as global to BrowserTestBase (maybe in
prepareRequest()
). Like example, he pointing onWebTestBase::curlExec()
, where these cookies are added here:In addition to the main benefits for debugging, this will provide additional compatibility between WTB and BTB.
Comment #25
dawehnerSorry 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.
Comment #26
Wim LeersFor #22/#24/#25.
Comment #27
Anonymous (not verified) CreditAttribution: Anonymous commentedAdded XDEBUG_SESSION for debugging + test.
Comment #29
Anonymous (not verified) CreditAttribution: Anonymous commentedOh, fixed. + test-only with only new test.
Comment #30
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #32
dawehnerOh, I had no idea one can provide multiple data providers, nice!. I love how this encapsulates the putenv bit!
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.
Comment #33
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks, @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
Cookies can store many values. Maybe if we create a default value with a large index, it will remain when we merge:
Not. It is not works, because
GuzzleHttp\Client::prepareDefaults()
combines this with custom values like: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, thanheaders['Cookie']
.So, we lost default headers['Cookie'] key too :(
3. Then what to do?
More radical solutions:
class DrupalClient extends GuzzleHttp\Client
, and return it in getHttpClient()ResourceTestBase::decorateWithXdebugCookie()
toBrowserTestBase::decorateWitCookie()
(as-is version) and toXdebugRequestTrait::decorateWithXdebugCookie()
(stripped-down version, based on XDEBUG_SESSION from \Drupal::request()).Comment #34
Anonymous (not verified) CreditAttribution: Anonymous commentedI 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':
Comment #36
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #37
Anonymous (not verified) CreditAttribution: Anonymous commentedHow 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 thesetUp()
instead of dataProvdier (because all dataProviders are processed before test start).Done as a challenge. Nevertheless, perhaps we should continue #36.
Review:
What if we decouple this method on two methods:
and add both decoration to initial client instance, like:
+ add decoration method with all cookies (not only with XDEBUG)?
Comment #38
dawehner@vaplas
At least for me the really important thin is that
getHttpClient
That sounds sensible, can you show how the end result would look like?
Comment #40
Anonymous (not verified) CreditAttribution: Anonymous commentedSure. But my implementation is probably obese 😶
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']
?Special test for these decorative methods.
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?
Comment #42
Anonymous (not verified) CreditAttribution: Anonymous commentedI suggest splitting this issue into several parts, and to solve each one as well as possible, without delaying other parts.
\Drupal::httpClient()
and$this->getSession()->getDriver()->getClient()->getClient()
to getHttpClient()/getMinkClient() where applicable).I'd love to make these parts, after approval.
Comment #43
Anonymous (not verified) CreditAttribution: Anonymous commentedCompleted 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!
Comment #44
LendudeThis still makes sense to me. Also, off loading the cookie stuff to follow ups makes sense. Little steps.
Comment #45
alexpottCommitted 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.
Extra space crept in from somewhere - fixed on commit.
Comment #49
quietone CreditAttribution: quietone at PreviousNext commentedpublish change record