Problem/Motivation

Drupal\Tests\help\Functional\HelpTopicsSyntax is slow and uses State storage a lot to store and retrieve data. Changes to State from #3496257: Race conditions in CacheCollector/State (again) cause the test to run especially slow, over 19 minutes on CI.

Steps to reproduce

Proposed resolution

Replace use of State storage with direct key value storage.

  • Convert from a functional test to a kernel test, since we can do drupalGet() from #3390193: Add a drupalGet() method to KernelTestBase
  • Inline HelpTestTwigExtension and HelpTestTwigNodeVisitor to the kernel test class so that class properties can be used as storage instead of database
  • Remove help_topics_twig_tester module

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#14 3586606-nr-bot_56x84y_g.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3586606

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

godotislate created an issue. See original summary.

godotislate opened merge request !15551

godotislate’s picture

godotislate
he/him
commented
Status: Active » Needs review

Replaced state with keyvalue and changed to a kernel test in MR 15551.

godotislate’s picture

godotislate
he/him
commented
Issue summary: View changes
godotislate’s picture

godotislate
he/him
commented

Working on this gave me the idea that similar to how kernel tests can define their own hook implementations, it'd probably be useful for them to be able to define their own routes as well. So I opened #3586832: Allow kernel test classes to define their own routes. MR is ready; it was pretty easy lift.

longwave made their first commit to this issue’s fork.

longwave opened merge request !15577

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

IMO we don't need the drupalGet() calls at all - HelpTopicTest::verifyHelp() already does a smoke test of the controller, I don't think we explicitly need to test each page load. If we don't do the refactoring here either then the change is easier to read too.

Locally MR!15577 runs in under 9 seconds:

Time: 00:08.581, Memory: 46.71 MB

OK (1 test, 11096 assertions)
godotislate’s picture

godotislate
he/him
commented

If we don't do the refactoring here either then the change is easier to read too.

There are still a lot of State storage writes, which is the main impetus for this issue, and caused the test run to take 20 minutes when #3496257: Race conditions in CacheCollector/State (again) was in.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Should we merge the two MRs together then, if you agree that we can drop the drupalGet() calls?

godotislate’s picture

godotislate
he/him
commented

Yeah, that sounds good.

longwave closed merge request !15577

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Merged the two together, also split out the nodeVisitorProcessingInformation array to individual variables, then realised we don't even need one of them (max chunk and chunk count were the same!) so refactored that away as well.

edit: @godotislate I just realised I overwrote your changes.. but I think my merge commit is effectively the same, just then I went a bit further with the refactoring?

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3586606-nr-bot_56x84y_g.txt91 bytes

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.

godotislate’s picture

godotislate
he/him
commented
Status: Needs work » Needs review

Fixed merge conflict.