Integrate help test into module uninstall test

Here's a first pass... Locally, it doesn't seem to fail where it should though, and I am not sure why. For instance, the test I wrote on #2473105: Update hook_help texts that link to modules that can be uninstalled fails on the Comment module if Node is not enabled, but this one doesn't. I don't understand why that would be? Let's see what the test bot thinks.

Yeah... this test doesn't fail at all the places that the one on #2473105: Update hook_help texts that link to modules that can be uninstalled does.

I don't understand it, because I think that this module install/uninstall test does the same thing that one does: install a module with only its dependencies, test the help (this I added in the patch here), and then uninstall it. But on the other issue, that test causes a problem, and here it doesn't. ??!?

Well, I see one difference. The test I wrote on the other issue does this:

a) find a list of all the modules

b) for each one:
b.1) Install the module
b.2) Test the help
b.3) Uninstall the module and anything else that was installed automaticaly (detect this by looking at the module list before and after)

The Module Install Uninstall test does the following:

a) find a list of all the modules

b) for each one:
b.1) Install the module
b.2) Test the help
b.3) Uninstall the module itself but not dependencies

c) Uninstall all the dependencies, detected via what was in the module dependency list in the info files for each module.

This means that for instance, by the time you get to the Menu UI module, you've got Block installed, because block_content depends on block and B comes before M.

So. I'll need to modify this module install uninstall test a bit more if it's to be a good test for the help. I think it will also make it a better test in general but it will run a bit slower because it uninstalls more along the way.

Let's try this one.

Trying again. Not bothering with interdiffs until someone else is following this issue and/or reviewing patches.

The test bot appears to have stalled on testing this patch for some reason. Uploading again.

OK, now the test is working properly, or close to.

It looks like you cannot actually disable the Forum module fully. The existing module install test doesn't detect this. It's causing several other problems in the test.

And there are several problems that are fixed by the patch on #2473105: Update hook_help texts that link to modules that can be uninstalled, which are now detected by this test. So maybe I'll wait until that issue's patch is committed and retest to see what can be done about the remaining failures.

Also filed
#2488632: Help test module should be in the Testing package
because this test is now testing the help_test module and I think it should be skipped. Hopefully that will not cause other problems.

#2473105: Update hook_help texts that link to modules that can be uninstalled was committed finally. Rerunning this test to see what fails now.

So the test is now finding two problems:

a) The Config Manager module has the wrong text in its help. This is being worked on: #1831798: Update hook_help() for config manager module

b) Apparently you cannot uninstall the Forum module after you install it. The existing install/uninstall test doesn't figure this out. I'll look into it...

So I looked into the Forum problem.

The reason Forum cannot be uninstalled is that you have to delete the "General Discussion" taxonomy term that is created by the Forum module when it is installed. Apparently, no other module has this problem. You can see this on the Modules page if you expand the Forum module -- it tells you that in order to uninstall you have to delete the taxonomy term first.

I'm tempted to write a special case into this test and see if it will then pass.

Huh, there was already special case code in there for forum module but it wasn't working. Let's try this instead.

Whoops, fix error in delete code.

OK, now we're talking! Just that one config.module problem, which isn't getting fixed quickly on the other issue.

So, I can fix that in the patch and then I think we should commit this test change, which actually improves the existing Module Install Uninstall test (which didn't have the Forum problem fixed correctly as far as I can tell).

All green! Actually seeking reviews now. ;) Summary and beta evaluation are updated...

Whoops! :) Well, that one line doesn't seem to be required in this test, since obviously it isn't doing anything. Removed it. (I needed it in the help test for some reason but apparently not here.)

Removed that line completely. No other changes.

Fixing typos/wording in issue summary.

Thanks for taking a look!

I realized that the config help line there was also not conforming to our help text standards in the link to the online docs. Added an assert for that. There may be test failures... anyway here is a new patch with that and the fixes for #28.

Let's see if this one turns green... I have my doubts that all the help has the right online docs link, but you never know, since ifrik recently reviewed them all. We'll see! ;)

Oh yeah I wanted to add Parent to this issue.

Whoops, I screwed up the assert for the link. Dang 2nd vs. 3rd argument.

That's what I was afraid of. There are 23 modules that are legitimately failing this test due to online docs links that don't follow our standard text of "online documentation for the Foo Bar module". So with this test assertion, we'd need to fix up the links to online docs problems. I think we should do that in a separate issue.

So what I've done is make that separate issue and put in a @todo here for that assertion and commented it out. The separate issue is #2493475: Fix online docs references in Help to conform to our standards and it's already marked as "related" to this one.

I've left the Config module fixes in here, because that's just one module and it makes the rest of the test pass.

Oh, nitpick: realized I missed a blank line before the end of the test class.

So, hopefully this one will turn green and then I think it's ready go go; ready for other opinions. ;)

Summary update for this latest development.

Thanks for the review, I can address your code points...

Regarding the philosophical questions..

What this changes: Previous to this patch, the module install/uninstall test, for each module, installed it, and then unistalled just the module itself, while adding the dependencies to a list of modules to be uninstalled later. Also the dependencies were then omitted from install/uninstall if they came later in the loop. So by the end you had quite a long list of modules that were not tested.

With this patch, what changes is that each module's dependencies are uninstalled along with the main module for each step in the loop, and the test verifies that they all go away. This is slower, but it is necessary to find problems in the help -- we had several problems with unstated dependencies in the help pages, like if Comment was on but Node wasn't, it would crash.

Regarding "could we also check the help page for all modules between installing and uninstalling them again"... Maybe I am not understanding your question, but we can't try to display help for modules that are uninstalled. The help only displays if the module is installed, it's a hook. The test is already checking the help page for modules after they are installed, except for the ones like Help and required modules, which are checked outside the main install/uninstall loop, so ... I don't get the question.

Regarding time and whether we should do this... If we do not add this test, we're going to run into more problems with Help pages that introduce unstated dependencies -- before #2473105: Update hook_help texts that link to modules that can be uninstalled we had about 10 of them in Core. I would like to avoid that, and in the process also enforce (via a test) that new modules have help that at least minimally satisfies our standards for help. The way to avoid having more is to have a test.

The test that I wrote on #2473105: Update hook_help texts that link to modules that can be uninstalled was slow. This one is undoubtedly a bit less slow because it is combined with another test. I don't know how to figure out how much time it adds.... my local machine is a LOT slower than the test bot and the various bots are not all the same speed. Any suggestions on how to figure out the additional time?

Anyway... If we don't want to test this, then OK but we will undoubtedly have problems in the future because we had quite a few in the past.

I rerolled the patch, and applied the three code suggestions from #36. The previous patch didn't apply, so an interdiff was not easy to do, sorry.

A note on item 1: I decided for consistency to make little methods called assertModuleNotInstalled and assertModuleSuccessfullyInstalled, so that I can call these same methods both for Help and for the other modules in the loop. This maintains consistency without the copy/paste.

A note on item 2: Since Forum is the only module with a pre-uninstall routine, I just took out the "let's plan for more" code, and called the method directly with an (if it's forum) check:

     if ($name == 'forum') {
        // Forum has an extra step to be able to uninstall it.
        call_user_func(array($this, 'preUninstallForum'));

item 3: fixed the @param array $info into @param string $name. whoops.

Typo. Also simplified calling the forum uninstall function.

Well, those new test fails are valid: it's detecting that two newly added test modules do not have help. They should be marked internal and they're not, apparently. Here's a fix.

Reroll. Several of the bugs identified by this test have since been fixed, so a few hunks are missing from the previous patch.

Uh oh, patch needs an update. The 8 exceptions are an unrelated failure, but the rest are due to this patch. Install/Uninstall UI changed.

Let's try this one...

One more UI change I didn't catch.

Um... I am not sure I understand #51. The test is now passing on the test bot. If you are getting a "maximum execution time exceeded" error, I think you can change the settings in your php.ini to make it allow a longer run time.

Thanks for doing that!

So the 3 test failures here are legitimate bugs. Two causes:

a) The dynamic_page_cache module help states its module name different from what is in its yml file. Apparently the yml name was changed and hook_help() wasn't, or so I'm assuming.

b) The update_test_postupdate module is not marked as hidden/testing and it should have been.

Yet another illustration of why we need this test so that these errors can be fixed before they're committed to core!

So we can fix both of these problems with the following patch, I think... Also the @todo has been satisified so uncommenting that line in the patch.

Let's see how this goes...

That's odd. The test results are not displaying right. Anyway, there are apparently two online documentation links that are failing:

ail: [Other] Line 75 of core/modules/system/src/Tests/Module/InstallUninstallTest.php:
Correct online documentation link is in the help page for dynamic_page_cache

fail: [Other] Line 75 of core/modules/system/src/Tests/Module/InstallUninstallTest.php:
Correct online documentation link is in the help page for filter

So not all of #2493475: Fix online docs references in Help to conform to our standards was apparently fixed. That is easy to do though... one more patch/interdiff.

Waiting for those two issues will delay this patch. Can we just get this patch reviewed/committed rather than splitting those off into other issues? If this patch had already been committed, we would have avoided issues...

Thanks testbot... Patch 61 is old. Patch 63 has passed. Someone want to review this please?

Actually something seems to be wrong with the test reporting. I'm going to upload the latest patch again and see if the bot can figure it out this time.

Rerolled - indeed, those two testing modules in System have been fixed elsewhere.

So I think that the container rebuilds are necessary -- at least, when I was creating this test a while back, they were. But to see if that is the case, here are two patches:

a) Straight reroll - same exact patch without those two yml files with Testing added.

b) Same as (a) but with the two container rebuild lines removed. I do not think it will work, but if it does, we can commit patch (b); if not, we can commit patch (a).

Setting back to RTBC on account of it's the same patch ... let's see what the bot thinks.

Yeah, that is what I suspected: the patch without the container rebuilds does not work. So... marking the other one back to RTBC as it was before.

Oh wait. The no-rebuild patch only failed due to a test bot glitch on drupal-qa. I guess we should commit that one, as it passed on drupal-ci.