All unit tests blow up with a fatal error

Its a bit unclear to me why so much of the webtest setup and teardown are going into the testbase.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/UnitTestBase.phpundefined
@@ -38,55 +38,43 @@ abstract class UnitTestBase extends TestBase {
+    $module_list['system']['filename'] = 'core/modules/system/system.module';
+    module_list(TRUE, FALSE, FALSE, $module_list);

Why do we need any modules in the list? Couldn't it just be empty, this is a unit test.

Also, re: the registry PDOException, I've had a patch for that sitting quietly ignored for a while. Blatant plug: #1532056: Allow the use of PSR-0 classes in unit tests

#15: drupal8.test-unit.15.patch queued for re-testing.

Ok, played around with this a bit and can confirm that executing a unit test with run-tests.sh currently completely fails. Applying the patch improves this a lot, although there are still some fails.

I grepped for all unit tests that we currently have and tried to execute them.
find . -name "*.test" | xargs grep "extends UnitTestBase"

Copied and pasted the output into the following, comma separated list:

FilterUnitTestCase,LanguageBrowserDetectionTestCase,BlockTemplateSuggestionsUnitTest,InfoFileParserTestCase,
GraphUnitTest,BatchPercentagesUnitTestCase,MenuTreeDataTestCase,SymfonyClassLoaderTestCase,UuidUnitTestCase,
UnicodeUnitTest,ThemeHtmlTag,TableSortTest,CommonXssUnitTestCase,CommonSizeUnitTestCase,
CommonAutocompleteTagsTestCase,CommonHTMLIdentifierTestCase,CommonCascadingStylesheetsUnitTestCase,
CommonValidUrlUnitTestCase,CommonValidNumberStepUnitTestCase,CommonDrupalParseInfoFileTestCase,
CommonDrupalAttributesUnitTestCase,CommonDrupalArrayUnitTest,CommonJSONUnitTestCase,BootstrapGetFilenameTestCase,
BootstrapTimerTestCase,BootstrapResettableStaticTestCase,BootstrapMiscTestCase,
BootstrapOverrideServerVariablesTestCase,ImageDimensionsScaleTestCase,UpdateCoreUnitTestCase

(This is after applying the patch)

The only one that completely fails is TableSortTest. That test uses $this->verbose() (Should IMHO be removed, but that doesn't mean that it shouldn't work), this leads to this:

exception 'PDOException' with message 'SQLSTATE[42S02]: Base table or view not found: 1146 Table 'd8.simpletest991129system' doesn't exist' in /home/berdir/Projekte/d8/core/lib/Drupal/Core/Database/Statement.php:58
Stack trace:
#0 /home/berdir/Projekte/d8/core/lib/Drupal/Core/Database/Statement.php(58): PDOStatement->execute(Array)
#1 /home/berdir/Projekte/d8/core/lib/Drupal/Core/Database/Connection.php(506): Drupal\Core\Database\Statement->execute(Array, Array)
#2 /home/berdir/Projekte/d8/core/includes/database.inc(215): Drupal\Core\Database\Connection->query('SELECT * FROM {...', Array, Array)
#3 /home/berdir/Projekte/d8/core/includes/module.inc(188): db_query('SELECT * FROM {...')
#4 /home/berdir/Projekte/d8/core/includes/module.inc(111): system_list('module_enabled')
#5 /home/berdir/Projekte/d8/core/includes/module.inc(779): module_list()
#6 /home/berdir/Projekte/d8/core/includes/module.inc(697): module_hook_info()
#7 /home/berdir/Projekte/d8/core/includes/module.inc(982): module_implements('file_url_alter')
#8 /home/berdir/Projekte/d8/core/includes/file.inc(443): drupal_alter('file_url', 'sites/default/f...')
#9 /home/berdir/Projekte/d8/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php(457): file_create_url('sites/default/f...')
#10 /home/berdir/Projekte/d8/core/modules/system/tests/tablesort.test(62): Drupal\simpletest\TestBase->verbose('$ts: <pre>array...')
#11 /home/berdir/Projekte/d8/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php(521): TableSortTest->testTableSortInit()
#12 /home/berdir/Projekte/d8/core/scripts/run-tests.sh(369): Drupal\simpletest\TestBase->run()
#13 /home/berdir/Projekte/d8/core/scripts/run-tests.sh(22): simpletest_script_run_one_test('8', 'TableSortTest')
#14 {main}FATAL CommonAutocompleteTagsTestCase: test runner returned a non-zero error code (1).

(Note that the fatal error is displaying the wrong class name, which I think is fixed in the other issue).

Not sure, but shouldn't exactly this behavior be fixed with this patch (invoking a hook)? What's happening there?

Also got the following test failures:

CSS Unit Tests 9 passes, 3 fails, and 0 exceptions
Miscellaneous bootstrap unit tests 4 passes, 1 fail, and 0 exceptions

Not sure if that's a local issue or if there is actually something wrong there.

Confirm this bug.
Steps to reproduce from UI:
1) enable testing module
2) visit admin/config/development/testing
3) try to run image_dimensions_scale() test from Image

An AJAX HTTP error occurred. HTTP Result Code: 500 Debugging information follows. Path: /batch?id=2&op=do StatusText: Service unavailable (with message) ResponseText: PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'd8.simpletest870301system' doesn't exist: SELECT * FROM {system} WHERE type = 'theme' OR (type = 'module' AND status = 1) ORDER BY weight ASC, name ASC; Array ( ) in system_list() (line 185 of /var/www/core8/core/includes/module.inc).

I was uncertain about the way the TestBase was being loaded up with WebTestBase functions but on review it makes some sense. Mostly the database setup functions but since they aren't called in UnitTestBase it doesn't really matter.

I do wonder if some of the common parent site resets should be in a TestBase::setUp method though. That would just be some logic cleanup not really related to the issue being fixed though.

One observation I did have was:

+++ b/core/modules/simpletest/lib/Drupal/simpletest/UnitTestBase.phpundefined
@@ -38,55 +38,42 @@ abstract class UnitTestBase extends TestBase {
+    $this->originalModuleList = module_list();

this

+++ b/core/modules/simpletest/lib/Drupal/simpletest/UnitTestBase.phpundefined
@@ -38,55 +38,42 @@ abstract class UnitTestBase extends TestBase {
+    module_list(TRUE, FALSE, FALSE, $this->originalModuleList);

and this are causing warnings. module_list returns a list of module names (keys) and the fixed_list argument expects some sort of formed metadata array so when we hit drupal_get_filename('module', $name, $module['filename']); the module is a string not an array and it complains about a illegal string offset.

 Exception Warning    module.inc          96 module_list()                      
    Illegal string offset 'filename'

I have nothing to add here. Just wanted to congratulate you on an awesome issue name. :)

Ok, found the problem. drupal_static_reset() also reverts the module_load_all() static to FALSE and then the call to theme() fails because it checks for exactly that. That results in an exception thrown after the test did run when you run the tests in the UI.

The attached patch adds the following below that call in tearDown(), not sure if there is a better way:

    // Restore has_run state.
    $has_run = &drupal_static('module_load_all');
    $has_run = TRUE;

This should fix the tests...

#25: unit-tests-blow-up-1563620-25.patch queued for re-testing.

#1541958: Split setUp() into specific sub-methods has been committed to 8.x. Additionally, some tests have been converted to PSR-0.

Edit: Crosspost with catch ;)

Re-roll. Shouldn't contain any actual changes.

Looks like I messed up the merge, will try again later.

Ok, the reason is that tearDown() is now in TestBase, and unit tests have no tables, so it "failed".

This patch moves that part back to WebTestBase, the alternative would be to make it conditional and ignore it silently if there are no tables.

If we move the deleting of the prefixed tables back to WebTest (which makes sense, IMO), shouldn't the whole prefix generation and database prefix mangling be left there as well?

Unit tests are supposed to use a different prefix as well, there just aren't any tables with that prefix available so a query will fail. Otherwise, you end up accessing the main site if something goes wrong in the test.

Yeah, the "databasePrefix" is a bit misleading, it gets used for more then just the database, its used for file storage and other stuff. Since we do file stuff in all tests, it is needed in the base.

Sorry about that. Let's try this again. Must have used an old patch.

Instead of changing SearchExceptTest to a web test, I noticed that the setUp() is nonsense, we can simply remove it and then it works just fine...

Also noticed that the tests in BoostrapMiscTestCase::testCheckMemoryLimit() fail if you have the memory limit set to -1. That's not something for this issue, though.

Ah I see, but doesn't this mean that it is impossible for *any* module (except those which are required) to provide unit tests? That's kinda unfortunate, isn't it? :)

just call drupal_load()?

Ok, confirmed that the search tests fail when executed while search.module is disabled. Restoring the original code now actually works, as sun pointed out. So yes, drupal_load() works.

Fixed the comment.

I tested #38 earlier today as I was experiencing lots of errors everytime I ran a full test on 8.x core. Patch cleared them right up.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/UnitTestBase.phpundefined
@@ -38,55 +38,40 @@ abstract class UnitTestBase extends TestBase {
+    // Re-implant theme registry.
+    // Required for l() and other functions to work correctly and not trigger
+    // database lookups.
+    $theme_get_registry = &drupal_static('theme_get_registry');

Where does l() get called during unit tests? Verbose calls that affect the parent site or similar? I think we need more explanation why we're restoring this stuff for the unit tests, when unit tests shouldn't have access to the db anyway.

With the search test, it should really be a full functional test, but since we don't need to touch that code here let's not.

Otherwise looks great.

Out of interest double checked whether this conflicts with the kernel patch, and it doesn't, so they should both be good to go in next IMO.

Discussed that question with berdir in irc. Short version is I opened a follow-up issue that contains the answer, don't want to hold this patch up on this patch up on anything since we have zero unit test coverage due to this bug at the moment, so all kinds of things can silently break. See #1611430: Verbose debug output in unit tests relies on the database.

Committed/pushed to 8.x, moving to 7.x for backport.

I looked at backporting this patch but module_list function differs between D7 and D8. Someone will have to do it :)

I also looked at backporting this but I am totally stumped -- how does the module list get reset after the test run? I believe the 8.x patch committed is bogus. That said I am attaching my work so far, it is not complete at all. Did I mention that we need to mark the methods with which class they belong to? Moving parts was nuts without it cos I obviously had no idea which setUp / tearDown I was looking at any minute. I will never stop repeating this over and over. I doubt this patch passes. I am even unsure why this needs backport did anyone verify the bug exists in D7?

8.x now uses drupal_static() for the static variables in module_list(). This means it's reset during the generic drupal_static_reset() executed in setUp().

I have no idea how this can be backported...

I was incorporating that into this, not knowing it's a separate issue. Given the code that needs to moved what about closing it as duplicate and finishing this?

I can see some small differences between the D7 and D8 patch. Not sure they are intentional. Listed some examples.
Lot of internal knowledge is needed to mark this rtbc (or just trst sun ;) )

+++ b/modules/simpletest/drupal_web_test_case.phpundefined
@@ -521,6 +521,186 @@ abstract class DrupalTestCase {
+    $this->setupDatabasePrefix = TRUE;

Can't find this in the d8 patch

+++ b/modules/simpletest/drupal_web_test_case.phpundefined
@@ -521,6 +521,186 @@ abstract class DrupalTestCase {
+    $this->originalUser = $user;

Missing in the D8 version

+++ b/modules/simpletest/drupal_web_test_case.phpundefined
@@ -521,6 +521,186 @@ abstract class DrupalTestCase {
+    $this->setupEnvironment = TRUE;

same as above

-29 days to next Drupal core point release.

Still applies, and looks good.

Yikes! This is a big patch. I really think for 7.x, this could use final sign-off by David. Assigning to him.

Yikes, it is big, but after #1541958: Split setUp() into specific sub-methods we are on a roll here :) And to a large extent, it's big because it's moving code around.

I did find a couple issues:

1. +    // Restore has_run state.
   +    $has_run = &drupal_static('module_load_all');
   +    $has_run = TRUE;
   ....
   +    // Prevent module_load_all() from attempting to refresh it.
   +    $has_run = &drupal_static('module_load_all');
   +    $has_run = TRUE;
   

   In Drupal 7, module_load_all() isn't using drupal_static() currently, so these can't be working as intended.

2. +    // Ensure that TestBase::changeDatabasePrefix() has run and TestBase::$setup
   +    // was not tricked into TRUE, since the following code would delete the
   +    // entire parent site otherwise.
   +    if (!$this->setupDatabasePrefix) {
   +      return FALSE;
        }
   

   I think we don't want this in Drupal 7; see #1541958-80: Split setUp() into specific sub-methods for why. (And I believe http://drupal.org/project/simpletest_clone is a real-life example of something this might break.)

3. +    // All static variables need to be reset before the database prefix is
   +    // changed, since Drupal\Core\Utility\CacheArray implementations attempt to
   +    // write back to persistent caches when they are destructed.
   

   (minor) The code comment here refers to a Drupal 8 class name.

4. Finally, I am curious why the base-level DrupalTestCase class is getting a tearDown() method when it doesn't define a setUp() method... That seems unusual especially because tearDown() expects things to be set up in a particular way. I guess the only place that could cause issues is if a custom or contrib test class expected no tear down to take place at all, which seems unlikely, but still wondering if there are any issues there.

   For Drupal 7, would it be safer to rename this top-level tearDown() (which is essentially a helper function anyway) to something else, e.g. standardTearDown(), and then explicitly call it from the child classes?

Needs issue summary

If this is back to normal priority, and given that no one seems to be working on getting it done, can we remove the "avoid commit conflicts" tag?

I am tentatively removing the "avoid commit conflicts" tag on this issue, since there has not been a new patch since the end of July and I'm not seeing evidence of current work on a new patch. It doesn't seem to comply with:
http://drupal.org/node/1207020#avoid-commit-conflicts

If I'm wrong, feel free to put the tag back. :)

IMO "Avoid commit conflicts" should only apply to patches that are RTBC (and possibly needs review). Needs work needs work anyway, so no reason to avoid conflicting with it. :)

I am a bit confused whether this is the correct issue.
On a D7 site, I am running into the exact problem that neclimdul describes in #19:

module_list returns a list of module names (keys) and the fixed_list argument expects some sort of formed metadata array so when we hit drupal_get_filename('module', $name, $module['filename']); the module is a string not an array and it complains about a illegal string offset.

The code is still there apparently:
http://drupalcode.org/project/drupal.git/blob/refs/heads/7.x:/modules/si...

This seems to be a serious bug, but I don't understand why it pops up just now, and has not happened to me before.
I tried some git blame (in the repo browser on d.o.), but after that I am only more confused.

Re #68 (no-backport policy),
that would mean we have to start a new issue for this D7 bug?
I have no idea whether this should be "needs work" or "needs backport".

#69
This particular problem can be solved with a quite simple patch.
Because DrupalUnitTestCase does only temporarily unset a module in the list, and only add one that already has its drupal_get_filename() set, the drupal_get_filename() can be skipped.

The attached patch is not the most beautiful solution, but maybe in this case we should try the most minimal patch possible.
I also attach an ANTI patch with no effective changes, which I am very curious if it will blow up or not.

My ANTI patch is nonsense. I wish I could stop the testbot on the previous post :/

Probably the reason this is all green is because locale module is not enabled on the testbot site, whereas it is enabled at my local test install.

So we would need to add a web test in simpletest module, that enables locale and then tests a unit test.

#71: drupal-7.x-module_list-DrupalUnitTestCase-1563620-71.patch queued for re-testing.

I agree this is not the most elegant solution as I dislike test hocks or more generally testing affecting the production code, but it has the merit to remove this warning which pollutes the whole test suit.

Works for me.

agree with #74 except that maybe we could have a better solution for D7 as error come from simpletest and not core ?

Correct me if i'm wrong but here is an another approach to fix this issue only for D7 (sorry no time for D8).

We load module enabled from current profile before switching from database prefix as system table doesn't exists and doesn't seems to have any effect. Then format list, exclude locale module and store list including filename even if not used.
Then, tearDown() method will reset original module list including locale module.

Sorry to change issue status for D7 but curious to see if it could solve this issue in a better way.

Works for me

I was not able to reproduce this bug until I eventually realized it's probably PHP >5.4 only (PHP 5.3 doesn't throw any notices or errors on drupal_get_filename('module', $name, $module['filename']) when $module is a string).

But it seems like we shouldn't have to force $fixed_list to be used every single time like the patch is doing. (There are also minor grammar issues in the code comment, and $infos should be something like $info instead.)

Can't we just do something like the attached?

RTBC for me, thanks

Works for me, too.

Updated issue summary.

I have experienced the same problem with Simpletest in D6. If you are looking for a Drupal 6 patch, I posted a version of David_Rothstein's patch at #77 at #2149929: Illegal string offset 'filename'.

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/0b1196b

Guess we should restore this issue to its original Drupal 8 status (since most of the original patch here was never backported to Drupal 7).