Convert functional tests in help module to kernel tests

Thanks for doing a trial run like this! Very helpful and encouraging that the parent issue is on the right track.

If a branch off an issue branch confuses the tooling so much, probably not the end of the world to have an initial commit of #3390193: Add a drupalGet() method to KernelTestBase in here, no? The changes tab will be bloated, but it should be fairly simple to review changes via each proceeding commit...

Anyway, thanks again!

> and it turns out the only issue is that the paths weren't prefixed with a preceding slash, so the path was appended to the path of the previous request resulting in 404s

Can you explain a bit more about where this path appending thing happens? Is it something we can maybe fix?
Though OTOH, since the Drupal convention is now that paths should have the initial slash, it's probably a good thing to add that consistently.

I've had a look at your patch on the other issue for converting BlockContentPageViewTest. I'm honestly confused by what this failing test is doing anyway:

  public function testPageEdit(): void {
    $block = $this->createBlockContent();

    // Attempt to view the block.
    $this->drupalGet('block-content/' . $block->id());

    // Ensure user was able to view the block.
    $this->assertSession()->statusCodeEquals(200);
    $this->drupalGet('/block-content-test');
    $this->assertSession()->pageTextContains('This block is broken or missing. You may be missing content or you might need to install the original module.');
  }

Why would the block report as being broken? What's causing it to be broken? This test needs to explain what it is doing!

Rebased.

I had to remove history module from being installed in one test, but it made no difference to the test at all!?!?

HelpTopicTest isn't working, and the error is to do with how the Help module or Twig is outputting inline links.

The problem is

        $options['absolute'] = TRUE;

in HelpTwigExtension.

Filed #3582382: absolute links don't work in Kernel tests.

We could skip HelpTopicTest for now to get this in.

Trying to see if #3582386: remove absolute from HelpTwigExtension links works.

I don't understand why we need to add this to drupalGet():

$this->content = $out

UiHelperTrait::drupalGet() doesn't have it.

We probably should switch from using AssertContentTrait methods to using Mink methods as part of the conversion.

Re: #17 and #18: While I appreciate the desire for elegance and simplicity in the new trait, I'd strongly vote in favor of doing whatever we can to minimize the changes necessary to convert something from Functional to Kernel. If we have to update a ton of assertions, it's going to massively slow down conversions, bog down reviewers, require more cognitive load, etc. That 1 line (and minor waste of RAM while the test is running, if I understand correctly) seems like it'll save a ton of effort. So IMHO, I'd sacrifice a little elegance for easier conversions.

Thanks!
-Derek

I'm not totally against adding $this->content = $out but aren't PHPStan/PHPCS going to complain about a trait setting a property it doesn't have?

#3582580: add checkForMetaRefresh() method to HttpKernelUiHelperTrait is adding the setting of $this->content.

Adding clickLink in #3582769: Add clickLink() to HttpKernelUiHelperTrait, changing this MR accordingly.

We should add these to the main trait in a separate issue:

+  /**
+   * Overrides AssertContentTrait for use in Kernel tests.
+   *
+   * @see \Drupal\KernelTests\AssertContentTrait::xpath()
+   * @see \Drupal\Tests\BrowserTestBase::xpath()
+   */
+  protected function xpath($xpath, array $arguments = []): array {
+    $xpath = $this->assertSession()->buildXPathQuery($xpath, $arguments);
+    return $this->getSession()->getPage()->findAll('xpath', $xpath);
+  }
+
+  /**
+   * Get the current URL from the browser.
+   *
+   * @see \Drupal\Tests\UiHelperTrait::getUrl
+   */
+  protected function getUrl(): string {
+    return $this->getSession()->getCurrentUrl();

Rebased, and updated for changes in #3582985: Add content from HttpKernelUiHelperTrait::drupalGet() to $this->content for use by AssertContentTrait.

Re #24: I opened #3583594: Add getUrl() and (renamed) xpath() helpers to HttpKernelUiHelperTrait but the pipeline in there fails all Kernel tests with a conflicting xpath() method in both traits. Seems #3582985: Add content from HttpKernelUiHelperTrait::drupalGet() to $this->content for use by AssertContentTrait is the better solution, then we don't need a separate copy of xpath(). Then that would leave #3583594 to only add the getUrl() method. As I wrote over there, maybe we should merge all these follow-ups to be easier to manage, coordinate, and hopefully backport?

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.

Rebased and fixed baseline issue.

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.

Should the MR still be in draft?

Actually should this be postponed on #3191278: Use plugin id to make help section headers HTML IDs ?

If that gets in first, it's going to be a nightmare to rebase this MR, because the files are moved.

So the issue in #33 is RTBC and more then likely will land first. Vs making it a competition lets just postpone this as I feel that one is slightly more important, not to down play these conversions but the tests are there so nothing is lost. If that makes sense.

@smustgrave please don't postpone - we don't need to - resolve git conflicts is part of working together and are not horrible.

$ git merge main                                                                                                 
Auto-merging core/modules/help/tests/src/Kernel/HelpTest.php
Auto-merging core/modules/help/tests/src/Kernel/HelpTopicTest.php
Merge made by the 'ort' strategy.
 core/modules/help/src/Controller/HelpController.php                                | 1 +
 core/modules/help/src/Hook/HelpHooks.php                                           | 5 ++++-
 core/modules/help/templates/help-section.html.twig                                 | 3 ++-
 core/modules/help/tests/src/Kernel/HelpTest.php                                    | 4 ++--
 core/modules/help/tests/src/Kernel/HelpTopicTest.php                               | 2 +-
 core/profiles/demo_umami/themes/umami/templates/classy/misc/help-section.html.twig | 3 ++-
 core/themes/claro/templates/classy/misc/help-section.html.twig                     | 3 ++-
 core/themes/stable9/templates/admin/help-section.html.twig                         | 3 ++-
 8 files changed, 16 insertions(+), 8 deletions(-)

And git correct merged the assertions. - I did not even need to do anything.

noted

Only functional test that remains are upgrade tests and HelpTopicTranslationTest. I'm assuming HelpTopicTranslationTest has to remain as it does does a drupalLogin(). Rest looks good.

I think we need to decide what to do about the xpath etc. methods in #28 - go ahead here then update the test once one of those issues is in, or wait for one of those to land. I don't have a strong preference as long as we end up in the same place, but moving back to needs review for that.

@catch you mean #3583594: Add getUrl() and (renamed) xpath() helpers to HttpKernelUiHelperTrait that is RTBC maybe it could be committed and used here?

Now that #3583594: Add getUrl() and (renamed) xpath() helpers to HttpKernelUiHelperTrait I'm assuming catch you wanted to see that used here somewhere?

This one was postponed once (all be it incorrectly by me) so let’s try and land it.

Committed 2129f98 and pushed to main. Thanks!
Committed 3c5fc5f and pushed to 11.x. Thanks!

I recreated the baseline on 11.x in order to merge this there.

Actually the new tests are going to fail on 11.x so reverted.

Backported to 11.x now that #3583594: Add getUrl() and (renamed) xpath() helpers to HttpKernelUiHelperTrait got backported.