Add more test coverage to Help Topics

On #3041924-52: [META] Convert hook_help() module overview text to topics, andypost suggested we should add tests for the syntax/content of help topics. My response on the next comment:

That sounds like a good idea.

The current tests for the module overviews from hook_help test for certain content, like the appearance of the module name. But I don't think we can/should test for content in help topics, because they're not supposed to be module-oriented. I also don't think we should require each module to have at least one topic file, because there may be circumstances where a particular module's files are put elsewhere (like a group of modules that share topics or putting them in the core space), or where a particular module doesn't need help because its tasks are described elsewhere.

So... for templates that do exist, I think we could write a test. Here's the sample template file (linked in the issue summary of the other issue): https://www.drupal.org/files/issues/2019-04-08/sample.html_.twig_.txt

Here are some things I think we could maybe test for:
- meta tag for the title (label) must be present (help_topic:label)
- allowed meta tags: help_topic:label, help_topic:top_level, help_topic:related -- there must not be any others [although maybe someone would write a subclass of the plugin manager that would use other meta tags?]
- there must not be an H1 tag
- if there is an h3 tag, there must be an h2 tag; if there is an h4, there must be an h3; etc. (proper hierarchy of headings)
- can we test for HTML syntax validation, once it's rendered from Twig?
- is it possible to test whether all of the text in the file is actually wrapped in translation commands {% trans }?

Yes, we at least need a test that tries to load all the help topics and display them, and makes sure they have a title! Glad you are working on this!

Yes, translation already has another issue.

The other issue for translation is not actually about testing. So... Adding translation and also the ideas from #3 to the issue summary as things we can possibly test.

We already have HelpTopicTranslationTest actually, so we don't need to add more testing for translation (the Roadmap also lists this task as done). Unit tests and cache tests were the two areas identified by the core maintainers as needing attention, and then we added the other ideas about having tests to validate new topics (which I still think are a good idea).

Thanks for your interest in this issue, and the patches!

A few notes:

a) It seems like these sorts of tests should maybe not be based on BrowserTestBase unless that is really necessary? If we are checking the structure of Twig templates, I don't think we need a browser? Probably could be a Kernel test? See https://api.drupal.org/api/drupal/core%21core.api.php/group/testing/8.8.x

So probably you would need to add a new test file rather than adding to this one.

b) Also you have added some coding standards problems to the file, so you'll need to remove those problems. To see those, click through to the search results, and you'll see where it says there are coding standards problems.

c) You should not do your own help topic discovery. You should instead use the Help Topics Manager class, I think? That way you will also test help topics provided by themes and profiles.

d) I'm actually wondering for help topic syntax checks if we should add Coder rules instead of testing this way? Because then it would also cover contrib when they get into writing help topics.

Ah, that makes sense about the Discovery process. Very good point. But you'll need to also check the theme directories and the core directory to be complete, just like the getDiscovery() code you quoted:

    // We want to find help topic plugins in core, modules and themes in
      // a sub-directory called help_topics.
      $directories = array_merge(
        ['core'],
        $this->moduleHandler->getModuleDirectories(),
        $this->themeHandler->getThemeDirectories()
      );

We may not have any topics in there currently, but we plan to shortly.

Regarding Coder, the project is https://www.drupal.org/project/coder/ ... I do not know if it currently checks Twig files, but it seems like it would be possible. If so, we would create a child issues of this one, put it in that project, and work on the rule there. We'd probably need to create a Twig help topic file that violated each one of our tests, and one that would pass our tests, and then use that in some tests in that project.

Once that rule got into the Coder code, we would then patch in this issue so that Core would run that test by default. The list of coder checks is inside the core/drupalci.yml file.

I think we should check the other syntax things in the Coder issue also. Updating issue summary, and I'll also update the Coder child issue.

I am going to add the Coder issue as a separate item on #3027054: Help Topics module roadmap: the path to beta and stable so we can move forward on this and not delay it on Coder stuff.

What else needs to be done here on this issue?

I took a look at the current patch, and I'm not sure exactly what it is supposed to be testing for really -- I guess just that the Discovery class can read all of the topic files and get their meta-data out? I guess that is good, and I don't see any glaring problems in the patch so far.

But what we really need, in order to satisfy this issue, is:
- Tests for the cache meta data for the admin/help page (with Help Topics enabled)
- Tests for the cache meta data for an individual help topic page
- Unit tests on the various classes that are part of the help topics module, such as HelpTopicDiscovery, the plugin manager, etc. [Some of them may already have unit tests.]

So, setting this to needs work because we need more tests added.

So, I took a look at what tests we have vs. need:

a) The only Unit test we have is in core/modules/help_topics/tests/src/Unit/HelpTopicDiscoveryTest.php
It seems to cover the same exception that this test covers, so we probably don't need that exception test. And it is a unit test for the Discovery process. So, that is taken care of.

b) Other classes that we don't have unit tests for:
- HelpTopicPluginManager [not sure if it needs a unit test? probably? do other plugin managers have unit tests?
- HelpTopicTwig
- HelpTopicTwigLoader

c) Functional tests we have:
- HelpTopicTranslationTest: Tests translation of a help topic title, and of something in the body of a help topic

- HelpTopicTest:
-- verifies that the Topics section is on admin/help, that various links/titles are there and as expected, and that they are in the right order
-- verifies that topics go away when you uninstall a module/theme
-- tests the plugin manager alter hook [should that be on the unit test for the manager?]
-- tests correct access to view topics
-- tests breadcrumbs [so I think we don't need a unit test for the Breadcrumb class?]

d) Functional tests missing: cache tags on admin/help and on an individual topic page

e) Topic tests we have in this patch: Discovery process can load all Twig templates (only meta-data is loaded)

f) Topic tests we don't have in this patch:
- Syntax checks that we should probably do in Coder on that other issue
- Can we actually use Twig to render/display the topic... Maybe use the Twig loader to parse the topic? There could be problems with URL variables if routes are not defined (if the module isn't enabled)?

Adding previous comment to issue summary, more or less, and adding issue summary template.

In investigating #3066512: Add checks for syntax and display of help topic Twig template files I found out that Coder doesn't sniff Twig files. So, that issue is now back in Core and I'm updating issue summary.

Adding some information about caching to the issue summary item (d).

I think that the test in the latest patch here would be better off as part of #3066512: Add checks for syntax and display of help topic Twig template files, so I'm hiding its file here, uploading there, and will ask for scott_euser to get credit on that other issue.

This is the one remaining issue in our Roadmap to Beta that doesn't have a patch yet (since the other patch, which was all about help topic syntax, got moved to #3066512: Add checks for syntax and display of help topic Twig template files). So... I'm going to work on this today. Also minor update to issue summary.

Here is a patch that adds unit tests for the HelpTopicTwig and HelpTopicTwigLoader classes. They both pass locally. No interdiff -- this is a totally new patch.

That is all I have time to do for the next few days, so unassigning in case someone else wants to write more tests. See issue summary for list of what needs to be done.

Adding more notes about what has been tested to the issue summary... I think we have adequate test coverage for the help topic plugin manager (see new notes in issue summary). So all that remains to be done (I think?) is to add tests for the cache tags/contexts.

So, here it is! I think this issue is done... assuming the bot agrees.

Not quite I guess -- 2 coding standards messages. Here's an interdiff and new patch.

OK. The only difference between the patch in #37 and in #30 is changing directories to be not static (as suggested in #32 review). The other thing suggested there was to use the trait for cache tags, but as noted in #35, we couldn't do that because it does exact comparison, which is not what we are doing here. So, I think this should be back to RTBC.