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.