Remove $install parameter from DrupalUnitTestBase::enableModules(), encourage individual schema tables

DrupalUnitTests are unit tests, extended unit tests.

Like any other unit test, all we're doing is to load code, and we execute it. Like any other unit test, we have a container, so as to be able to use and mock services.

The extension is that we're accounting for modularity. We're providing default implementations of services of the base system, and optionally, services of dependent modules and other modules. The most important service we're providing is the module/hook system, since pretty much every module functionality in Drupal relies on that, and that's why we're loading and enabling modules in a fixed module list without installing them.

We're doing that, because 1) authoring tests for modules with dependencies and contributed modules would be tremendously hard to do otherwise, and 2) mocking base system services as well as services of dependent modules and other modules in a bare unit test would lead to completely unrealistic test results.

Having to install anything in a DUTB test is the curse, not a desire. If you need to install something or too much, then your code isn't unit-testable at all, and a web test is more appropriate.

The only reason for why some DUTB tests are installing particular modules is that they need their database schema/tables, since the tested code attempts to read or write to it. That's not any different to a DatabaseCacheBackend unit test, which equally needs to create the schema first.

Therefore, this issue "works as designed" from my perspective.

That said, if anything, then we can consider to remove the $install flag from enableModules(), so as to encourage test authors to use installSchema() with individual/selective schema tables instead. (That wouldn't prevent them from using module_enable() though, but at the same time, there's actually nothing wrong with using it.) However, it would probably clarify that installing modules in DUTB tests is discouraged.

Yeah, sure.

However, I don't think we need to rename $modules to something else. I think the consistency with WebTestBase::$modules is fine there. One thing less to learn and bear in mind. The fact that it's a unit test should already make clear that nothing gets installed in any way by default.

So the actual tasks would be:

1. Remove $install parameter from DUTB::enableModules(array $modules).
2. Add a DUTB::installConfig($module) to install a module's default config. (or multiple? $modules?)
3. Add a DUTB::installSchema($module, $table) to install a database table schema. (already exists)
4. Add a DUTB::disableModules() to disable modules.

This should wait for #1331486: Move module_invoke_*() and friends to an Extensions class though, so postponing.

That said, we can already check how much is going to fail... ;)

I have to say, I always felt lazy using enableModules() knowing I probably only needed on table, so I'm glad to see this being rectified. It will take some legwork to find out which tests need which tables, but I'm +1 to this as a whole.

Re-postponing on #1331486: Move module_invoke_*() and friends to an Extensions class.

re: #11: Multiple topics/issues there:

1. The typical way to resolve such hard dependencies on database tables is to provide an in-memory implementation and register that as a replacement in DUTB::containerBuild(). That's how we're handling core services already.
2. For modules, we can do that in the module's Bundle::build() method, based on the Kernel's environment name (i.e., specifically looking for 'testing'). If we wanted to make that more explicit, then we could register TestBundle last and make it loop over all other register bundles to invoke a special buildTest() method.
3. In any case, I don't think we ever want to remove service definitions from the testing container.

#1331486: Move module_invoke_*() and friends to an Extensions class landed, so we're back in business here.

Attached patch is more or less just a re-roll from scratch against HEAD, since the module_handler patch changed a lot in DUTB. I didn't update existing calls to enableModules() yet.

Also doesn't contain any changes for #12.2 / #13.2 yet. I wonder whether we could move that to a follow-up issue?

Something is really broken with d.o issue tags since very recently. No idea why the new tag is shown as added, but wasn't actually added.

Attached patch includes the necessary DUTB test changes throughout core. Luckily, manually running all of those tests didn't take much time :)

+++ b/core/modules/edit/lib/Drupal/edit/Tests/EditTestBase.phpundefined
@@ -29,7 +29,9 @@ function setUp() {
-    $this->enableModules(array('field', 'field_sql_storage', 'field_test'));
+    $this->installSchema('field', 'field_config');
+    $this->installSchema('field', 'field_config_instance');
+    $this->installSchema('field_test', 'test_entity');

One downside of doing this is that you need to know way more about the internals of that module than you ever wanted.

For example, my entity tests failed because the role table was removed and I tried to create it.

Just like this will fail once fields are converted to CMI.

And entity_test defines *a lot* of tables for multiple entity types, revisions, data tables and so on.

Maybe we can make the second argument optional (= all tables) or at least support an array of tables?

+++ b/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.php
@@ -184,39 +200,52 @@ protected function installSchema($module, $table) {
-      module_enable($modules, FALSE);
+  protected function enableModules(array $modules) {

Wrong method name. enableModules() has confused me before, because it usually means module_enable() and that the module will be fully installed. That is not the case at all here. Better use "loadModules" or "includeModules".

Same problem with the static $module property: in WebTestBase it means that this array of modules will be installed and enabled. That is also wrong here, so the property should be named $loadModules or $includeModules or whatever better name you can come up with.

Just a thought though, I don't want to hold up this issue on naming conventions.

Yes, I considered that, but decided it wouldn't be a good idea, as it conflicts with the basic paradigms that have been outlined in this issue.

Allowing to install all defined tables of a module schema is almost identical to calling module_enable(). And that's what we want to get rid of here.

Also, if it was a pure unit test, then you'd have to create individual schema tables on your own, too.

My perspective is that a (Drupal) unit test should know exactly what it needs and be explicit in its entire setup. Any kind of wildcards only add an unknown amount of arbitrary test dependencies. In turn, the test does not actually know anymore what exact functionality it tests.

Based on that, I'm not really sure whether it is a good idea to have the installConfig() method to install a module's default configuration... but I guess we can move on with that for now.

What larger sets of tests can always do is to introduce a base test class that sets up shared dependencies. We're doing that for the most part already, which is why there's only a difference of 10KB between #14 and #17.

This should get almost all of the remaining failures done.

However, I wasn't able to figure out the actual dependencies of Drupal\views\Tests\Handler\RelationshipTest.

Also, Drupal\views\Tests\Handler\FilterStringTest triggers a "Too many connections" error when running the test locally on my machine. Adding the 'persist' tag to the database service did not help — most probably, because every new test method sets up a completely new DrupalKernel, and thus, for every new test method, a new database connection is created. We likely need to intentionally leak the connection from the test runner into the test in setUp(). Or alternatively, we need to ensure that the kernel is properly shut down, including all services, and including the test database connection. I'll probably create a separate issue for that.

That's something we need to fix anyway in the database in DIC issue. The idea there was to a) persist (although we didn't have a word for that back then) and b) add support for changing the prefix without having to create a new connection.

+++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterDefaultConfigTest.phpundefined
@@ -26,18 +26,22 @@ public static function getInfo() {
+    $this->installSchema('user', 'users_roles');
+    $this->installSchema('user', 'role_permission');
+    $this->installConfig(array('user'));

Any reason installSchema couldn't take an array of tables? This hunk would look more sane as

$this->installSchema('user', array('users_roles', 'role_permission'));
$this->installConfig(array('user'));

+++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterDefaultConfigTest.phpundefined
@@ -26,18 +26,22 @@ public static function getInfo() {
+    $this->installConfig(array('user'));
+
+    // Install filter_test module, which ships with custom default format.
+    $this->installConfig(array('filter_test'));

And the two of these could be combined

+++ b/core/modules/views/lib/Drupal/views/Tests/Handler/FilterNumericTest.phpundefined
@@ -14,6 +14,8 @@
+  public static $modules = array('system');

+++ b/core/modules/views/lib/Drupal/views/Tests/Handler/FilterStringTest.phpundefined
@@ -14,6 +14,8 @@
+  public static $modules = array('system');

+++ b/core/modules/views/lib/Drupal/views/Tests/Handler/RelationshipTest.phpundefined
@@ -16,6 +16,8 @@
+  public static $modules = array('field', 'user');

+++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/StyleMappingTest.phpundefined
@@ -12,6 +12,8 @@
+  public static $modules = array('system');

Missing docblock here and several other places.

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewUnitTestBase.phpundefined
@@ -31,7 +33,14 @@ protected function setUp() {
+  protected function setUpFixtures() {

Is this an existing method? I couldn't find another. Can we get a docblock?

Incorporated most of the review, installSchema() supports multiple table names now. For setUpFixtures(), I copied the docs from ViewTestBase::enableTestModule() for now. I'm playing with the idea of making setUpFixtures() an official method on DUTB, but will defer that to a separate issue, since I want to double-check how this method is named and how it works in PHPUnit first.

I deny to add repetitive docs for $modules; the property and its behavior is documented on the base test class already.

Finally figured out what's going on in that Views RelationshipTest. Note that #1893850: Cleanup relationship tests and don't use the node module (mistakenly?) converted it into a WebTest, since Views\PluginTestBase extends WebTestBase, not DrupalUnitTestBase. Since it was a unit test before, I corrected that by introducing a new Views\PluginUnitTestBase in this patch. It's possible that we want to kill the web-based PluginTestBase entirely, since plugins ought to be unit-testable, but that's left for a separate issue and VDC to figure out. :)

Fixed the parse error.

Re-rolled, fixed the reported test fails and updated the environment to unit_testing.

There will very likely be new fails as new tests have been added. Will try to do another re-roll to fix those too. Let's try to get this in ASAP, this patch will get bigger and bigger as more tests are added.

That should fix the remaining failures. I don't seem to get those role_permission related ones locally, maybe because I used the UI?

Whatever we do, this creates an environment which is fake and not in a good way; these tests are not testing any real Drupal behavior.

I would say that, to a certain degree, creating fake/mocked environments is the point of unit tests. Yes, it might not be able to cover some edge cases but a lot of code should work/be testable in a mocked/fake environment just fine. And for the rest (of which we still have a lot), we have integration tests. Care to clarify "not in a good way"?

I am not convinced about $moduleList/addtoModuleList(). It doesn't tell me what it does/how it is different from $modules, all it does is tell me it's a list of modules... which $modules is too.

Reading the documentation of that property in DrupalUnitTestBase tells me exactly what it is and how it differs from UnitTestBase and WebTestBase. As it says there, it only loads modules and does not enable/install them. So... what about loadModules()? Not sure the property should be named $loadedModules, $modules is IMHO fine as well.

This looks great to me, thanks.

I think that #36 circles back into #1 - DUTB tests are as "real" as you (manually) make them, just like any other unit test. Again, if your test depends on hook_install() to be called and then you should ask yourself whether it is legit and appropriate to use a DUTB test. No one forces you to write a DUTB test.

As written in #35, I'm wondering if enableModules() should be renamed to loadModules(), including improved documentation of that function. I think that would match nicely with the documentation that is currently on the modules property which says that it only loads and doesn't really install/enable modules.

@chx: I understand your concern. However, so far, DUBT proves to be very useful, especially when running tests locally and it also helps to get testbot execution time down (well, not really down, just helps to balance the slower test execution and ever increasing amount of tests). While real unit tests would be preferable, everything that is not pure OOP code and invokes hooks, calls functions, uses plugins can currently not be a real unit test but DUBT is a step in the right direction IMHO and helps to point out hidden dependencies which should make the conversion to real unit tests/phpUnit easier.

Leaving at RTBC, maybe someone can assign to @catch so that he can have a look at this. Would love to get this in so that I can continue converting stuff to DUBT.

re: #38:
Well, enableModules() is slightly more than just loading. As documented in its phpDoc, the critical piece is actually that the modules are added to the DrupalKernela and to the ModuleHandler, which in turn means that the module/bundle's services are available and also hook implementations are found and invoked. As such, loadModules() would be misleading. We could investigate somewhat more appropriate registerModules() and unregisterModules(), but since that's a simple rename, I'd prefer to defer that to a follow-up.

#33: dubt-tests-install-1891516-33.patch queued for re-testing.

Looks good to me. There's going to be some oddities with DrupalUnitTestBase but if anything this makes them more explicit. Committed/pushed to 8.x.

Needs a change notice.

I've updated the existing change notice to remove $install related examples, provide an installSchema() example with multiple tables and also added installConfig().

http://drupal.org/node/1829160

I think that's enough...