Add a drupalGet() method to KernelTestBase

1. Huge +1. Love this idea.
2. It'd be great to be able to use this to check the output from a View page display, too.

Thanks!
-Derek

Linking the original issue

Rather than adding one method, writing a kernel based mink driver would be ideal

Copying from slack

https://github.com/symfony/symfony/blob/6.4/src/Symfony/Bundle/Framework...

Looks like symfony have a solution for this that uses sub processes

If we can get this to work we get drupalGet, submitForm, assertSession etc all for free

That was the original intent of step 3

#3454092: Convert WebAssertTest to a Unit test is doing a similar-ish thing - except converting a functional test to a unit test where only very specific markup/headers need to be tested.

Still think this is a great idea and there are probably dozens or hundreds of tests we could change from functional to kernel tests using this.

Rerolled with 11.x branch, only one phpunit https://git.drupalcode.org/issue/drupal-3390193/-/jobs/2285879 issue is remaining.

Thanks
Samit K.

I have created separate MR 9043, removed the unrelated changes, observed pipeline test failures.

Couple of things observed:
Drupal\KernelTests\Core\DrupalKernel\DrupalKernelTest run successfully on local, it seems more of issue on here.

Root cause of issue: Undefined global variable $base_url (core/tests/Drupal/Tests/KernelHtmlDebugTrait.php:130)

If I tried to add empty condition & set the default value conditionally test passed (pipeline can be observed)

I believe in below code of line, both of these variables , need to evaluate further whether these are set or not on giltlab as on local issue not replicate or either can provide suggestions that 'll be quite helpful to fix it further.
getenv('BROWSERTEST_OUTPUT_BASE_URL') ?: $GLOBALS['base_url']; (core/tests/Drupal/Tests/KernelHtmlDebugTrait.php:130)

As the existing MR in draft state & issue is not replicating on local it seems issue occurs due to existing MR is behind some commits.

I don't have access to move the existing MR from draft state, already showing error message on existing MR: Merge request must not be draft, due to which can't even rebase from gitlab or also facing issue on local to rebase it.

if you noticed even I have started working initially with existing MR, however due to restriction or not have access of existing MR, I end up with raising new MR along with other changes mentioned in detail (prev comment.), if existing MR in open state then it would be easier for me as well to proceed forward rather than creating new MR..

Got your concerns, to avoid confusion I have set visibility hidden of new MR. & already addressed query (#13) for test failures in prev comment.

A good candidate for this change: #3469573: Speed up ElementTest.

@catch so work should continue here correct?

Hello,

Fixed the failing test cases.

Thanks
Samit K.

Discussed this at Drupalcon Barcelona in @joachim's BOF.

In order to implement @larowlan's suggestion in #8 we researched what Symfony does and found that the framework bundle already provides a BrowserKit implementation that can talk to a Symfony Kernel: https://github.com/symfony/framework-bundle/blob/7.1/KernelBrowser.php

It is hopefully possible to copy this class into Drupal core, wrap it in a BrowserKitDriver, and instantiate a Mink session from KernelTestBase - and we should be able to do almost anything that BTB can do in terms of calling drupalGet() and WebAssert assertions from KTB.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Fixed phpcs and updated the baseline in this MR, but also opened #3480442: Add return types to UserCreationTrait for a proper fix so we don't need to keep adding to the baseline.

I'd be interested to see if we can use something like this in Drupal Test Traits, to perform a drupalGet without the browser. So for that it would be nice to have these in a trait, and not necessarily with the words "kernel test" in it. But having the theme stuff in that same trait would not be useful in that regard, but perhaps wouldn't hurt.

This looking great, we can mark #2469717: Step 3: Create a KernelWebTestBase base class/driver as a duplicate

When we start doing conversions, we should work on adding base-classes and traits first. The amount of additional setup required for e.g. a node module test is going to be a fair bit, so having that in all the tests would mean a lot of duplication otherwise.

Needs a reroll at present

I did some further tinkering with this. Wondering if we actually need our own ::drupalGet in the trait rather than just using the one from UiHelperTrait? I tried it out and it worked, but I had to implement ::prepareRequest as an empty function.

I wonder if drupalGet, drupalLogin and drupalLogout should be moved out of UiHelperTrait, they have nothing to do with the UI, whereas the rest of that trait has methods like click, clickLink, etc which make a lot more sense. Then we would rename this trait something like HttpKernelHelperTrait (without the word UI) and it would use whatever trait has drupalGet. Same applies for the assertSession method, we don't need to duplicate it if we can share the existing one.

Not sure if this helps or just complicate things.

Ok that all makes sense about refactoring and keeping them separate.

The browser test drupalGet() doesn't return anything, so why should the kernel test version?

It does return something:

   * @return string
   *   The retrieved HTML string, also available as $this->getRawContent()

See: https://git.drupalcode.org/project/drupal/-/blob/11.0.5/core/tests/Drupa...

Looks like tests are red..

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

This is starting to look great, thanks for working on it!

The latest pipeline has failures in NodeLinksTest now that it's converted to Kernel, among other test failures.

Also, initial look at the open threads on the MR and I think all of them are good suggestions that should be incorporated into this.

I'll try to do a more thorough review soon. This is really exciting!

Thanks again,
-Derek

Looks like a great idea, thanks.
I left my feedback on MR.

I was wonder about theme used, but you already mentioned that there is no theme.
I don't clearly understand what does it mean?
You already mentioned limit - "block placing". Are there any other limits?

Edited the issue summary. But there are several fields still blank. Also

Working-but-in-need-of-clean-up code is here:

I dont know what that means, if someone can please clarify. What can those links supply to this issue?

Thanks for update MR. New wording looks good, and thanks for provide information about caching.
I check MR again and have one more question.

I love this feature, and I am going to use it as soon it will be deployed. Thanks for your effort.

We are going to need a CR for this, i think.

Thank you @joachim. Great work on this issue!

Edited the CR, reworded in places: to emphasise the new possibilities the change provides!

I also did some minor edits (mostly just formatting) on the CR, and agree that looks really good.

Tried to resolve as many of the MR threads as I could.

There's still some valid feedback that hasn't been addressed.

Re: #60: Any future refactoring to consolidate these traits and methods will end up being more strictly typed. As new code, I believe our policy is to add always strict types, including return values. No code will be simultaneously using both traits -- a test is either Functional or Kernel but not both. I don't understand any benefit of leaving off the return types. There's no BC to preserve, since this is all brand new.

Thanks!
-Derek

p.s. Initial pass at saving credit:
@joachim, @pooja_sharma, @samit.310, @mstrelan, @alexpott, @mxr576 for code.
@oily, @nikolay shapovalov and myself for reviews.

I still need to review this properly, but overall it looks good.

Tagging for an 11.2.0 priority - doesn't need to be tied to a release as such because 95% of the usage will be in core tests, but if we get this in, we can start to work on conversions, and it'll be available to contrib earlier etc.

There's some comments in the MR from @mstrelan that need addressing - thanks for working on this

I tried to convert a few BrowserTestBase tests to kernel tests to try this out and have a couple observations.

1. \Drupal\Tests\help\Functional\HelpPageOrderTest
This calls $this->getTextContent() which in BrowserTestBase resolves to \Drupal\Tests\UiHelperTrait::getTextContent, but in KernelTestBase resolves to \Drupal\KernelTests\AssertContentTrait::getTextContent which depends on AssertContentTrait::setRawContent being called. Of course the test could be refactored to not call $this->getTextContent(), but I'm wondering if we should do something about that here, such as calling AssertContentTrait::setRawContent in drupalGet. Once I worked around it the Kernel test passed in 3 seconds as opposed to 12 seconds before.

2. \Drupal\Tests\block_content\Functional\BlockContentPageViewTest
This required lots of refactoring, which is as expected, but I couldn't get the foobar_gorilla block to load in ::testPageEdit. I was able to manually place the system_powered_by_block and get that to load, so I'm willing to concede that there's probably something I'm missing, rather than blocks not appearing because of the new drupalGet.

3. \Drupal\Tests\dynamic_page_cache\Functional\DynamicPageCacheIntegrationTest
This was probably a bit ambitious. The first blocker I couldn't easily work around was that the default cache policy for DPC prevents caching if PHP_SAPI is cli, which it is now that we're in a Kernel test. This might lead to other gotchas down the track if folks are expecting DPC to work when using drupalGet in Kernel tests.

That's as much as I've got time for today. Unfortunately I didn't find an existing test that could be converted cleanly, but nonetheless I'm quite excited about this.

I'm not sure whether to:

a. call AssertContentTrait::setRawContent in drupalGet()
b. change getTextContent() to use Mink, since that's now available? And then we can later on deprecate setRawContent().

I'm not sure either. I don't think we can deprecate setRawContent because I think some tests use it after directly rendering things not using drupalGet. Note that UiHelperTrait::drupalGet has this comment that I think is not true either, because setRawContent only happens in KernelTestBase.

   * @return string
   *   The retrieved HTML string, also available as $this->getRawContent()

@mstrelan could you try adding this to the top of AssertContentTrait::getTextContent()?

Yeah that worked. I think probably the simplest solution is to call $this->content = $out; from the new drupalGet, that also makes it work.

Could you push your work on that test somewhere so I can have a look at it?

Added patches for POC I did on block_content and help tests.

The docs on HttpKernelUiHelperTrait::drupalGet() say that page caching modules won't work. Should that list of warnings be moved from the method docs to the trait docs, so they're more visible?

I must admit I didn't look at the docs, I just expected it would work like BrowserTestBase. I don't think moving them would make a difference. I do think we could probably work around this though by either replacing that policy or modifying it to use a service that can be replaced or decorated to pretend it's not the CLI. But this is best suited to a follow up.

Opened #3517299: Convert functional tests in help module to kernel tests with a few more help tests I converted, see https://git.drupalcode.org/issue/drupal-3517299/-/merge_requests/1/diffs for the diff.

Added a comment in #3517299-4: Convert functional tests in help module to kernel tests that the only thing blocking multiple requests in the tests I've looked at is the missing preceding slash.

UiHelperTrait has a buildUrl method that ensures the path is absolute, I think we need to do the same thing here.

Some more conversions at #3519393: Convert functional tests in navigation module to kernel tests.

I was able to work around more issues with multiple drupalGet calls, this time with lazy builders. The issue was that we needed to reset the MemoryBackend cache after the first drupalGet. This is something we already do in UiHelperTest by calling $this->refreshVariables. Here is the commit in that issue in case we want to add it here.

But I ran in to another issue - installing modules doesn't reset the hook implementations. So in NavigationDefaultBlockDefinitionTest::testNavigationDefaultBeforeNavigation where it tries to install navigation_test_block first, and navigation later, the hook_page_top implementation in navigation module is never invoked. I'm not sure a way to solve that yet.

One thing to make sure we have here is support for sessions.

There are some core access checks (most notably the CSRF one) that don't run if there's no session, which would mean the result we get here is different to that with BrowserTestBase

@joachim As of sessions...
I think it's Drupal sessions, not mink sessions.
Which is about cookies.
One year ago (time flies!) you made a commit on this:
> 889f5a0a - Removed sessions and cookie stuff from initMink(). We don’t need this?
HTH!

I think session support could definitely be left for a follow-up, there are dozens/hundreds of tests we could shift without needing to be logged in.

Re #95: After a module install, the Drupal container is rebuilt and is actually a completely new object, so the test class property needs to be updated to reference the new container object instead of the old one, which is what ConfigOtherModuleTest::installModules() does.

#2066993: Use magic methods to sync container property to \Drupal::getContainer in functional tests introduced a magic getter to sync the container property to \Drupal::getContainer() in browser tests for this reason. I don't recall what, if any, discussion about kernel tests there was, but it's a long thread.

we'll have something that works

I think this is too general a statement for me to make, especially since I haven't looked at the use case here,
I'm only speaking to after you do any container rebuild in a kernel test, you have to be careful that the test class's container property, any other properties and local variables that are services or derived from services, such as entity storage handlers, need to be refreshed to reference the new container object and services from the new container.

I suspect a lot has changed with hooks and module installs since #87, I remember seeing some issues that I thought might have helped here.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I'm impressed with the performance improvements the HttpKernelBrowser based mink driver brings. Great work!

At the same time I'm surprised that sessions seem to work fine with the changes proposed here. My expectation was that making session work with the kernel browser would require replacement of various session components.

The MR removes the cli SAPI check from the session middleware. But there are still various cli SAPI checks in place in SessionManager. Those checks, e.g., prevent that session data is stored and loaded from the database.

I stepped through KernelTestHttpRequestTest::testSession() with a debugger in order to realize what happens here.

In functional tests, the response is generated in the test child site. The container and all its services are newly initialized in every request. The session subsystem relies on cookies in order to correlate session identifiers with session data in the database. This is exactly how request/response cycles work with HTTP user agents.

In kernel tests, the container instance is constructed once and is reused throughout the test. The response is generated in the test runner. This means that the same session instance is reused across requests. The MR as it currently works doesn't respect cookies on the request object and doesn't store/retrieve session data into/from the database.

One could argue that the current MR relies on state leaking from one request to the other in order to support sessions. I'm unsure whether this is the desired behavior.

I can imagine one situation where this breaks: Namely if something triggers a mid-test container rebuild. That could be acceptable if the goal of this issue is to speed up testing of simple cases.

Other things like SessionManager::regenerate() (used at login/logout) will be more or less no-ops. I'm honestly not sure about the consequences of that.

That could be acceptable if the goal of this issue is to speed up testing of simple cases.

I think that it probably is but we should try to define what simple is.

We have a lot of functional tests that set up a few things, hit a single page, then do some assertions. Those will be great candidates to convert to this.

There are probably also some tests, especially around access stuff, where we want an authenticated user to trigger certain cache contexts etc., but that again is probably a single request scenario so could come under the simple case, and the details of session storage etc. don't matter we just need a session at all.

Once we get into multiple requests with an authenticated user, then either that indicates a test that's doing setup in the UI (very common pattern for tests added 15 years ago) where instead it only needs the request to the 'final' page, or something that should be left as a functional test because it really wants to test as close to a real site. There are bound to be tests in-between these but we don't need to start with those.

This is the first time I see going with the Symfony kernel actually benefits Drupal. Took long enough but wow. This is amazing. Thanks for the hard work.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Cookie management of native PHP session is done in the engine (session.c). Looking at Symfony is not enough to comprehend whats happening under the hood.

No session management for a first step seems like a good idea, we can open a follow-up.

We have at least some tests that give editor/admin permissions to anonymous users to save creating an authenticated user, and that's also a possible workaround for some simpler tests of functionality that's behind permissions.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

This looks really encouraging to me. The amount of new test framework code is minimal, most of the MR diff is actual test changes.

One question - is node links test really added or should that be getting deleted from a functional test somewhere?

One more question - do you know where the phpstan baseline additions are coming from? If it's using different traits that makes sense, but I would have thought things would move around rather than being added as such.

Is it possible to add a void return type to HttpKernelUiHelperTrait::alter()? It would remove those new entries in the baseline.

Right, but you can add a return type to an implementing class method even if the interface doesn't have one.

The only issue is whether an existing subclass of that class also implements alter and does not have the void return type. But that seems really unlikely?

I've done alter().

But htmlOutput() and initBrowserOutputFile() come from BrowserHtmlDebugTrait, which isn't getting changed in this MR.

Yup, looks fine. I haven't looked in depth at the rest of the MR, so not sure how close this is otherwise. But that change specifically looks fine to me.

MR is green again, I made three changes:

1. Explictly used one of the traits in KernelTestBase so that phpstan would notice - it doesn't like traits imported via other traits apparently.

2. Added a couple of phpstan baseline entries for existing traits.

3. Had to rename an ::alter() method in the hook ordering kernel test, because it conflicts with the new ::alter() method on KernelTestBase. Only one example in core but maybe we should note this in the CR?

None of these are substantial changes so I'm going to go ahead and RTBC here.

Added some feedback on the MR, and also this note on the CR:

-    $this->drupalGet('node');
+    $this->drupalGet('/node');

This is not what the change record says:

The test trait \Drupal\Tests\HttpKernelUiHelperTrait provides methods drupalGet() and assertSession() which work the same way as drupalGet() in the test trait Drupal\Tests\UiHelperTrait for Browser tests.

Why do we need the leading slash adding?

UiHelperTrait::drupalGet() has explicit handling for forward slashes via ::buildUrl()

    // The URL generator service is not necessarily available yet; e.g., in
    // interactive installer tests.
    elseif (\Drupal::hasService('url_generator')) {
      // Strip $base_path, if existent.
      $length = strlen($base_path);
      if (substr($path, 0, $length) === $base_path) {
        $path = substr($path, $length);
      }
      // Additionally strip any forward slashes.
      if (strlen($path) > 1) {
        $path = ltrim($path, '/');
      }

I can't remember the details of why, probably dates back to simpletest, but functional tests are extremely forgiving due to this - partly to support drupalGet() of completely unrouted or even external URLs.

I think it's fine for kernel tests that only support routed paths to be stricter, so updating the docs/CR seems fine.

edit: talked to longwave and joachim in slack seconds after posting this and changed my mind to the other direction, probably easier to make it work both ways.

I added void return types to the two methods in the existing trait, I agree it's highly unlikely anyone will be overriding them, much less returning anything from them. This makes phpstan happy. Moving back to RTBC since that's a tiny change.

Did a final review of this, no further notes. Looking forward to converting the slower tests and using this elsewhere.

Committed and pushed to main, thanks!

Did not backport cleanly but we should land this in 11.4 as well, marking for backport. Did not publish the change record yet, let's do that when it lands in 11.x.

Opened #3582242: Convert functional tests with suitably simple HTTP requests to kernel tests for next steps!

Kernel test failure:
https://git.drupalcode.org/issue/drupal-3390193/-/pipelines/782982/test_...

Drupal\KernelTests\Core\Test\PhpUnitApiGetTestClassesTest::testEquality with data set "Testsuite: unit"
RuntimeException: The autoloader expected class "Drupal\Tests\search\Kernel\node\NodeSearchTest" to be defined in file "core/modules/search/tests/src/Kernel/node/NodeSearchTest.php". The file was found but the class was not in it, the class name or namespace probably has a typo.

core/modules/search/tests/src/Kernel/node/NodeSearchTest.php

is missing 'node' in the namespace declaration.

Test failures here were due to #3575605: Move tests of search to search module which somewhat bizarrely only appear to show up on 11.x despite the same issues being in both 11.x and main.

Back to green here again.

Committed and pushed to 11.x, thanks!

Also published the change record.

Sure, done.

+1 to backport, FWIW. I'd love to be able to do this in more of my contrib projects, too, but it'd be a PITA to only be able to change the tests for D11. Basically everything I maintain still supports D10, too.

The release managers are considering a backport, but since there won't be 10.7.x, this would have to go in a 10.6.x patch release, in which case it'd probably also need to be backported to 11.3.x for a patch release there as well.

Also, would backporting this issue alone be sufficient, without any of the follow ups?

Thanks for considering it. If we backported, I think we'd want some of the follow-ups, too. At least to flesh out the API surface of the trait. In particular:
#3582769: Add clickLink() to HttpKernelUiHelperTrait
And assuming they land in the near future:
#3582228: add $options and $headers parameters to HttpKernelUiHelperTrait::drupalGet()
#3582985: Add content from HttpKernelUiHelperTrait::drupalGet() to $this->content for use by AssertContentTrait

I'm less interested in backporting the conversions, but I think backporting the "full" trait would be great (if we can).

Thanks again!
-Derek

Discussed with @longwave and @catch, and what we're leaning toward is backporting to 11.3/10.6 all the relevant issues together, once they're complete. Per #159, that's currently:
#3390193: Add a drupalGet() method to KernelTestBase (this issue)
#3582769: Add clickLink() to HttpKernelUiHelperTrait
#3582228: add $options and $headers parameters to HttpKernelUiHelperTrait::drupalGet()
#3582985: Add content from HttpKernelUiHelperTrait::drupalGet() to $this->content for use by AssertContentTrait

We can reassess if it turns out other issues need to be included.

Moving this and #3582769: Add clickLink() to HttpKernelUiHelperTrait to "Patch to be ported" so that they'll be easier to find later.

Cool, sounds great. Thanks!

Would it be easier to merge more of the follow-ups into a single issue / MR?

@dww that would make the backport easier, not necessarily the initial commits. But we could do an omnibus backport MR somewhere definitely so we're sure all the cherry picks are correct etc.

Should this be backported before #3582228: add $options and $headers parameters to HttpKernelUiHelperTrait::drupalGet()?

See #160. 4 issues are going in together, probably with a combined MR. #3582985: Add content from HttpKernelUiHelperTrait::drupalGet() to $this->content for use by AssertContentTrait is still outstanding.

Can we close #3582228: add $options and $headers parameters to HttpKernelUiHelperTrait::drupalGet() then? It's committed but also marked backport.

I'm not sure I understand? We're leaving them patch to be ported for 11.3.x/10.6.x.

Sorry for the delay I meant if we are backporting

#3390193: Add a drupalGet() method to KernelTestBase (this issue)
#3582769: Add clickLink() to HttpKernelUiHelperTrait
#3582228: add $options and $headers parameters to HttpKernelUiHelperTrait::drupalGet()
#3582985: Add content from HttpKernelUiHelperTrait::drupalGet() to $this->content if it exists

As 1 MR can we close the ones that are committed and just leave this one open?

I'm not completely sure we're doing 1 MR, though @catch mentioned the idea and it probably sounds good. I think we can leave all the issues open to keep track of them, and they won't really be in 10.6 until we commit them. Since they'll be in the next patch release, I think we should keep the individual issue statuses aligned.

@dww or @joachim: Were there any others issues that should go in for this effort outside the 4 previously mentioned (including this one)?

Is #3589626: clickLink in kernel tests erroneously prefixes some paths with / needed for backporting the correct functionality to 10.6? Are there any other bug issues that need considering?