Add checks for syntax and display of help topic Twig template files

We have a few more syntax checks to add to this issue.

Adding note about not checking that only the currently-defined meta tags are there.

It appears that Coder does not work on Twig files, only PHP.

So, I'm moving this back to Drupal Core to figure out how to check syntax of Twig files, and updating issue summary.

Since we are doing this in Core, let's expand the scope slightly to include testing whether the topics can be displayed.

I don't think it's going to be possible to have a test that works for contrib, as far as I can tell. Updating issue summary.

There's a patch on #3037228: Add more test coverage to Help Topics that should probably be here instead.
https://www.drupal.org/files/issues/2019-07-08/core-add-more-test-covera...
Please credit scott_euser on this issue.
[adding this to the summary]

I am going to start working on this today and see what I can do...

OK, here we go! I think I managed to test everything in the issue summary that could be done without actually rendering the topic with Twig, which I am not sure will work well due to the variable stuff (routes may not be defined). As a note, I had to filter out test modules and themes in order for the tests to pass, which tells me that they are good tests (we intentionally have some bad topics in the test files under help_topics/tests/modules and help_topics/tests/themes).

Oh wait... OK I made one interdiff and then realized I could probably test one more thing. So, the first interdiff is from Scott's patch from the other issue, and the 2nd is the additional changes I made.

Anyway, let's make sure the test bot agrees, but this test passes locally with 164 assertions.

Oh, one other note: I did have to edit one topic file, because it failed the test -- it listed a topic as "related" that was the wrong ID.

This is some nice work!

1. +++ b/core/modules/help_topics/tests/src/Kernel/HelpTopicCoreTopicsTest.php
   @@ -0,0 +1,178 @@
   +    foreach (['module', 'theme'] as $type) {
   

   do we need to consider profiles here too?

2. +++ b/core/modules/help_topics/tests/src/Kernel/HelpTopicCoreTopicsTest.php
   @@ -0,0 +1,178 @@
   +    // Figure out which topics are top-level.
   +    foreach ($this->definitions as $id => $definition) {
   

   could use array_filter here?

3. +++ b/core/modules/help_topics/tests/src/Kernel/HelpTopicCoreTopicsTest.php
   @@ -0,0 +1,178 @@
   +      $this->verifyTopic($id);
   

   could pass $top_level_ids as a variable and avoid $this->topLevelIds, would be more portable if ever needed to move verifyTopic to a trait

4. +++ b/core/modules/help_topics/tests/src/Kernel/HelpTopicCoreTopicsTest.php
   @@ -0,0 +1,178 @@
   +    vfsStream::setup('root');
   +    vfsStream::create([
   +      'modules' => [
   +        'test' => [
   +          'help_topics' => [
   +            // An invalid topic missing required details.
   +            'test.topic.html.twig' => '',
   +          ],
   +        ],
   +      ],
   +    ]);
   +    $fail_directories['expected_failure'] = vfsStream::url('root/modules/test/help_topics');
   

   nice

5. +++ b/core/modules/help_topics/tests/src/Kernel/HelpTopicCoreTopicsTest.php
   @@ -0,0 +1,178 @@
   +    if (!in_array($id, $this->topLevelIds)) {
   ...
   +          $ok = $ok || in_array($related_id, $this->topLevelIds);
   

   we should use the third argument here because the values are expected to be strings

6. +++ b/core/modules/help_topics/tests/src/Kernel/HelpTopicCoreTopicsTest.php
   @@ -0,0 +1,178 @@
   +      else {
   

   no need for an else after a $this->fail

7. +++ b/core/modules/help_topics/tests/src/Kernel/HelpTopicCoreTopicsTest.php
   @@ -0,0 +1,178 @@
   +        $this->assertTrue($ok, 'Topic ' . $id . ' is either top-level or related to a top-level topic');
   

   we could flip this and $this->fail anytime the item was missing, instead of keeping track over the loop, i.e call $this->fail in the loop as soon as things go bad

8. +++ b/core/modules/help_topics/tests/src/Kernel/HelpTopicCoreTopicsTest.php
   @@ -0,0 +1,178 @@
   +    $this->assertFalse(empty($body), 'Topic ' . $id . ' has a non-empty Twig file');
   ...
   +    $this->assertFalse(empty($body), 'Topic ' . $id . ' Twig file contains some text outside of meta tags');
   

   could use assertNotEmpty?

9. +++ b/core/modules/help_topics/tests/src/Kernel/HelpTopicCoreTopicsTest.php
   @@ -0,0 +1,178 @@
   +    // Remove all the translated text, whitespace, and HTML tags, and there
   +    // should be nothing left.
   

   I'm not sure what the point of this is? Are you asserting there's nothing that is outside a trans tag?

10. +++ b/core/modules/help_topics/tests/src/Kernel/HelpTopicCoreTopicsTest.php
    @@ -0,0 +1,178 @@
    +    $this->assertTrue(empty($text), 'Topic ' . $id . ' Twig file contains only translated text');
    

    and could use assertEmpty here

11. +++ b/core/modules/help_topics/tests/src/Kernel/HelpTopicCoreTopicsTest.php
    @@ -0,0 +1,178 @@
    +    $doc->loadHTML($body);
    

    nice

12. +++ b/core/modules/help_topics/tests/src/Kernel/HelpTopicCoreTopicsTest.php
    @@ -0,0 +1,178 @@
    +        $this->assertTrue($num_headings[$level - 1] > 0 || $num_headings[$level] == 0,
    

    nice!

Addressed, I think, all the review comments in #12 -- either by changing code or updating comments so it's clearer what is happening and why (I hope!). A few notes:

• Item 4 -- that code you said "nice" about came from the original patch from @scott_euser (see issue summary -- please credit him on this issue!).
• When I switched to using array_filter for calculating top_level, the result is an array of definitions (not of IDs), so I left it that way. Some code changed from in_array to isset as a result.
• Passing both $definitions and $top_level to the verifyTopic function
• Rearranged a bit on checking the top_level logic. I think it's a bit clearer.

Good question... I hadn't realized that we were throwing an exception if an unrecognized meta was encountered. Let's just leave it as it is, and I'll update the issue summary with a proper list of what we're testing and what we're not testing.

I did add a note that there is (somewhere) in Core a module install/uninstall test that goes through every module in core and does some basic tests. We could potentially add to that so that in addition to the hook_help test it is already doing, we load all the help topics and render them with Twig, to verify they will render. We can't really do that here due to Twig variables for URLs probably not working.

I think it makes sense to see if that can be done in this issue, so let's leave it at Needs Review for the moment and I'll take a shot at that.

Hm. I took a look at InstallUninstallTest

I expect that we don't want to add an Experimental module's tests to that code. So... we either need to
a) Postpone adding a test to render all the Twig topics until this module is Stable.
b) Write a separate test that would take a long time to run, and do something similar to InstallUninstallTest, (install modules one by one) but verify the help topics instead of the hook_help (and omit all the testing that modules can be installed/uninstalled). Plan to fold it into InstallUninstallTest eventually.
c) Write a test that would install all the non-hidden and non-test modules at once, and verify all the help topics can be rendered. This would not be as rigorous as testing with the modules individually installed, because it might mask problems in routes not being defined, but it would be better than nothing.

I think I'll do (c), but also create a follow-up issue to add to #3037230: Finalize the merge of Help Topics into Help that would fold the test into InstallUninstallTest so it would be more rigorous. If the test fails at that time, we can adjust the topics then.

I created the follow-up issue, and here is a patch that checks rendering. Updating summary. I also made a trait to do some things that were needed for both the kernel test and the rendering test.

Addressing 2 coding standards issues from last patch. I edited patch file directly (took out 2 spaces and added 1 space) and did not make an interdiff, hope that is OK, it's exactly the same code.

It looks great, just one more question about code duplication with discovery of extensions

+++ b/core/modules/help_topics/tests/src/Functional/HelpTopicsRenderTest.php
@@ -0,0 +1,71 @@
+  public function testHelpRendering() {
...
+    $module_directories = $this->listDirectories('module');
+    $this->container->get('module_installer')->install(array_keys($module_directories));
+    $theme_directories = $this->listDirectories('theme');
+    $this->container->get('theme_installer')->install(array_keys($theme_directories));
...
+      $this->listDirectories('profile');
+    $directories['core'] = 'core/help_topics';
...
+    $definitions = $this->discoverAllTopics($directories);

+++ b/core/modules/help_topics/tests/src/Kernel/HelpTopicCoreTopicsTest.php
@@ -0,0 +1,143 @@
+  public function testCoreTopics() {
...
+    $directories = $this->listDirectories('module') +
+      $this->listDirectories('theme') +
+      $this->listDirectories('profile');
...
+    $definitions = $this->discoverAllTopics($directories);

+++ b/core/modules/help_topics/tests/src/Kernel/HelpTopicDiscoveryHelperTrait.php
@@ -0,0 +1,72 @@
+  protected function listDirectories($type) {
...
+    $lister = \Drupal::service('extension.list.' . $type);
+    foreach ($lister->getAllAvailableInfo() as $name => $info) {
...
+      if ((strpos($path, '/tests') === FALSE) &&
+        (strpos($path, '/testing') === FALSE)) {
...
+  protected function discoverAllTopics(array $directories) {
+    $discovery = new HelpTopicDiscovery($directories);
+    return $discovery->getDefinitions();

I see this duplicate a lot of code for each test to collect directories, but tests are part of core

So instead of strpos() the \Drupal\Core\Extension\ExtensionDiscovery::scan() could filter out testing modules.

It makes it more inline with HelpTopicDiscovery::findAll() - which could be extended with $include_tests = FALSE argument (probably follow-up)

I ran into a lot of problems when I was running this test interactively and trying to get it to run, because if it discovering themes, modules, and profiles that were really just for testing -- which is why I ended up with strpos. If you know of a way to make a list of modules, or themes, or profiles that will get everything (not the ones currently installed) and exclude testing ones, yes, let's do it. But I couldn't figure it out from the documentation of those service classes. ?? strpos seemed the simplest option that actually works.

Also, the difference between the Kernel test and the Functional test is that in the Functional test, we actually need to install the modules/themes so we can test rendering. The reason for that is that the rendering uses routes that would not be defined without the modules enabled. But the Kernel test is very quick and doesn't actually render anything. They could be combined, but I was under the impression that we wanted as much testing as possible to live in Kernel tests and not Functional, so I left them as two separate tests (with a trait to reduce code duplication as much as I thought was possible).

So... Not sure what you want done with the tests here?

PS: I would really like to get this resolved/committed as soon as we can, so that people writing new help topics as part of #3041924: [META] Convert hook_help() module overview text to topics will see a test fail if their help topics do not pass these tests. Thanks!

To reduce duplicated code, I folded the Kernel and Functional test into one test. I decided it didn't make sense to have two separate tests running the same discovery. I didn't make an interdiff for this merge -- it would have been almost the complete patch, so it seemed pointless. But it's the same code, just merged together, including all of the asserts from both of the tests, and the methods from the helper trait.

After I did that code merge, I took a look at the question of whether I could get rid of the strpos stuff (see comment #19). Here is the code I ended up with in listDirectories():

    $directories = [];

    if (!isset($this->extensionDiscovery)) {
      $this->extensionDiscovery = new ExtensionDiscovery(
        $this->root,
        FALSE,
        [],
        $this->siteDirectory
      );
    }

    // See issue https://www.drupal.org/project/drupal/issues/3076098
    // to understand why this is necessary.
    if ($type == 'profile') {
      $this->extensionDiscovery->setProfileDirectories([]);
    }
    else {
      $this->extensionDiscovery->setProfileDirectories(['not_a_directory']);
    }
    $extensions = $this->extensionDiscovery->scan($type, FALSE);
    $directories = [];
    foreach ($extensions as $machine_name => $extension) {
      $directories[$machine_name] = $extension->getPath();
    }

As you can see, first I ran into #3076098: ExtensionDiscovery documentation is wrong about scanning profile directories, which I had to put in a strange work-around for.

And then, even with this work-around, it still found all the testing profiles in its scan, in spite of the fact that I told it to NOT find testing items. Here's the list of profiles it found:

    [demo_umami] => core/profiles/demo_umami
    [minimal] => core/profiles/minimal
    [standard] => core/profiles/standard
    [testing] => core/profiles/testing
    [testing_config_import] => core/profiles/testing_config_import
    [testing_config_overrides] => core/profiles/testing_config_overrides
    [testing_install_profile_all_dependencies] => core/profiles/testing_install_profile_all_dependencies
    [testing_install_profile_dependencies] => core/profiles/testing_install_profile_dependencies
    [testing_install_profile_dependencies_bc] => core/profiles/testing_install_profile_dependencies_bc
    [testing_missing_dependencies] => core/profiles/testing_missing_dependencies
    [testing_multilingual] => core/profiles/testing_multilingual
    [testing_multilingual_with_english] => core/profiles/testing_multilingual_with_english
    [testing_requirements] => core/profiles/testing_requirements
    [testing_site_config] => core/profiles/testing_site_config

I didn't create an issue for that yet. But I think the strpos is the best solution until these bugs in the discovery process are fixed.

Also as a note, if we want to use the above code, there is something missing from the paths it is returning, because the HelpTopicDiscovery found no topics. So that would also need to be fixed.

Anyway, here is the patch with the single merged test.

Created issue for the other problem with ExtensionDiscovery

As one more note: If you look at InstallUninstallTest, you will see that after scanning all modules, it then filters them:

// This essentially calls ExtensionDiscovery::scan(), with default constructor and params.
$all_modules = system_rebuild_module_data();
...
$all_modules = array_filter($all_modules, function ($module) {
      if (!empty($module->info['hidden']) || !empty($module->info['required']) || $module->status == TRUE || $module->info['package'] == 'Testing') {
        return FALSE;
      }
      return TRUE;
    });

So it is filtering modules based on what is in the .info.yml file -- excluding modules marked "hidden" or in the "testing" package.

An earlier version of this patch (which I don't think I ever uploaded) did something similar, but then when I got to themes and profiles this scheme didn't work... which again is why I ended up with the strpos filtering, which does work even though it is kind of crude.

So... once again... I think for this test we should just leave the strpos in, and not try to solve the related issues in ExtensionDiscovery when we really need to get a test in place for help topic syntax before we get too far in writing new topics (which is happening now).

Only one nitpick - property visibility, metadata is only a question of strictness

1. +++ b/core/modules/help_topics/tests/src/Functional/HelpTopicsSyntaxTest.php
   @@ -0,0 +1,230 @@
   +  public static $modules = [
   

   This property is protected in parent class

2. +++ b/core/modules/help_topics/tests/src/Functional/HelpTopicsSyntaxTest.php
   @@ -0,0 +1,230 @@
   +    // Remove the meta tags (already tested above), and Twig set and variable
   +    // printouts from the file.
   

   Not sure how strictly it should test metadata but
   - label and related covered (both name and content attributes)
   - for "top_level" - should it make sure that this meta has no other attributes?

+++ b/core/modules/help_topics/tests/src/Functional/HelpTopicsSyntaxTest.php
@@ -0,0 +1,230 @@
+    $lister = \Drupal::service('extension.list.' . $type);
+    foreach ($lister->getAllAvailableInfo() as $name => $info) {
+      $path = $lister->getPath($name);
+      // You can tell test modules because they are in package 'Testing', but
+      // test themes are only known by being found in test directories. So...
+      // exclude things in test directories.
+      if ((strpos($path, '/tests') === FALSE) &&
+        (strpos($path, '/testing') === FALSE)) {
+        $directories[$name] = $path . '/help_topics';
+      }
+    }

the $info is not used, so I'd add $names = array_keys($lister->getAllAvailableInfo());

Regarding the meta-data, the code in HelpTopicDiscovery does this:

        foreach (get_meta_tags($file) as $key => $value) {
          $key = substr($key, 11);
          switch ($key) {
...
            case 'top_level':
              $data[$key] = TRUE;
              if ($value !== '') {
                throw new DiscoveryException("$file contains invalid meta tag with name='help_topic:top_level', the 'content' property should not exist");
              }
 

So it looks like we have that covered. At least, it's verifying that there isn't anything in the value. It's not valid HTML syntax to have anything but name/value attributes on a meta-tag.... I think that's good enough?

Taking your other two suggestions though. [That $info was being used while I was developing the patch (I was trying to use it to filter out hidden modules/profiles) but I stopped using it and never took it out of the patch.] Let's see how this goes...

I think we can add some more tests to verify discovery and topic syntax fails in the ways we expect. Setting this to Needs Work until I have that done.

OK, here's a new patch, which adds some more discovery tests. This verifies that all the exceptions in HelpTopicDiscovery will be triggered with suitably bad Twig files. I switched from using a virtual file system to having actual topic files in a "bad_help_topics" subdirectory of the test module. Thoughts?

Ah. That last patch changed an Exception message (when I went to test it, I realized it was kind of written unclearly, so I updated it), and another test depended on that. So, fixing that. Also the coding standards error (unused use statement for the virtual file system that last patch doesn't use any more).

Also I thought up how to test the other syntax things, to make sure the test will catch the errors it should catch.

I don't have time to finish these updates today, but I'm working on them...

OK, here it is! See previous comment for what this patch does.

The test bot likes it. I think it's ready for final review/commit. Any thoughts?

This is the last issue for Beta for Help Topics that is not at RTBC. Any thoughts?

Still applies clean, I see nothing to add here

1. +++ b/core/modules/help_topics/tests/src/Functional/HelpTopicsSyntaxTest.php
   @@ -0,0 +1,317 @@
   +    $discovery_problems = [
   +      [
   +        'subdirectory' => 'provider',
   +        'file' => 'test.topic.html.twig',
   +        'message' => "file name should begin with 'expected_failure'",
   +      ],
   +      [
   +        'subdirectory' => 'top_level',
   +        'file' => 'expected_failure.topic.html.twig',
   +        'message' => "contains invalid meta tag with name='help_topic:top_level', the 'content' property should not exist",
   +      ],
   +      [
   

   these look like data providers, so should we use a dataProvider? or is this done on purpose because of the setup cost of a browser test?

2. +++ b/core/modules/help_topics/tests/src/Functional/HelpTopicsSyntaxTest.php
   @@ -0,0 +1,317 @@
   +    catch (\PHPUnit_Framework_Error_Warning $e) {
   

   These are an implementation feature of PHPUnit - and we're inadvertently coupling ourselves to a specific version of phpunit (those classes don't exist in phpunit version > 4 and are only provided via a BC shim in our bootstrap.php).

   I think instead we should catch the bad parsing in the discovery layer and throw our own specific exceptions. i.e. I would prefer we refactored the test to catch our actual exceptions instead of relying on their implementation details.

   E.g the code throws DiscoveryException, so shouldn't we be catching that instead?

The first one is definitely because of browser test, if we make it with dataProvider it will setup core for each case

The second a bit tricky
- invalid html catchable with libxml flags https://www.php.net/manual/en/libxml.constants.php so no exception could be used
- second is result of exception in child site, will debug how to catch it in different way

Somehow it makes sense to limit the test to scan only core...
I got few contrib modules in "modules" dir and they cause test failures

RE #38 - limiting the test to scan for only Core is probably a good idea, although if we don't do that, then a contrib maintainer could run the test on their contrib module and also verify their module's help topics are good?

RE #37... I think the idea in #36 of catching the parsing in the discovery layer and throwing our own specific exception makes more sense than what we are currently doing. I will not have time to make a patch to do that in the next 2 days, but I would have time to review one if @andypost makes a patch. Otherwise, I might have time to make a patch on Friday.

I came up with some time today, so I will make the next patch now.

Here's a new patch that:
(a) Only looks at topics in Core

(b) Removes the use of

catch (\PHPUnit_Framework_Error_Warning $e)

As a note, this was in there for the HTML parsing step in the test (not in Discovery), so I modified that code to use the XML library's error handling instead of PHP warnings, and I didn't have to modify the Discovery class.

(c) Changes

catch (\PHPUnit_Framework_ExpectationFailedException $e)

so that it looks like core/modules/system/tests/src/Functional/Menu/BreadcrumbTest.php and core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php ... I hope that is sufficient? If not... What we want to do in this test is to assert that a given topic fails the verifyTopic() method in the expected way (generates the expected assertion fail message). Is there a better way to do that? If so, we should fix this test and the other two in Core in the same way.

Fix coding standards messages.

(that last could be preg_grep just in case someone rerolls the patch but it's fine as is)

Want to RTBC it then? :)

Re #43 - I think strpos() is faster here instead of preg..

+++ b/core/modules/help_topics/tests/src/Functional/HelpTopicsSyntaxTest.php
@@ -0,0 +1,329 @@
+  /**
+   * Tests that help topic discovery fails in expected ways.
+   */
+  public function testDiscoveryExceptions() {
+    $base_bad_directory = \Drupal::service('extension.list.module')->getPath('help_topics_test') . '/bad_help_topics/discovery/';
+    $discovery_problems = [
+      [
+        'subdirectory' => 'provider',
+        'file' => 'test.topic.html.twig',
+        'message' => "file name should begin with 'expected_failure'",
+      ],
+      [
+        'subdirectory' => 'top_level',
+        'file' => 'expected_failure.topic.html.twig',
+        'message' => "contains invalid meta tag with name='help_topic:top_level', the 'content' property should not exist",
+      ],
+      [
+        'subdirectory' => 'bad_meta',
+        'file' => 'expected_failure.topic.html.twig',
+        'message' => "contains invalid meta tag with name='foo'",
+      ],
+      [
+        'subdirectory' => 'no_label',
+        'file' => 'expected_failure.topic.html.twig',
+        'message' => "does not contain the required meta tag with name='help_topic:label'",
+      ],
+    ];
+
+    foreach ($discovery_problems as $info) {
+      $fail_directories = [
+        'expected_failure' => $base_bad_directory . $info['subdirectory'],
+      ];
+      $this->expectException(DiscoveryException::class);
+      $this->expectExceptionMessage($fail_directories['expected_failure'] . '/' . $info['file'] . ' ' . $info['message']);
+      $this->discoverAllTopics($fail_directories);
+    }
+  }

This feels like a duplicate of existing tests - see \Drupal\Tests\help_topics\Unit\HelpTopicDiscoveryTest - if there is something missing it should be added there. I can't find anything in this test that is not covered in that test.

Doh! You are totally right. Here's a patch that removes that part of the test.

I think it makes sense to follow up to decouple unit vs functional tests better, but current patch is about syntax tests

This is going to need a small update now that #3069109: Replace help_topic meta tags with front matter is in. Also a reroll for the test help topics (new format).

Here is a new patch. What has changed:
a) Help topics added in this patch have front-matter metadata instead of meta tags. So, things like:

<meta name="help_topic:label" content="Making your site secure"/>
<meta name="help_topic:top_level"/>
<meta name="help_topic:related" content="menu_ui.menu_overview"/>

become

---
label: 'Making your site secure'
top_level: true
related:
 - menu_ui.menu_overview

That change is made in all the topics.

b) The syntax test had to be modified to remove front matter instead of meta tags in its syntax checking:

+    // Remove the meta tags (already tested above), and Twig set and variable
+    // printouts from the file.
+    $body = preg_replace('|\<meta.*\/\>|sU', '', $body);

became

+    // Remove the front matter data (already tested above), and Twig set and
+    // variable printouts from the file.
+    $body = preg_replace('|---.*---|sU', '', $body);

Along with a change to the error message for meta tags being gone:

$this->assertNotEmpty($body, 'Topic ' . $id . ' Twig file contains some text outside of front matter');

(used to say "outside of meta tags").

I've reviewed @jhodgdon's patch in #50 and it looks good to me...except for a couple of errors in a comment. I've attached an interdiff and a new patch fixing a typo and updating a TODO URL to the actual issue. (The one linked has been closed as a duplicate.)

In the patch's core/modules/help_topics/tests/src/Functional/HelpTopicsSyntaxTest.php:

+    // Verify that if we remove all the translated text, whitespace, and
+    // HTML tags, there is nothing left (that is, all text is translated).
+    $text = preg_replace('|\{\% trans \%\}.*\{\% endtrans \%\}|sU', '', $body);
+    $text = strip_tags($text);
+    $text = preg_replace('|\s+|', '', $text);
+    $this->assertEmpty($text, 'Topic ' . $id . ' Twig file has all of its text translated'); 

Looks like we are looking for Twig trans tags to strip out, along with whitespace and HTML. But we also can have Twig set tags to set a URL up, for example:

{% set help_topic_url = render_var(url('help_topics.help_topic', {id: 'help_topics_test.additional'})) %}

So, do we need to add to that preg_replace to account for Twig set tags?

Thanks for the new patch! Interdiff looks good to me.

Regarding #53, we are already removing set statements a few lines up from there:

// Remove the front matter data (already tested above), and Twig set and
+    // variable printouts from the file.
+    $body = preg_replace('|---.*---|sU', '', $body);
+    $body = preg_replace('|\{\{.*\}\}|sU', '', $body);
+    $body = preg_replace('|\{\% set.*\%\}|sU', '', $body);

So I think we're good. We have set statements in several of the existing core topics, which are all (with this patch) passing the tests.

Great! I'm +1 for an RTBC. Do we need another review?

Looking at deadlines and the road map parent issue, I say we do this contingent upon #3086075: Use Twig to strip Twig syntax from help topics files in the syntax checker happens before we deem it stable.

That sounds like a good idea. I'll add that to the Roadmap issue for Stable.

Here's a few tidy ups:

• Using better assertSomething methods
• Consitently using \Drupal::service() over $this->container0>get()
• Less methods because we plan to fold this into InstallUninstallTest

Wow, that's a lot of assertSomething methods I didn't know about. ?!?

+1 for leaving this at RTBC. Changes look fine.

Committed 41a9b82 and pushed to 8.8.x. Thanks!

@jhodgdon PHPUnit has a lot of assertSomething that are really nice and have excellent messages.

Totally agree with the follow-up #3086075: Use Twig to strip Twig syntax from help topics files in the syntax checker - was going to ask for it here - +1 on have that as a stable blocker.