WebTestBase massacre when required modules cannot be enabled

While at this, any chance of displaying which module is actually missing? Due to dependencies of dependencies of...., that's sometimes harder to figure out than you'd think.

So this looks pretty cool, but:
1. I don't really understand all of the changes that are not in WebTestBase.
2. I think we should provide tests for this behavior.
3. #5 would actually be pretty cool. Would that mean we could pass FALSE to $moduleHandler->install().

Also, shameless plug: #2170023: Use exceptions when something goes wrong in test setup

Re #21: I now looked into this in detail and, yes, those changes make a lot of sense!

What I meant with the ModuleHandler part is that it would be cool if we would be able to report which modules could not be installed. You're right that that should not be done in this issue.

I still think this issue should be blocked on providing test coverage for this. I realize it's non-trivial currently but with all the non-trivial an subtle bugs that occur in the testing framework IMO it is essential that we test everything we can.

The recent patches in #2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead make some changes to the testing system which would make testing this significantly easier, which I intend to extract into #2170023: Use exceptions when something goes wrong in test setup. At that point we can either incorporate this issue over there or this issue could be postponed on that one as it will be significantly easier afterwards (including test coverage).

Edit: Forgot the word 'not' :-)

@Berdir:

While at this, any chance of displaying which module is actually missing? Due to dependencies of dependencies of...., that's sometimes harder to figure out than you'd think.

+1 — and after refactoring, that's actually a DUTB test. ;)

@Xano:

Maybe we should, but the current test set-up is unstructured and due to the nature of the testing system testing the testing system, we would have to create a completely new web test case…

Yes, that is a major problem of our current WebTestBase architecture, especially concerning Simpletest tests (testing the testing framework). — Simpletest is a very expensive module to install, because it performs massive filesystem scans in order to prime its static caches.

In other words, the desire for tests for this issue is the most excellent use-case for:

#1411074: Add a flag to set up test environment only once per test class; introduce setUpBeforeClass() and tearDownAfterClass() [PHPUnit]

(which, uhuh, could apparently get trivial after #2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead has landed)

@OP:

WebTestBase massacre when required modules cannot be enabled

Before reading comments in here, I literally interpreted this title to mean the formal notion of 'required' modules — which actually is a thing in Drupal (that we should get rid of):

#1688162: Replace required flag of modules with proper dependencies

WebTestBase happily goes ahead and causes a massacre of exceptions and failures if a test's module dependencies cannot be enabled. Let's make test results a little less bloody by simply aborting the test.

#2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead (or if necessary #2170023: Use exceptions when something goes wrong in test setup) will fix that by wrapping the invocation of setUp() into a try/catch block. Primarily due to other architectural reasons, so the "bug" specifically expressed here won't be covered by tests.

I'm saying "bug" and not bug, because IMO there is a (yet undefined) upper limit/extreme of possible test cases that could theoretically be accounted for in tests. As of now, that limit is not defined for Drupal core, but we do make many exceptions in the form of "not worth to test" already.

Especially as this concerns a "bug" that is developer-facing only and actually has to be en-coded by a developer first, I personally believe that case exceeds the upper limit/extreme.

However.

What's much more concerning in light of this issue here is this:

1. drupal_install_system() manually installs User module, without removing it from the list of to be installed modules of the installation profile. But yet, installation succeeds. WTF?
2. WebTestBase::setUp() removes duplicates from the list of to be installed $modules, but then it calls ModuleHandler::install($modules, FALSE); i.e., skipping dependency resolution. Almost all tests do not supply a completely resolved list of dependencies. But yet, that operation succeeds. WTF?
3. Likewise, WebTestBase::setUp() executes the non-interactive installer to (1) install required modules and (2) the chosen installation profile. Aforementioned setUp() step most often tells ModuleHandler::install() to install the exact same modules again. But yet, installation succeeds. WTF?

Long story short:

This issue complains about a lack of error reporting in case dependencies cannot be installed in case of errors.

But reality is, close to none of our existing tests should be able to install in the first place. Almost all of them are seemingly installing modules over already installed modules. That is a fundamental lack of error handling (in the ordinary, not-yet-error case).

While the end of this story may appear unrelated to you, fact is that you can only prevent a massacre if the massacre is properly caught in the first place.

@sun: I think you're assumption is wrong. ModuleHandler::install() currently contains the following code.

      // Only process currently uninstalled modules.
      $installed_modules = $module_config->get('enabled') ?: array();
      if (!$module_list = array_diff_key($module_list, $installed_modules)) {
        // Nothing to do. All modules already installed.
        return TRUE;
      }

So it's totally fine to call install() with a bunch of modules, that are already installed. Not sure that's a good thing but if we want to change that that should be a separate issue.

Here's an updated patch after the TestBase refactoring. I left in the assertion about the installed modules, as that might be useful, although that's arguable. I'm not tied to it in any way.

Also I removed a whole bunch of code that was checking the test IDs, as those are no longer generated in the first place. That was introduced as a regression test for #290316: Simpletest test_id assignment broken (critical on PostgreSQL), though, so I'm not sure what to make of that. I
*think* it's safe to remove it anyway, but not sure.

No interdiff as I started from scratch.

Thanks for the pointer/clarification, I wasn't aware of the already-installed check in install(). :)

I removed a whole bunch of code that was checking the test IDs, as those are no longer generated in the first place.

You actually removed some crucial Simpletest assertion assertion tests ;)

I'm not sure why you think that test IDs would be no longer generated, they definitely are still generated.

However, as a potential follow-up issue to #2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead, I was considering to remove the test ID entirely in favor of using the (database) prefix only.

Sorry, I was imprecise above. It might be that the test IDs are still generated (if you say so, that certainly works for me :-)) but as can be seen from the patch the way that the test IDs are currently fetched is from the simpletest results page. Because the test does not get set up properly, though, the test ID no longer shows up there. That's why I removed that code. If you know some magic way to figure out the test ID, we can certainly reintroduce that code. Another possibility would be maybe to move this to a separate test, which *does* perform a full setup?! Help appreciated :-)

Well, the functionality you're testing here rather belongs into BrokenSetupTest instead of SimpleTestTest in the first place?

If you think there is a hidden bug in SimpleTestTest, then I think that should be a separate issue. This issue should not touch it.

However, since BrokenSetupTest might be removed soon in #2177079: Test classes have access to TestBase::$original* properties, I wouldn't mind to add a completely new test class for testing the unknown module exception here.

That said, I also wouldn't mind if we wouldn't touch any simpletest tests at all for this minor DX adjustment.

Well the tests currently specifically check for the assertions that are shown in the results page for a test that enables a missing module. So that part has to change either way. I can certainly extract that into a separate class, i.e. MissingModuleDependencyTest or similar, but that doesn't change any of the fundamentals. That also doesn't solve the test ID problem.

If you think there is a hidden bug in SimpleTestTest, then I think that should be a separate issue. This issue should not touch it.

Not sure what you mean by that. I don't think there's a bug. It's just that the current behavior (i.e. finding the test ID in the results page) does not work with this patch.

MissingModuleDependencyTest or similar, but that doesn't change any of the fundamentals. That also doesn't solve the test ID problem.

Sorry, I have troubles to understand the meaning you seem to implying. Could you elaborate on what "fundamentals" and which "test ID problem" you mean? And how's that related to this issue? :)

MissingModule[Web]Test sounds fine to me.

finding the test ID in the results page) does not work with this patch.

Yes, to keep the SimpleTestTest functional, we need to remove the conditional $modules override and the (few) lines which assert that a missing module appears as a failing assertion, because the rest of the test is currently expected to continue.

As a result of that the asserted test summary line will also change (-1 in failures).

WebTestBase now uses Drupal\Core\Tests\FunctionalTestSetupTrait::installModulesFromClassProperty().

installModulesFromClassProperty() handles MissingDependencyException to fail tests which can't install dependencies.

WebTestBase is also effectively deprecated, and tests should be converted to use BrowserTestBase.