Problem/Motivation
In both D7 and D8, DrupalUnitTestCase::setUp() attempted to access module_list() after switching the database prefix (to ensure that no function in a unit test can work with any data from the environment executing the test), but this blew up with a fatal error, since class Database is not found.
This made it difficult to run any unit tests. 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
Proposed resolution
sun created a patch in comment #54 that works for D8. This checks for an empty DB prefix, and if a prefix exists, it clones the current connection and replaces the current prefix.
Remaining tasks
The issue has been committed to D8. No further work is planned on this issue for D7. sun wrote in issue 68:
Getting close to D8 feature freeze, and facing a heavily diverged simpletest framework between D8 and D7, it's close to impossible to retain 'similarity' between major core versions, and doing so involves a huge risk of breaking things badly in D7. Therefore, I'm calling out the end to testing framework backports
User interface changes
N/A.
API changes
N/A.
Original report by sun
DrupalUnitTestCase::setUp() attempts to access module_list() after switching the database prefix (to ensure that no function in a unit test can work with any data from the environment executing the test), but this blows up with a fatal error, since class Database is not found.
In turn, this means that we likely do not run any unit tests for quite some time...
Comments
Comment #1
sunPatch in #0 makes DrupalUnitTestCases run via run-tests.sh again.
However, when running via the SimpleTest UI:
That's likely triggered by calls to t() in assert*() functions. Possibly caused by one or both of these:
#1213536: Non-resettable theme_get_registry() cache causes problems for non-interactive installations
#1404198: Separate database cache clearing from static cache clearing and data structure rebuilding
Comment #3
sunok folks, I've no idea what happened here. In addition to these critical bugs, we have a major performance regression for unit tests. run-tests.sh needs 4 seconds for a single one :(
FWIW, attached patch seems to resolve #1. I'm moving the new methods to prepare and change the test environment into the DrupalTestCase base class, since both unit and integration tests need to perform the identical operations.
Comment #5
sunSearchExcerptTestCase cannot be a unit test, because search_excerpt() calls into helper functions which try to invoke module hooks.
Comment #6
sunPlease consider this patch as a major stop-gap fix.
Even with this patch, the error and exception handlers are not fully working correctly. For example:
In #1445224-20: Add new HTML5 FAPI element: color, the unit test calls into a static method, which in turn may throw an InvalidArgumentException. I didn't specify the namespace root or a use statement for the exception at first; i.e.:
This triggered the class autoloader(s), including the registry lookup. Since unit tests run in an isolated environment and (purposively) don't have a database, that throws an PDOException, because {registry} does not exist.
So what's potentially still missing in this patch is to unregister the registry classloader in DrupalUnitTestCase::setUp() and restore it in tearDown().
(If this patch was for D7, then we'd rather want to change and enhance the database connection prefix, so that unit tests can still look up classes in the unprefixed {registry}. But since this is for D8, and because we want to kill the registry for D8, that makes no sense.)
That said, that kind of exception should be captured - at least it was exposed in my test runs - to some extent. The actual exception failure you get in the test log is not helpful at all, since it only shows the PDOException, but not the actual/initial exception that triggered the follow-up exception. This likely means that Simpletest's exception handler is not parsing the thrown exception correctly. Debugging the test exception handler can be a separate issue.
Alas, we need to rethink unit tests entirely (in separate follow-up issues). Most of this is ultimately caused by the fact that SimpleTest tries to log all test assertions into a database table that is expected to exist. In turn, there has to be a fully working parent site environment, which at minimum is able to execute database queries and potentially also lookup and load classes - even for unit tests. I'll post a meta issue with a plan and roadmap for a total testing system revamp as soon as time permits.
Comment #7
sunThis patch blocks #1565718: Script test runner does not clean after fatal error now.
Comment #8
sun#1444620: Remove file signing from configuration system and #1541958: Split setUp() into specific sub-methods broke this patch.
Comment #9
sunIntroduced $this->originalConf to simplify tearDown() and heavily improved the docs/comments.
Beware for future re-rolls: The new DrupalTestCase::tearDown() cannot be simply copypasted from the original anymore.
Comment #10
sunRedone from scratch due to #1541676: Convert Simpletest base test classes to PSR-0
Comment #12
sunSorry, misnamed variable.
Comment #13
neclimdulIts a bit unclear to me why so much of the webtest setup and teardown are going into the testbase.
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
Comment #14
sunThe problem with module_list() is that it doesn't accept a fully empty list as $fixed_list. Thus, I had to put something in there, and went with system.module.
We could change the signature/logic in module_list(), but that wouldn't be backportable (and I believe that we should/must backport these fixes).
Comment #15
sunLooks like we can make module_list() support an entirely empty $fixed_list, and that still looks backportable to me.
Comment #16
Berdir#15: drupal8.test-unit.15.patch queued for re-testing.
Comment #17
BerdirOk, 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:
(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:
(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:
Not sure if that's a local issue or if there is actually something wrong there.
Comment #18
andypostConfirm 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 fromImage
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).
Comment #19
neclimdulI 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:
this
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.Comment #20
sunThanks for the idea @Berdir - enhanced your shell snippet into a fully working command: ;)
Attached patch fixes all issues.
Comment #22
sun#20: drupal8.test-unit.20.patch queued for re-testing.
Comment #24
chriscohen CreditAttribution: chriscohen commentedI have nothing to add here. Just wanted to congratulate you on an awesome issue name. :)
Comment #25
BerdirOk, 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:
This should fix the tests...
Comment #26
sunThanks. Looks good.
However, this patch is now blocked on the follow-up fix in #1541958: Split setUp() into specific sub-methods and needs to be re-rolled (with conflict) afterwards.
Comment #27
catch#25: unit-tests-blow-up-1563620-25.patch queued for re-testing.
Comment #28
Niklas Fiekas CreditAttribution: Niklas Fiekas commented#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 ;)
Comment #30
BerdirRe-roll. Shouldn't contain any actual changes.
Comment #32
BerdirLooks like I messed up the merge, will try again later.
Comment #33
BerdirOk, 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.
Comment #34
tstoecklerIf 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?
Comment #35
BerdirUnit 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.
Comment #36
neclimdulYeah, 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.
Comment #37
sunHrm. I compared the latest patch to my last patch in #20, and somehow, we've lost a couple of things along the way. Between #25 and #30, the patch suddenly re-started based on earlier code.
The empty() needs to be changed into an isset(), or otherwise, any call to module_list() will reload and refresh the module list.
This was previously replaced with a drupal_static_reset() in TestBase::tearDown().
Keeping the removal of prefixed tables in WebTestBase is definitely correct.
Comment #38
BerdirSorry 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.
Comment #39
sunIf the test works with this removal, then it only works, because Search module is enabled in the parent site that is running the test (which means that search.module is loaded already).
In other words: You'll get a fatal error if Search module is not enabled in the parent site, and adding such a dependency is generally wrong.
However, with all these changes here, we could as well leave this test alone as a unit test and do not change it at all - despite that it's calling into its Search module API functions which in turn are invoking module hooks. That is, because with this new code, we're resetting and emptying out the module list, so it doesn't actually matter whether the API functions are trying to invoke module hooks.
Sorry, this grammar/typo in "to l()" originates from me. Should be "for".
With those adjustments, we should be done.
Comment #40
BerdirAh 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? :)
Comment #41
sunNot really. Unit tests do not have any "default" code they can rely on to be loaded. That's by design, and not only applies to Drupal's unit tests.
Therefore, all unit tests always have to explicitly load the code they want to test, and that's really nothing new.
Comment #42
neclimduljust call drupal_load()?
Comment #43
BerdirOk, 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.
Comment #44
kbasarab CreditAttribution: kbasarab commentedI 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.
Comment #45
sunThanks!
I'm tempted to add the Avoid commit conflicts tag here, but let's try without. A quick commit ASAP would be nice.
Comment #46
catchWhere 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.
Comment #47
catchDiscussed 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.
Comment #48
aspilicious CreditAttribution: aspilicious commentedI looked at backporting this patch but module_list function differs between D7 and D8. Someone will have to do it :)
Comment #49
chx CreditAttribution: chx commentedI 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?
Comment #51
Berdir8.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...
Comment #52
sunLike the original patch for D8, this backport is blocked on #1541958: Split setUp() into specific sub-methods, which has not been backported yet.
Comment #53
chx CreditAttribution: chx commentedI 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?
Comment #54
sun#1541958: Split setUp() into specific sub-methods landed, so this can be backported.
Quite a pain to extract + redo the changes (and making sure to only include the changes of this issue).
Comment #55
aspilicious CreditAttribution: aspilicious commentedI 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 ;) )
Can't find this in the d8 patch
Missing in the D8 version
same as above
-29 days to next Drupal core point release.
Comment #56
sun@aspilicious: The differences you've pointed out are stemming from #1541958: Split setUp() into specific sub-methods, which landed for D8 and D7 already. The earlier patch for D8 on this issue did not include the changes of the follow-up patch on that issue.
Thanks for the additional confirmation that the D7 patch for this issue is (almost) identical to the D8 patch :)
Comment #57
sunI'm tempted to mark this backport RTBC even though it is my own patch, since it is incredibly hard to (re-)roll and we should really try hard to keep the testing framework in sync between D7 and D8.
Comment #58
tim.plunkettStill applies, and looks good.
Comment #59
sunComment #60
webchickYikes! This is a big patch. I really think for 7.x, this could use final sign-off by David. Assigning to him.
Comment #61
David_Rothstein CreditAttribution: David_Rothstein commentedYikes, 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:
In Drupal 7, module_load_all() isn't using drupal_static() currently, so these can't be working as intended.
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.)
(minor) The code comment here refers to a Drupal 8 class name.
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?
Comment #62
BrockBoland CreditAttribution: BrockBoland commentedNeeds issue summary
Comment #63
sunPlease bear in mind that the higher-level goal is to keep the testing framework in sync between D7 and D8.
1) Good point. Should be adjusted accordingly.
2) I think we want to have that protection - especially because of simpletest_clone projects that might override setUp()/tearDown() -- we need to be 100% sure to not operate on the original database of the parent site.
3) Very minor, yes.
4) There is no TestBase::setUp(), because the individual operations are split out into prepareDatabasePrefix(), changeDatabasePrefix(), and prepareEnvironment() - which are being called in different ways from the various test case implementations. That is, because unit tests need to perform different operations than web tests in between those standardized operations. So there is a setUp() + tearDown() parity, it just looks different. I don't see what renaming TestBase::tearDown() to something else would change, or what problem it would try to address. All test cases are supposed to call into it.
One essential background info that might be missing for this patch is that unit tests did not properly set up the test environment previously. This is why the generic tearDown() method exists for all tests on the base class - it makes sure to properly clean up the test environment that is (and has to be) common for all test runs.
---
That said, I'm demoting this to normal, since I think that unit tests are not as horribly broken in D7 as they were D8 -- the entire point of the backport is really just to keep the testing framework in sync, so any future changes + fixes can be applied more easily (since this and related changes are quite huge, and we're generally running low on developer resources on the testing framework, so keeping things consistent should hopefully help with future backports of bug fixes).
Comment #64
jhodgdonIf 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?
Comment #65
sunUgh. I looked into the drupal_static() difference in module_load_all(), which is a regular static in D7, and there does not seem to be a way to reset and re-run its code as in D8.
That said, I'm adding explicit $reset arguments to module_load_all() and module_list() in #1215104: Use the non-interactive installer in WebTestBase::setUp() to actually get rid of the drupal_static() in them, since their values (and thus the locked module list) gets lost as soon as any arbitrary code calls into drupal_static_reset().
Thus, and despite the fact that the patch actually passed the tests, I think that the only way to backport this to D7 would be to backport those changes that replace drupal_static() with explicit $reset arguments. (In fact, module_list() already has $refresh in D7, so only module_load_all() would have to be changed.)
Comment #66
jhodgdonI 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. :)
Comment #67
webchickIMO "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. :)
Comment #68
sunThe primary purpose of backporting these changes was to retain a comparable testing framework between D8 and D7, so as to make future backports easier.
Getting close to D8 feature freeze, and facing a heavily diverged simpletest framework between D8 and D7, it's close to impossible to retain "similarity" between major core versions, and doing so involves a huge risk of breaking things badly in D7.
Therefore, I'm calling out the end to testing framework backports.
For D7, you get to work with the current mess. At the very least, that's "predictable."
Needless to say, actual bug fixes are excluded from that. However, given that there are 30,000+ test assertions that pass against D7, people will have to make pretty good arguments and show solid proof that there is actually a bug somewhere. ;)
Let's focus on the future. (Which isn't Simpletest either way.)
Thanks everyone for working on this and making Drupal awesome! :)
Comment #69
donquixote CreditAttribution: donquixote commentedI 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:
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".
Comment #70
donquixote CreditAttribution: donquixote commented#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.
Comment #71
donquixote CreditAttribution: donquixote commentedMy ANTI patch is nonsense. I wish I could stop the testbot on the previous post :/
Comment #72
donquixote CreditAttribution: donquixote commentedProbably 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.
Comment #73
YesCT CreditAttribution: YesCT commented#71: drupal-7.x-module_list-DrupalUnitTestCase-1563620-71.patch queued for re-testing.
Comment #74
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedI 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.
Comment #75
chaby CreditAttribution: chaby commentedagree 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.
Comment #76
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedWorks for me
Comment #77
David_Rothstein CreditAttribution: David_Rothstein commentedI 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?
Comment #78
chaby CreditAttribution: chaby commentedRTBC for me, thanks
Comment #79
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedWorks for me, too.
Comment #79.0
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedUpdated issue summary.
Comment #80
alberto56 CreditAttribution: alberto56 commentedI 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'.
Comment #81
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted 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).