Add GenericModuleTestBase and use it to test general module things

I've been thinking about this issue for ages. It'd be fantastic if Drupal shipped with a generic test that could easily be extended by any contrib or custom module that wanted to make sure it's working under common circumstances - for example a reinstall scenario.

Here's a start.

Adds GenericModuleTestBase. System and DbLog modules subclass it.

Tests hook_help() and uninstall/reinstall.

I started to look at moving HelpTopicsSyntaxTest over to this, but it relies on all modules being installed, not just the one being tested, so I think that's better left in its own test. Maybe we can factor bits out but should be its own issue.

Ideally:

1. Agree on the naming/namespace etc. - it could move to core/tests but then the dependency on help module seems a bit odd.

2. Once what's here is reviewed, add subclasses for all core modules. Might make an OK novice task.

it could move to core/tests but then the dependency on help module seems a bit odd.

Should the generic test somehow be extendable in some way?

Should the generic test somehow be extendable in some way?

This crossed my mind but no idea how. We can't rely on subclasses because we want people to inherit from one class then get new test methods/assertions for free, so needs to be a single central class not with functionality spread around modules. Same problem with adding various traits. Adding some kind of event system to tests so that help can add its assertion in seems like overkill.

I think we can park the extensibility discussion - I think it is okay for the focus of this issue to be to add a base class to core that can test any API provided by anything we include in the drupal/core package including optional modules like help. And if we want to extend/refactor this using PHPUnit's events or something like that in future then we can.

Are the namespaces off?

They were indeed.

Woo that was it!

I think we either need to add subclasses for all core modules here, or open a follow-up to do that, before this can be committed.

So I would vote for a follow up and maybe a CR?

The ticket for subclasses could get large I imagine and a lot back n forth. But getting this merged could allow contrib modules to start using.

Thoughts?

I think given the tests should not have any logic in them it'd be great to add them here. Plus we could add a test to ensure any new non test modules has such a test.

Also adding this to all modules in core is likely to help us discover if there are any issues with the base class.

Got me convinced @alexpott.

updated remaining tasks

Plus we could add a test to ensure any new non test modules has such a test.

I think testing all core modules here is good, but for me I would put 'testing the test coverage' into a follow-up since this is a blocker for gitlab ci performance improvements.

Added the magic. I thought about it when adding the property and rejected it initially since it could be hard to document, but it results in an empty class so really, what is there to document then?

Moving back to needs review - once we're happy with what modules have to do, we can tag this novice for applying the pattern to all core modules, but would be good to be fairly confident about that before we do it.

Added tests for all modules. Also, remove 'Currently' from a sting because I thought it was superfluous.

Plus we could add a test to ensure any new non test modules has such a test.

I think testing all core modules here is good, but for me I would put 'testing the test coverage' into a follow-up since this is a blocker for gitlab ci performance improvements.

Having this test coverage would make it more easier to rtbc this issue, and I think it would be very helpful to have. However because this is already a big boost I agree we shouldn't wait for this.

Some mini mini comment language nit-pic for not native tongues:

Nothing to assert for hidden, required modules.

Is the comma an extension meaning "required modules which are hidden" or is it a list like a and b, for "hidden and for required modules"?

The test unfortunately is not currently testing the module we hoped it was... see code comment on MR.

Fixed the very silly thing. Added a new empty method to the base class preUninstallSteps() and implemented that in forum and workspaces.

Back to needs review, changes from #27:

Removed the tracker and help_topics subclasses because both are deprecated. If we wanted to leave that in we'd have to add @group legacy to the test classes.

Handling the fact that the current database driver can't be uninstalled.

help_topics no longer implements a standard hook_help() because it's informing people its deprecated, so removing that test. Tracker test works with @group legacy.

All green, and seems all threads have been addressed. Marking but would this need a CR?

Added change record: https://www.drupal.org/node/3388092 →

Left some comments

I think we need to perform some asserts when we do the uninstall/install step

I also think we would want a unit test to make sure every module has one of these tests.

Added the unit test, which will fail because the test was removed from help_topics. I didn't add that back so we can have a fail test for this change.

Add a parameter to the test to list the modules that do not have a GenericTest. It seemed the simplest solution.

I was dreading adding the unit test because I figured we'd need to discover all functional tests and look for a subclass. Just looking for the class name is of course fine because this is core and we can control it.

Needs work for @alexpott's two points.

Yes unit test takes 1 second with the data provider. Pushed a commit for that.

All threads do appear to be resolved, so remarking.

Committed and pushed to 11.x

It doesn't apply to 10.1.x so can't backport it, setting to Patch (to be ported) to discuss whether we want to do that?

HelpTopicSyntaxTest is where the conflict is

Reasons to backport:

1. Contrib will be able to use the new base class 6 months quicker.
2. 10.1.x commit/scheduled runs (and the occasional MR) will be faster, especially if we keep backporting other changes.

I think those are both good enough reasons so opened a backport MR against 10.1.x. Merge conflicts were just a code comment.

Think that 100% makes sense, could we get a 10.1 MR?

Sweet

Backported to 10.1.x

Automatically closed - issue fixed for 2 weeks with no activity.

Hey everyone! Thanks for this, we were always manually implementing install / uninstall tests for all of our modules. This definitely solves this problem!
Although I have a few questions on "GenericModuleTestBase":

• I am fine with enforcing the implementation of "hook_help", but why are we enforcing specific strings inside the hook_help text?.
• Why are all generic tests cramped into one single "testModuleGenericIssues" method? This makes it very hard to override part of the generic tests (e.g. skipping the "hook_help" part).
• After installing / uninstalling a module, shouldn't we somehow check, that the site is still up? (e.g. checking the status code of the front page).

Just a few questions to stir up a small, friendly discussion! Have a nice day, everyone! 😊

If you have ideas for improving the work here, please open new issues, definitely keen to improve things if there are issues