While investigating how some things where suppose to work in ModuleHandler and what things it supported I was unable to find any tests. It seems that the testing of module handler is handled by all our other tests using it but there are no explicit tests.

Still a work in progress but this has ~41% line coverage and ~80% function coverage of ModuleHandler.

CommentFileSizeAuthor
2252991-psr4-reroll.patch26.01 KBxjm
#4 2252991-modulehandler-phpunit-tests-4.patch26.01 KBneclimdul
#4 2252991-interdiff-4.txt2.24 KBneclimdul
#1 ModuleHandler-tests.patch25.14 KBneclimdul
ModuleHandler-tests.patch19 KBneclimdul
ModuleHandlerCoverage.png127.15 KBneclimdul
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
Status: Active » Needs review
FileSize
ModuleHandler-tests.patch25.14 KB

80% coverage. I'm not sure the other code can be effectively tested in unit tests. I'm sure they can but they're going to request a lot of mocking and in some cases knowledge of code I'm not familiar with. This seems enough to get a unit test class into code.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
Issue summary: View changes
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Issue tags: +PHPUnit

Nice work!

  1. +++ b/core/tests/Drupal/Tests/Core/Extension/ModuleHandlerTest.php
@@ -0,0 +1,492 @@
+    $this->assertTrue($this->moduleHandler->load('module_handler_test'));
+    $this->assertTrue(function_exists('module_handler_test_hook'));

    Does phpunit tests run in a particular order? Can we do a !function_exists before the load call? 

  2. +++ b/core/tests/Drupal/Tests/Core/Extension/ModuleHandlerTest.php
@@ -0,0 +1,492 @@
+    $module_handler->expects($this->once())->method('load');

    Should we ensure that load is called with the proper argument?

    On top of that I wonder whether we could also provide a similar test as well: add a module, call reload and ensure it is loaded. 

  3. +++ b/core/tests/bootstrap.php
@@ -7,6 +7,8 @@
+define('DRUPAL_ROOT', realpath(__DIR__ . '/../../'));
+

    We do have a couple of tests where this constant is defined inside the test itself, so maybe open a follow up to clean this up.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
FileSize
2252991-interdiff-4.txt2.24 KB
2252991-modulehandler-phpunit-tests-4.patch26.01 KB

1) I think so but seems like a valid test. At worst it keeps us from accidentally breaking it in the future which is still good.
2) Yes and it doesn't hurt. We actually want to ensure all modules are "loaded" on second reload. @see interdiff
3) Yeah, not my favourite part of the patch. I've moved this next to the other goofy bootstrap constant. I'll open the follow up.

The last submitted patch, 1: ModuleHandler-tests.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

This is a really nice improvement and we can always extend in the future if the code is written properly.

+++ b/core/tests/bootstrap.php
@@ -92,8 +90,9 @@ function drupal_phpunit_register_extension_dirs(Composer\Autoload\ClassLoader $l
-// Look into removing this later.
+// Look into removing these later.

Nice catch!

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented

Follow ups #2270323: Remove DRUPAL_ROOT constant redefinitions from unit tests #2270331: Provide DRUPAL_ROOT accessor on objects to allow better phpUnit unit tests

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 4: 2252991-modulehandler-phpunit-tests-4.patch, failed testing.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
Status: Needs work » Needs review

4: 2252991-modulehandler-phpunit-tests-4.patch queued for re-testing.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
Status: Needs review » Reviewed & tested by the community

back to rtbc

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed 
diff --git a/core/tests/Drupal/Tests/Core/Extension/modules/module_handler_test/module_handler_test.info.yml b/core/tests/Drupal/Tests/Core/Extension/modules/module_handler_test/module_handler_test.info.yml
index f5fd86c..7ae9fde 100644
--- a/core/tests/Drupal/Tests/Core/Extension/modules/module_handler_test/module_handler_test.info.yml
+++ b/core/tests/Drupal/Tests/Core/Extension/modules/module_handler_test/module_handler_test.info.yml
@@ -4,4 +4,3 @@ description: 'Test module enabled by default in module handler unit tests.'
 package: Testing
 version: VERSION
 core: 8.x
-hidden: true

Hidden is no longer need for test modules. Fixed during commit.

Committed 3d5560b and pushed to 8.x. Thanks!

  • Commit 3d5560b on 8.x by alexpott: 
    Issue #2252991 by neclimdul: PhpUnit tests for ModuleHandler.

Status: Fixed » Closed (fixed)

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