Convert functional tests in navigation module to kernel tests

Rebased, and cherry-picked the MR from #3582228: add $options and $headers parameters to HttpKernelUiHelperTrait::drupalGet() that we need here.

One test is failing.

I suspect the problem is here:

    $module_installer->install(['navigation']);

In a Kernel test, will this install the blocks & whatever as was happening in the Browser test?

Hmm so supposedly ModuleInstaller will do a full install.

But stepping in the debugger shows that after these lines:

    $module_installer->install(['navigation']);
    $this->installConfig(['navigation']);

the config table has these rows only:

core.extension
field.settings
file.settings
navigation.block_layout
navigation.settings
system.menu.content
system.menu.navigation-user-links

Ah, I was barking up the wrong tree.

navigation_test_block module has no default config -- it has an implementation of hook_navigation_defaults().

However I don't understand how this is meant to work. In navigation module we have this hook_modules_installed(), which isn't going to look at navigation_test_block module, only the modules which are getting installed:

  public function modulesInstalled(array $modules, bool $is_syncing): void {
    // Do not modify config during sync. Config should be already consolidated.
    if ($is_syncing) {
      return;
    }
    foreach ($modules as $module) {
      $blocks = $this->moduleHandler->invoke($module, 'navigation_defaults');

      if (!is_array($blocks)) {
        return;
      }

      foreach ($blocks as $block) {
        $this->configActionManager->applyAction('addNavigationBlock', 'navigation.block_layout', $block);
      }
    }
  }

And AFAICT, hook_navigation_defaults() isn't invoked anywhere else.

How was this working in a Functional test?

EDIT: well, the Functional test is failing for me too ???

Hi, which is the problematic test? If I'm not mistaken, it was not mentioned above.

I can assume it's NavigationDefaultBlockDefinitionTest from your last comment, but I'm not 100% sure. That test is working well locally for me in main branch.

Regarding the mentioned hook, it was introduced in #3491081: Allow other modules to include their own Navigation blocks during installation and it is being used by Dashboard module. I'm unaware of other usages, tbh.

I'm happy to help here if you could provide your specific needs.

The failing test is NavigationDefaultBlockDefinitionTest.php --filter=testNavigationDefaultBeforeNavigation. How is it working for you on the MR?

I don't understand this comment:

> // Refresh the mink session so we don't have a stale container.

The service container? How does clearing $mink affect that?

We should document that this needs to be done on HttpKernelUiHelperTrait.

> removing the unnecessary call to $this->installConfig(['navigation']);

I'm confused by this too! I thought that in kernel tests, we had to install config manually as it's not done by module installation?

The broken test is because of this:

    // Ensure that any changes to variables in the other thread are picked up.
    $this->refreshVariables();

What's the reason for doing this in each drupalGet()? We might potentially want to test caching.

If I remove the call to refreshVariables() inside drupalGet(), it turns out that a call to refreshVariables() is only needed in one place in the test, in testNavigationDefaultAfterNavigation():

    // After installing Navigation Test Block, both elements are present.
    $this->refreshVariables();
    $module_installer->install(['navigation_test_block']);

but doing it this way round, which to me seems more logical as it's after the installation of a new module, breaks the test:

    // After installing Navigation Test Block, both elements are present.
    $module_installer->install(['navigation_test_block']);
    $this->refreshVariables();

I don't know why, and this doesn't smell good.

Thanks for figuring that out -- it really helps!

The problem is in fact that inside $http_kernel, there is an instance of KernelPreHandle which itself has an instance of DrupalKernel, and that has an instance of the container.

And rather than $this->mink = NULL, we can call initMink(), which is more elegant, and it means I can document it on that method.

Should the addition of query string etc support to drupalGet() go in a preliminary helper issue, so that backporting the test utilities is easier? Oops, that is already done: #3582228: add $options and $headers parameters to HttpKernelUiHelperTrait::drupalGet().

Apart from that, this is ready I think. @mstrelan could you move the MR from draft state?

Shouldn't this be postponed on #3582228: add $options and $headers parameters to HttpKernelUiHelperTrait::drupalGet()?

Calling initMink() is needed because the Mink browser has an instance of the HTTP Kernel service, which itself contains (deep down) a reference to the container.

Therefore, if you do something such as installing a module which requires the container to be rebuilt, you need to re-do the setup of the Mink browser, so there is a fresh version of the container deep in there. I documented this on initMink() -- does it need further documentation in the list of restrictions on drupalGet() (or its trait, I forget which) maybe?

Calling refreshVariables() before each drupalGet() is something for tests to do. If we do it inside drupalGet() then caching won't ever work.

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.

The changes in the MR are consistent with other Functional-to-Kernel test conversions that I've done recently. They look good to me.

The use of initMink() is covered in the discussion above, particularly #23.

I think that adding the return typehint for refreshVariables() is probably out of scope since we're no longer using it in HttpKernelUiHelperTrait. But I don't think there's a reason to roll it back, so I'm not going to set the issue to Needs Work for that. I'm just noting it.

And just to be certain, I checked the GitLab CI Kernel test logs to make certain that the tests are running. Both of them are listed in the results.

This needs a rebase for the phpstan baseline, when I tried to do that locally, I then got:

Return type mixed of method Drupal\FunctionalTests\Installer\InstallerTestBase::refreshVariables() is not covariant with return type void of method Drupal\Tests\BrowserTestBase::refreshVariables().

Which suggests it might need more than that.

Ignore #27, I was on 11.x by mistake.

Committed/pushed to main, thanks!