Use exceptions when something goes wrong in test setup

Cool, this should take care of #2049967: WebTestBase massacre when required modules cannot be enabled too, I think?

Note that I'm moving/consolidating the main test preparation/setup code into a new TestBase::setUp() over in #2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead.

1. +++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
   @@ -780,31 +787,50 @@ public function run(array $methods = array()) {
   +            $this->setUp();
   +            if (!$this->setup) {
   +              throw new TestSetUpException();
                }
   

   The boolean $this->setup flag/property is also obsolete when throwing exceptions, isn't it?

2. +++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
   @@ -780,31 +787,50 @@ public function run(array $methods = array()) {
   +          catch (TestSetUpException $e) {
   ...
   +          catch (TestTearDownException $e) {
   

   I'm not really sure whether it makes any sense to have dedicated exception classes for setUp and tearDown.

   1. Whenever an exception is thrown, the executed test class method is aborted.

   2. If an exception is thrown during setUp(), then the entire test class must be skipped, because none of the methods will succeed.

   But I'm not able to see why we need specialized RuntimeExceptions for that?

   Just catching any kind of exceptions in the different stages is completely sufficient:

   try {
     $this->setUp();
   }
   catch (\Exception $e) {
     $this->exceptionHandler($e);
     // Abort this entire test class.
     continue 2;
   }
   try {
     $this->$method();
   }
   catch (\Exception $e) {
     $this->exceptionHandler($e);
     // Proceed with tearDown().
   }
   try {
     $this->tearDown();
   }
   catch (\Exception $e) {
     $this->exceptionHandler($e);
     // Signify that not all test artifacts could be cleaned up.
   }
   

3. +++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
   @@ -780,31 +787,50 @@ public function run(array $methods = array()) {
   +            $message = $e->getMessage() ?: t("The test cannot be executed because it has not been set up properly.");
   

   If the thrown exception has no message, can we compose and use a more informative test assertion failure message?

   How about simply casting $exception into a string?

   $message = (string) $e;

   → http://php.net/manual/en/exception.tostring.php

   That would give a developer a much better idea of what went wrong.

   In fact, let's always use that instead of manually futzing with $e->getMessage()?

   ---

   That said, the old code used TestBase::exceptionHandler(), which contains the proper logic for logging an exception already — Why are you not using that?

4. +++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
   @@ -953,10 +973,18 @@ protected function prepareEnvironment() {
   +    if (!file_prepare_directory($this->public_files_directory, FILE_CREATE_DIRECTORY | FILE_MODIFY_PERMISSIONS)) {
   +      throw new TestSetUpException('Could not prepare the public files directory of the test site.');
   

   Here and elsewhere:

   I'd prefer to have (1) shorter and (2) more informative exception messages:

   "Failed to create public files directory '$this->public_files_directory'."

Please note that I had to refactor the exact same code for the exact same purpose and intentions in #2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead now, in order to account for interactive installer tests.

The interdiff in #37 refactors the entire environment preparation procedures of individual setUp() methods into a single and final (i.e. non-overridable) TestBase::prepareEnvironment() method.

And instead of "hoping" that someone will call it, TestBase::run() force-executes it on its own.

At the same time, that approach kills all of the utterly fragile $setupWhatnot state flags and replaces them with \RuntimeExceptions everywhere.

#2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead is an original spin-off from #1757536: Move settings.php to /settings directory, fold sites.php into settings.php and presents a blocking dependency for that. Given that this issue was seemingly spawned from the same parent issue, I'm not sure whether it makes sense to independently proceed here? — #2171683 gained major interest and lots of love from everyone in the meantime and the hope is that it will land very soon.

All of that being said, I wouldn't actually mind if this change would land upfront. But if possible in any way, it would be great and helpful if we'd copy/paste all of the relevant bits from #2171683 into this patch here.

Just wondering whether it would be possible to extract the test environment setup changes from #2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead as-is...

That said, those changes are strictly required for #2171683. By splitting them into this issue, that issue will be blocked and delayed further...

Time is running out to work on the top-level /settings.php issue (which definitely must happen before beta). So even if this test will happen to pass, I'm not a fan of the split.

The errors reported by the testbot are always different ones. Sorry for the noise.

Well spotted, @tstoeckler! That was the missing piece. :)

I was very tempted to simply delete the BrokenSetUpTest (because it seemingly no longer applies), but after inspecting the root cause for the test failures (_simpletest_batch_operation() no longer having a ModuleHandler after test completion), I realized that TestBase::tearDown() is still too fragile.

To fix that, TestBase::tearDown() is turned into an empty method, and the original code now lives in a dedicated + private TestBase::restoreEnvironment() method.

Like TestBase::prepareEnvironment(), TestBase::restoreEnvironment() is also called only from TestBase::run() in a controlled fashion.

Lastly:

I adjusted BrokenSetUpTest accordingly. But I actually do not think that this kind of data sharing should be possible.

In fact, it really concerns me that all of the $this->original* properties are accessible to tests. In a separate follow-up issue, we should convert all of them into a single + private TestBase::$original property (array/object) that holds all of the original data, NOT accessible to tests.

For now, I guess I'd be fine with keeping the adjusted test, but ultimately, I think we'll have to delete that test when making the original properties private.

Sorry, the changes to the Error class were not supposed to be contained.

Created #2177079: Test classes have access to TestBase::$original* properties for #27

Yay, we're green! :-)

Minor correction:

I grepped and those are the only occurences of $conf['simpletest_parent_profile'] in core currently, so this is obsolete already.

The configuration value still exists and is still set by WebTestBase::setUp(), but it lives in the simpletest.settings config object now. Only this adjustment of a top-level global $conf key is obsolete/unused.

FWIW, further discussion on that happens in #2175391: ExtensionDiscovery::getProfileDirectories() explicitly skips parent installation profile in installer, defeating its purpose

Tagging and bumping to major, because at least two other major issues depend on this change.

Updated issue summary to reflect the final proposed solution.

28: drupal8.test-env-setup.28.patch queued for re-testing.

Looks like good clean-up and I couldn't find any complaints. Committed/pushed to 8.x, thanks!

This broke http://wearepropeople.com/blog/debugging-tests-from-console-using-phpsto... please roll back.

I have no idea how to fix this but if you can, go ahead, please file a followup. Note that if phpstorm is accepting xdebug connections then --class simply hangs and never finishes. The same happens to drush si except with drush si if you switch off the xdebug connections it continues but the run tests script hangs hopelessly.

All in all, I believe the best would be to make --execute-test work again as that's urgent (not for me -- I reverted this in the IMP sandbox to be able to work) and in a separate issue explore how to make --class and xdebug work.

As far as I can see, that tutorial rightfully does not work anymore — the executed command line lacks a --test-id.

The lack of a properly prepared $test_id should have aborted test execution in the past already, because test assertion results are recorded and stored for a bogus/non-existing test run ID. The $test_id defaults to 0, so I can only assume that the results of all of those executions were cumulated and mixed into a single test ID.

Thus, that tutorial gives wrong/incomplete advice.

Funnily/coincidentally, I was skeptical whether the validation of updating the test_id with the new database prefix in changeDatabasePrefix() would ever be triggered, but in light of that incomplete/wrong usage of run-tests.sh, I'm glad that the validation successfully kicked in. :)

@tstoeckler is right in that #2134259: Make the Simpletest XDebug integration work for CLI requests and other issues improved the xdebug integration for tests.

However, I agree that the situation can be improved further, so I created:

#2183315: Improve XDebug integration for tests and run-tests.sh

FYI, I did a quick test and while it didn't hang (not sure what you mean with that), it simply never got to my breakpoint in the test class but run the test successfully.

Edit: nevermind. Clearly there's something broken in my setup. Debugging.

I filed a bug report http://youtrack.jetbrains.com/issue/WI-21929

The problem is solved by raising Max. simulatenous connections as described in http://youtrack.jetbrains.com/issue/WI-8440

It's possible that some of this (at least the try/catch around the setUp() and tearDown() methods) needs to be backported to Drupal 7. #2763435: Exceptions during the setUp() or tearDown() method of a test are not handled is an issue for that.