Add drupalGet() to KernelTestBase

Made a very rough start on a branch.

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

Got it working!

The methods in BrowserHtmlDebugTrait aren't going to work for kernel tests -- they occasionally make assumptions about having Mink. So my approach has been to make a parallel KernelHtmlDebugTrait. It deviates very little, so as a follow-up we should look at merging them. Things to change include:

- make the $message param of htmlOutput() required, not optional. There's only one place that htmlOutput() is called without it
- changing how getHtmlOutputHeaders() works
- fixing how $this->htmlOutputTestId is obtained
- ... other things I may have forgotten about.

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

📌 Convert WebAssertTest to a Unit test Needs review 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.

samit.310@gmail.com → made their first commit to this issue’s fork.

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

Thanks
Samit K.

pooja_sharma → changed the visibility of the branch 3390193-add-drupalget-to-kerneltestbase to hidden.

pooja_sharma → changed the visibility of the branch 3390193-add-drupalget-to-kerneltestbase to active.

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
getenv('BROWSERTEST_OUTPUT_BASE_URL') ?: $GLOBALS['base_url']; (core/tests/Drupal/Tests/KernelHtmlDebugTrait.php:130)

@pooja_sharma What's the reason for the new MR?

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.

So I end up with raising new MR along with other changes mentioned in detail in prev comment.

Unless your work is a different approach to the existing MR, there's no need to create a new MR, it just adds confusion.

The MR can be added to even with it in draft state - I can change it when it's ready for review, but IIRC

pooja_sharma → changed the visibility of the branch 3390193-kerneltestbase-add-drupalget to hidden.

Got your concerns, to avoid confusion I have set visibility hidden of new MR. & already added detail description for test failures.

A good candidate for this change: 📌 Speed up ElementsTest Active .

@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.

Working on this.

Got the basics of this working!

Getting @larowlan's suggestion to work was actually easier than we thought when discussed in the DrupalCon BoF -- all it needed was a 1-line method override! :D Symfony already has a Mink driver that operates the Symfony HTTP kernel.

I found a simple functional test, NodeLinksTest, to convert as a trial, and very satisfyingly it's down from:

> Time: 00:35.962, Memory: 12.00 MB

to:

> Time: 00:09.708, Memory: 12.00 MB

However...

HTML debug output isn't working yet. This is because KTB sets $this->siteDirectory to be in a VFS, and initBrowserOutputFile() doesn't know how to work that. (Also, KTB *really confusingly* sets $this->siteDirectory *twice!???). Not sure how to fix -- do we just chage initBrowserOutputFile() to handle that sort of path too?

KTB has drupalGet(), but it doesn't yet have the ability to submit forms. I'd suggest leaving that to a follow-up, unless it turns out to be fairly simple.

As ever, writing kernel test really confronts you with all the assumptions that code makes. Turns out node module has a hidden dependency on field module, for instance!

We might want to as a follow-up allow installConfig() to only install specific config items, as for example in NodeLinksTest, I'm having to install field module just so installConfig() doesn't complain about not having a schema for field.storage.node.body, which we don't care about here.

Tests that need to check for things like cache headers won't work with this, because AFAICT that's added later on after the HTTP kernel has returned a response.

Tests that work with block output don't currently work with this, because there's no current theme. I've tried enabling stark and placing blocks, and that's not working. I switched to working on converting a test that didn't use blocks rather than go down that rabbithole. That's maybe something that can be figured out though -- maybe in a follow-up, unless it's simple.

joachim → changed the visibility of the branch 3390193-add-drupalget-to-kerneltestbase to hidden.

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.

Figured out the blocks issue.

Why doesn't the response from the HTTP kernel have cache headers? Are those added later in a normal request? And how -- is there a way we can get them here too for tests that need them?

I think we should add a helper to do this:

    $this->container->get('theme_installer')->install(['stark']);
    $this->container->get('theme.manager')->setActiveTheme(
      $this->container->get('theme.initialization')->initTheme('stark')
    );

as it's pretty tedious boilerplate.

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.

  Line   tests/Drupal/Tests/BrowserHtmlDebugTrait.php (in context of class      
         Drupal\KernelTests\KernelTestBase)                                     

That seems like a false negative to me -- I can't change the signatures of methods in a trait that other classes are using too.

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 📌 Add return types to UserCreationTrait Active for a proper fix so we don't need to keep adding to the baseline.

Thanks!!

I'm pondering whether to move the methods added to KernelTestBase to a trait, where I'd also add a helper method for setting the active theme.

Thoughts on doing this (and also on what to call the trait? I'm thinking KernelTestUiHelperTrait)

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.

> So for that it would be nice to have these in a trait, and not necessarily with the words "kernel test" in it.

We need something in the name to differentiate it from UiHelperTrait which is for browser tests. HttpKernelUiHelperTrait?

It's been a week with no objections to either the plan or the name, so I've moved the code for making requests to a trait.

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

Rerolled.

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.

My plan with this issue -- and I see it's not stated above, but I do remember discussing it with people, probably @longwave? -- was to add the functionality in this issue, and then in a follow-up do refactoring for shared code with browser tests.

I think I'd rather keep the two drupalGet()s separate as there are things in one that aren't relevant at all in the other -- for example, every request calling isTestUsingGuzzleClient() makes no sense in a kernel test.

However, if you think HttpKernelHelperTrait is a better name than HttpKernelUiHelperTrait, to match up with future refactoring, then let's change that. I'm not sure about 'Ui' in a trait about making requests either.

I'm not sure about the latest commit --

-  protected function drupalGet($path, array $options = [], array $headers = []): void {
+  protected function drupalGet($path, array $options = [], array $headers = []): string {

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

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...

Oh. I was looking at the actual return type... :/

Ok in that case setting back to needs review.

Something for a follow-on: I think I've worked out how to get page caching working with this.

I'm working on 🐛 Cacheability is not respected when not using a lazy builder Active and I can get the test to fail (as expected) if I do this:

- enable page_cache module
- kill CommandLineOrUnsafeMethod, which prevents the page cache from handling the request.

Rebased.

Looks like tests are red..

Tests are green and @alexpott has made the changes from his review, so this is back to NR.

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.

Rebased.

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

@dww The reason those methods don't have return types is to keep them aligned with the same methods in BrowserTestBase and UiHelperTrait with a view to refactoring them to common code in a follow-up.

Thanks for the review!

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

The @return void lines were added in this:

commit c69f5917e5c3366e2e35db5d36a679356135c27f
Author: Alex Pott
Date: Fri Dec 13 13:12:45 2024 +0000

Add return types to minimise the baseline

looks like they're there for a reason.

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?

> I was wonder about theme used, but you already mentioned that there is no theme.
> I don't clearly understand what does it mean?

I've reworded on the docs about that a bit. Does that help?

> You already mentioned limit - "block placing". Are there any other limits?

I had a read back of this issue, and there's page caching too. I've added docs about that. Hopefully that can be figured out in a follow-up, in which case we'll just remove the docs :)

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

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.

Cleaned up IS.

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

Added a CR.

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.

Resolved everything and rebased.

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

Resolved 2 comments. I think the 3rd can be left as is.

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.

Thanks for having a look!

AssertContentTrait wasn't change by this MR.

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 getTextContent().

> but I couldn't get the foobar_gorilla block to load in ::testPageEdit

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

> The first blocker I couldn't easily work around was that the default cache policy for DPC prevents caching if PHP_SAPI is cli

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?

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

    // If this test uses \Drupal\Tests\HttpKernelUiHelperTrait to make requests,
    // then get the content from the Mink browser.
    if (isset($this->mink)) {
      return $this->getSession()->getPage()->getText();
    }

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 📌 [PP-1] Convert functional tests in help module to kernel tests Postponed 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: [PP-1] 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.