Problem/Motivation

#3386448: Get rid of InstallUninstallTest is a single test that installs every module, then runs some standard checks.

HelpTopicsSyntaxTest is another one.

While these catch issues in modules that otherwise might not add their own coverage, they're very inefficient in that a single test run can take several minutes to complete. They also inherently don't enable contrib to test the same things.

Steps to reproduce

Proposed resolution

Add GenericModuleTestBase, this should require only an empty subclass.

This test would then install and uninstall the module, check hook_help(), and possibly other things that we'll think of later.

Then all core modules (but not test modules) can implement the test.

If we still want to ensure coverage of all core modules, we could add a unit test that discovers whether all non-testing modules have a subclass of this test.


namespace Drupal\Tests\my_module\Functional;

use Drupal\Tests\system\Functional\Module\GenericModuleTestBase;

/**
 * Generic module test for my_module.
 *
 * @group my_module
 */
class GenericTest extends GenericModuleTestBase {}

Remaining tasks

Add tests per module, this should be scriptable since the only difference between the test files is the module name.

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3386458

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

catch created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

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.

catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes

catch opened merge request !4752

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review

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.

catch’s picture

catch
he/him
Primary language English
commented

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.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

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?

catch’s picture

catch
he/him
Primary language English
commented

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.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

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.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Are the namespaces off?

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

They were indeed.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Woo that was it!

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

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.

smustgrave’s picture

smustgrave commented

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?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

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.

smustgrave’s picture

smustgrave commented
Issue summary: View changes
Status: Needs review » Needs work

Got me convinced @alexpott.

updated remaining tasks

catch’s picture

catch
he/him
Primary language English
commented

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.

catch’s picture

catch
he/him
Primary language English
commented

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?

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

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.

catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes
catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes

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

quietone’s picture

quietone commented
Issue summary: View changes

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

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community
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.

dqd’s picture

dqd
Location London | N.Y.C | Paris | Hamburg | Berlin
commented

Minor comment language nit-pic for non native tongues:

Nothing to assert for hidden, required modules.

Is the comma meaning "required modules, which are hidden" or is it like "for hidden and for required modules"?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

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

catch’s picture

catch
he/him
Primary language English
commented

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

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

Back to needs review, changes from #27:

Add @group legacy to the tracker and help_topics subclasses because both are deprecated.

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

catch’s picture

catch
he/him
Primary language English
commented

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.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

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

catch’s picture

catch
he/him
Primary language English
commented

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

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs review

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.

quietone’s picture

quietone commented

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.

quietone’s picture

quietone commented

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

catch’s picture

catch
he/him
Primary language English
commented

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.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +DX (Developer Experience), +Test suite performance
catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Needs work

Needs work for @alexpott's two points.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

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

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

All threads do appear to be resolved, so remarking.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Version: 11.x-dev » 10.1.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

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

  • larowlan committed 6d934850 on 11.x 
    Issue #3386458 by catch, quietone, alexpott, larowlan: Add...
catch’s picture

catch
he/him
Primary language English
commented
Status: Patch (to be ported) » Needs review

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.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

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

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

catch opened merge request !4881

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Sweet

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Fixed

Backported to 10.1.x

  • larowlan committed 9b16ae0a on 10.1.x 
    Issue #3386458 by catch, quietone, larowlan, alexpott: Add...

Status: Fixed » Closed (fixed)

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

grevil’s picture

grevil commented

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! 😊

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

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