Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
If all goes well, then we see a verbose backtrace in the test result.
Comment | File | Size | Author |
---|---|---|---|
#94 | test.unit_.94.patch | 18.43 KB | sun |
#94 | interdiff.txt | 2.88 KB | sun |
#90 | test.unit_.90.patch | 18.26 KB | sun |
#90 | interdiff.txt | 1.58 KB | sun |
#88 | test.unit_.88.patch | 17.89 KB | sun |
Comments
Comment #2
sunSo here we go. ConfigImportTest fully passes with this.
I hope the tweak to drupal_flush_all_caches() is acceptable -- it's only called very rarely either way.
Comment #3
sunSlightly adjusted to use the prepared $this->configDirectories['active'], and do not output a verbose backtrace for debug() messages.
Comment #4
sunI think this is ready.
The only part that admittedly is a bit poor is the tweak to drupal_flush_all_caches() to prevent it from running in unit tests. For now, checking whether the 'system' table exists was the best and most unique condition I was able to come up with.
An alternative to that would be to check for the 'install_finished' variable that is being set by the installer on completion (which would have to be wrapped in a try/catch, since the variables table doesn't exist yet).
We certainly don't want to introduce a new WTF in Drupal core with this. But I'd really like to quickly move forward with the essential changes of this patch, so we can use unit tests for many more tests (and also get much better error reporting).
Comment #5
moshe weitzman CreditAttribution: moshe weitzman commentedThis looks good, but I think flush_caches might be the first of many hacks to get UnitTestBase where it is widely useful. I'm good with committing this. If we wanted a bit more assurance, I might ask for two more conversions from WebTestCase.
Comment #6
catchThat's not a unit test?
Comment #7
sun@catch,
We need to change our understanding of what a unit test is.
The idea and desperate need is to make it possible to use unit tests for many more tests.
This means that we will have to make some compromises, in particular with regard to functions like dfac() that are being called out of nowhere and assume a fully working system to exist. They are bad in terms of architecture/design anyway. However, there should not be many of them.
There's a lot of functionality that can be tested without a fully installed Drupal site. By leveraging the service container and overriding services to mocked/null implementations, we're actually in a position to truly start to use unit tests more aggressively. This wasn't possible before.
The only alternative I could provide instead of the tweak to dfac() is to remove the call to dfac() from config_import() in the first place — i.e., making the caller responsible for flushing all caches after performing an import. Perhaps that's not a bad idea anyway.
However, in any case, that would be a one-off adjustment to allow to use unit tests for Config system tests. I'm relatively sure that we'll run into the particular dfac() problem in other unit tests. But then again, I'm happy to make that a separate/later follow-up issue.
I advanced on this patch and converted almost the entire Configuration test group into unit tests. (Executed in 2 seconds.)
Comment #9
sunhah, resolving those test failures made me run into a huge PHPWTF with the $GLOBALS superglobal. Cost me some hours to get behind the magic insanity that starts to happen when you try to use array_* functions on $GLOBALS.
Attached patch should come back green again. Hopefully. ;)
Comment #10
sunReverted left-over debugging code in drupal_alter().
Comment #11
moshe weitzman CreditAttribution: moshe weitzman commentedLooks great to me. The drupal_flush_all_caches hack is gone. Was that deliberate?
Comment #12
catchNo, we really don't. We need to refactor code (and tests) so they're unit testable. #1637478: Add a PHP array cache backend converted several WebTestCase to UnitTestCase by refactoring the tests.
I wouldn't necessarily be opposed to an isolated hack (i.e. a new FastHackyTestBase) that does some of this to allow procedural code with some dependencies to be tested faster, but IMO it's a very, very good feature that unit tests completely blow up if you try to invoke hooks, because that tells you you're doing it completely wrong and need to figure out why.
Comment #13
sunI'm not sure I understand what the problem is. :(
This patch factually demonstrates that the module/hook system is able to work perfectly fine in a unit test. Yes, that relies on the fixed module list functionality. But as soon as #1331486: Move module_invoke_*() and friends to an Extensions class is done, we'll be able to turn that fixed module list functionality into an alternative implementation of that Extensions service, and use/inject that into tests (as well as the installer).
There's absolutely no reason why a unit test should blow up, just because a module hook is invoked. A unit test will blow up if you don't provide the required services, storages, and infrastructure. And that's still the case with this patch, and by design.
I just looked at #1637478-63: Add a PHP array cache backend and that is actually a one-off hack, which essentially does the same like the proposed generic changes here. In fact, some of the changes over there are bogus, since they do not prevent information from the parent site/test runner leaking into the test.
Comment #14
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedI think what catch is trying to explain - correct me if I am wrong - is that if your "unit test" is blowed up by a hook invocation, its because its not really a unit test.
Remember a unit test in procedural programming can be an entire module but is more commonly an individual function or procedure. (In object-oriented programming, a unit is an entire interface, such as a class, but could be an individual method.) [ref].
When it comes to hooks, this is a bit tricky, hooks are an implementation of the template pattern [ref], which allow programmer to alter functionalities without changing the base algorithm: hooking into steps of the execution of an algorithm. I'm quoting from the article: From the design and testability point of view (I think this is where catch is saying we are doint it wrong), Template Method should be used sparingly, and mostly in high-level components which acts more as a declarative layer and are not the subject of extensive unit testing.
In fact, the testing of the base algorithm, that can be compared to an abstract class, is usually non-standard for programmers that has made an habit of Test Driven Development, but is indeed possible: a custom subclass of abstract class is build specifically for the test. The author of the article says that testing the single concrete class (or implementation) is instead difficult as in every test you will throw in also the business logic of the abstract clss, which was factored out specifically to avoid dealing with it.
To make it a real unit test, that is, a test testing a unit of source code, you have to either:
There is a third and elegant solution (again taken from the article) that I will transpose here: When we found ourselves struggling with testing concrete hook implementers in isolation, we may want to refactor Template Method into a Strategy pattern or similar composition solution build with Dependency Injection in mind.
Strategy pattern is very similar to template method, but is not easy to translate for PHP: the benefits of template methods is that PHP nearly allows the use of such a pattern natively, for Strategy however, it implicates that each Strategies implements a common interface: 'invoke' for instance and are encapsulated into classes. That will not be something easy to put into Drupal, but at least will resolve totally the issue of "unit tests with template method".
Edit: just saw the #1331486: Move module_invoke_*() and friends to an Extensions class issue !
Sylvain
Comment #15
Crell CreditAttribution: Crell commentedI have to agree with catch in #12. A unit test is and should remain a "you're on your own, you control the universe" situation. Technically the fact that we even create a working database connection is more than we should be doing, but since it's just a raw connection I think we can get away with it.
Once we manage to make #1331486: Move module_invoke_*() and friends to an Extensions class happen (someone needs to tackle that), we may be able to mock more things in unit tests effectively. Specifically, we'll want to put the Extension object into the DIC and inject that, and then module_invoke_all() becomes yet-another-dumb-wrapper that we avoid when doing OO code. It still won't be complete, though, and to really use unit tests we need to just buckle down and refactor things to be unit-test-oriented (and thereby also get all of the other loose-coupling benefits such an architecture provides).
I'm not even convinced of FastHackyTestBase yet, but I'm curious to know what such a middle-ground would entail.
Comment #16
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedI think a clear way of underlining what Crell and catch are saying, is to think in terms of code coverage. If your test is covering more than the unit of source code that it is initially intended (i.e. supplementary database classes), then its more likely an integration/functionnal test.
Comment #17
boombatower CreditAttribution: boombatower commentedDoing unit testing of code that calls hook and/or chained functions was something that was discussed thoroughly back in Paris sprint that got testing into core. It was fairly well accepted that unit testings is/was virtually impossible for such code (especially drupal's code at that time).
The two extensive approaches were to use either runkit or to write a script that could break up the code, putting each function in separate file, provide a router function, and then any function call could be mocked. The script could be invoked before running tests (obviously cached and what not) and provide a set of functions to say I want to mock this functions with this return value (what you can do with runkit override function).
Seems like we are running into this wall again and either need to wait for the re-architecting described above or some sort of solution like those originally discussed. The reason the idea was dropped was simply that functional testing got a lot more coverage quickly and was generally much more feasible.
Hooks are one large blocker to unit testing, but I don't know how much code remains in a chained form that is difficult to unit test. If there is more than can be changed any time soon we should probably investigate one of these methods.
The script to break up the code and the router all worked back when so I can try to find them again, but runkit seems like a more powerful solution should we choose to go down this road.
EDIT: For those interested I believe this was the code http://drupalcode.org/project/simpletest_unit.git/tree, but again runkit is more fully featured, but obviously requires installation.
Comment #18
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commented@boombatower When #1331486: Move module_invoke_*() and friends to an Extensions class will be fixed, hook wont be a problem anymore. Template Method/hooks will be replaced by a composition solution build with dependency injection in mind. This would totally resolve the problem with hooks.
Comment #19
Crell CreditAttribution: Crell commentedNo, it will partially resolve the problem with hooks. Not all code would be able to benefit from it. It's still a good idea but it's not a silver bullet.
Comment #20
boombatower CreditAttribution: boombatower commentedRestructuring code with half the reason being testing always makes me wonder if our testing tools are lacking. I know this is probably not a popular thought in the software world, but seems like having the ability to simply manipulate things during testing saves a lot of unnecessary juggling to make it happen in the base syntax. Would have to look at things in detail, but just a reoccurring thought. Dependency injection seems like the extreme case...as it even breaks some of the fundamentals of OOP theory aside from generally making the code harder to follow.
EDIT: This is not intended to evidence against the patch, just thoughts.
Comment #21
sunI'm a bit lost here. :(
If I get you guys right, then you'd be fine with these changes if I move the proposed changes into a new
class FakeUnitTestBase extends UnitTestBase
-- is that correct? [give or take a better name](I'm not able to follow the argumentation against the current proposal, like, at all. Obviously, the ultimate goal is and has to be to make the module/extensions system as well as the hook system not only mockable, but also operational in special environments. We currently achieve that through $fixed_list, and ultimately exactly that needs to be replaced with an alternate FixedExtensions service implementation that's registered and loaded instead of the default runtime implementation. So. If I write a unit test that registers my own module only, in order to get my own hooks invoked to assert expected I/O in my functions, then, what exactly is not a unit test in that, please?)
Anyway, there's an incredible high amount of tests that are able to leverage this facility, so I will be more than happy to move the changes into a new base class, if that is what is required.
Comment #22
Crell CreditAttribution: Crell commentedsun: I'm open to a "SemiUnitTest" middle-ground, given our current code base. I'd like more information on how that would work, exactly.
That said, that does not mean that we redefine "unit test" to mean "kinda sorta unit-ish that we're hacking". A unit test is a unit test: Testing one and only one unit in isolation. If your unit is too big for that to make sense, it means your code is wrong. Period.
Refactoring code to be testable by for-reals unit tests should be the ultimate goal. We should not shirk from that goal. We should, rather, be emphasizing that, and code refactoring that offers no benefit other than "pure unit testable" should be encouraged, as that by design results in code that is more loosely coupled, easier to test, faster tests, and easier to extend later without breaking things in weird and unpredictable ways.
A quasi-unit test mechanism is an acceptable stop gap, and I'm open to it. I am not open to "We need to change our understanding of what a unit test is." You may as well try to redefine the word "function". Moving hooks to an Extension class will help, but then to make an object that uses hooks properly injectable we need to also make that Extension class injected into it, which would be the proper way to make it mockable. (That was my response to Sylvain.)
@boombatower:
Our testing tools are lacking in many ways, but that's not one of them. Our code base's testability is lacking more. Some code bases are just inherently difficult or impossible to test due to the number of dependencies and state they rely on. Lots of dependencies and global state are architectural bugs in and of themselves. Those are bugs we're slowly working on fixing, but we have a long way to go.
I have no idea what fundamentals of OOP you think DI breaks, but pointing out how wrong that sentence is would be out of scope for this issue. :-)
Comment #23
boombatower CreditAttribution: boombatower commentedI agree it is out of scope, but there are many articles on both sides of the subject that explain it better than I could. It also depends exactly how it is done.
As for not calling this unit testing that seems to make sense. Some terms I've seen used elsewhere are small (unit), medium (just a bit more than unit), and large (functional/integration) tests. For instance google http://googletesting.blogspot.com/2011/03/how-google-tests-software-part.... Any opinions on such names?
Comment #24
moshe weitzman CreditAttribution: moshe weitzman commentedThis patch moves us forward for sure.
+ $cache_objects = &drupal_static(__FUNCTION__, array());
Please justify new static caches.
I'm curious why we can't just call module_enable(). Why is the module_list call needed? Is it because we are in a Unit test?
Comment #25
sunMerged latest HEAD.
Improved documentation for enableModules(). [re @moshe]
I did not move the code into a new base class yet.
@Crell:
It would work exactly like this patch, no difference. Really, all this patch is doing is this:
1) It sets some more services to Null implementations (Lock\Null, Cache\Null, etc) and adjusts the config service to use a plain FileStorage (without cache), so none of these services try to query a storage that doesn't exist in unit tests. The services can still be overridden through
$this->container
in individual tests. I think this is, in fact, 100% in line with what you want and mean with "you're on your own".2) It allows a unit test to set a list of modules to be loaded. This is all optional. It uses the $modules property mechanism, identical to WebTestBase for collecting the modules. The specified modules are loaded only, and set as the $fixed_list of module_list(), which essentially enables our hook/event system, but only for the specified modules. This does not affect how you're testing functions in unit tests. The only thing it does is to allow you to test functions that might involve hook invocations. The effect is that you can finally unit-test API functionality that just happens to have a drupal_alter() or module_invoke() in it, which wasn't unit-testable before.
3) Doing this revealed that there are a range of global variables that are leaking into unit test methods.
Comment #26
Crell CreditAttribution: Crell commentedThat doesn't look too bad at first glance, but it appears that you're making UnitTestBase less Unit-y, rather than creating a 3rd middle-ground option. Making UnitTest less Unit-y is exactly the thing I am objecting to. Creating a 3rd base class, QuasiUnitTestBase (or whatever), I am more open to.
Comment #27
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedI am unable to apply the patch, I have a minor but unmatched patch segment (see attached).
I think its related to the last version I use from drupal-core, can you rebase it against HEAD ?
EDIT: Its ok, was able to do it myself :)
EDIT: I can't execute a single test in local, I am on the d8 HEAD version. Even with your patch not applied... If someone can head me to the right direction.
Comment #28
sunMerged in HEAD.
Comment #29
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedMaybe this has nothing to do here, but with the HEAD I cannot manage to run a single test from Drupal SimpleTest UI. Either with or without the patch test is running for infinite and just not complete. Any thoughts ?
Comment #30
moshe weitzman CreditAttribution: moshe weitzman commented#28: test.unit_.28.patch queued for re-testing.
Comment #31
moshe weitzman CreditAttribution: moshe weitzman commentedRe #26, I think a 3rd testCase class would increase confusion for not enough benefit. I think this patch moves the ball forward to RTBC. Committers can weigh in from there.
Comment #32
Crell CreditAttribution: Crell commentedMoshe: "Removing true unit tests from Drupal because our current code is too hard to test" is not a benefit. catch's comment in #12 sounds like he doesn't like "redefine unit test to not mean unit test" as an approach, either. (catch, feel free to correct me if I'm wrong.)
Comment #33
sunThoughts about a suitable class name ended up in "Drupal", because the extended unit test base class enables "Drupalisms" (a.k.a. module/hooks) for unit tests.
Moved UnitTestBase extensions into a new DrupalUnitTestBase.
Hopefully this makes everyone happy.
Comment #35
sunAdded (central) documentation for DrupalUnitTestBase::$modules.
Comment #37
sunMerged in HEAD.
Comment #38
sunRemoved backup/restore of global variables. Solution is too hacky; separate issue.
Everyone happy now? :)
Comment #39
plachHm, comments not wrapping properly ;)
Comment #40
sunCreated separate follow-up issue to properly discuss and address: #1805808: Global variables are leaking into tests, and from one test case into the next
Comment #41
sunheh, that's what you get for copypasting core code ;) Thanks!
Fixed phpDoc.
Comment #43
Crell CreditAttribution: Crell commentedMuch nicer, sun, thank you!
Why do we need to add this in UnitTestBase rather than DrupalTestBase?
Comment #44
catchThe main thing with this is if an existing unit test gets something like a hook invocation or a t() call or something, then it should blow up or otherwise fail, if we let people do those things with no repercussions then it makes it easier to write a really bad unit test.
On the other hand allowing procedural code without loads of dependencies to be tested in a much more stripped down environment than the testing profile is great as well. So the new test class seems like the best of both worlds there.
Comment #45
sun#41: test.unit_.41.patch queued for re-testing.
Comment #46
sunre: #43: @Crell:
"Scaling down" or limiting unit tests is not really part of this issue ;) but let me try to explain the current situation:
All tests are executed in a full bootstrap, which generally means that unit tests are executed in a much "richer" environment than you like. Various globals, statics, and environment variables are set up already.
Additionally, the Simpletest framework has to set up a minimal working space for a test's error log, verbose output, and other file data.
This preparation happens centrally in TestBase::prepareEnvironment() for all tests. Every test gets a new minimal bootstrap container via drupal_container(). drupal_container() calls into config_get_config_directory(), which needs config directories already.
That's why some services already exist in tests, including unit tests. I'm happy to investigate options to clean that up and make unit tests not have anything, but that really belongs into a separate issue ;)
If you just want me to move those added lines into DrupalUnitTestBase, mmm, ok...
re: #44: @catch:
I guess your remark is along the lines of @Crell's. But yeah, making unit tests blow up on any call to t() or anything else is a very long shot...
Moved service definition overrides into DrupalUnitTestBase.
Comment #48
sunFallback page title for user pages is 'User account' for anonymous users.
Other UserAccountLinksTests.php 118 Drupal\user\Tests\UserAccountLinksTests->testAccountPageTitles()
Comment #49
sun#46: test.unit_.46.patch queued for re-testing.
Comment #50
sunThanks to #1608842: Replace list of bootstrap modules, enabled modules, enabled themes, and default theme with config, {system} table is gone, so there's one barrier less for enabling modules in DrupalUnitTestBase.
Over there, we apparently overlooked that the list of enabled modules/themes can be completely empty. ;) So this patch contains some simple follow-up adjustments to that.
Unfortunately, that commit also caused by massive, horrible slowdown in performance for DrupalUnitTestBase:
The total time for 7 test cases went from 2 seconds to 20 seconds. I identified some problems, but wasn't able to figure out the major cause yet. :-(
That said, that shouldn't prevent this patch from landing. The performance is still much better compared to WebTestBase, and we can certainly figure out what the problem is later on (and get back to a lightning-fast unit test performance).
Merged in HEAD.
Fixed list of enabled modules/themes can be empty.
Fixed missing ability to override KeyValueFactory backend for unit tests.
Updated DrupalUnitTestBase for new system_list().
Comment #51
Crell CreditAttribution: Crell commentedSun, are you saying UnitTest went from 2 to 20, or the new DrupalTest went from 2 to 20? The latter is still better than the several minutes for WebTest. The former is a major red flag for me.
Comment #52
sunI did not actually test the performance of UnitTestBase, only DrupalUnitTestBase. Luckily, a recent test failure exposed all unit tests in core, so there's at least a list to benchmark ;)
Um... so the good news is that the performance decrease is not in DrupalUnitTestBase ;) The bad news is that UnitTestBase became horribly slow (try to run this set on HEAD:
Drupal\system\Tests\Plugin\DerivativeTest,Drupal\system\Tests\Plugin\DiscoveryTest,Drupal\system\Tests\Plugin\FactoryTest,Drupal\system\Tests\Plugin\InspectionTest
)... :( Again, that's on HEAD, without this patch, so separate issue:#1808200: Unit test performance significantly decreased since system list config conversion
Comment #53
Crell CreditAttribution: Crell commentedWell crap. Separate issue.
So does this patch have any impact on performance against HEAD? If not, I think we can move forward with it.
Comment #54
sunYes, this patch only has a positive impact on performance. Would love to move forward :)
Comment #55
BerdirLooks interesting, sounds like this could help to speed up tests that can't be complelety injected for the time being.
Not really a review, just some comments below.
#1764474: Make Cache interface and backends use the DIC is using a different approach for this, by explicitly passing the database info in using setParameter(). I think that is the better approach because it allows to use the Database without messing with the globals, but also requires a ton of changes, especially during the install process as you can see in that issue.
So I'll just merge this once this is commited and it will hopefully continue to work.
I had a similar problem in the issue mentioned above, but in that case, it was because the cache is a service there and the container is only restored after the module list is rebuilt. I guess that's not the problem here, might still be interesting as a cross-reference, because this change would probably hide my problem. See #1764474-79: Make Cache interface and backends use the DIC.
Also, I like the way you changed the KeyValue store factory to make it a bit more flexible. Will have to merge that the cache_update issue, once this and the cache DIC issue are in. Yes, lot's of dependencies between these issues.
Comment #56
sunOn database service:
Yeah, I've seen that. This patch just moves the definition though, without changes. The issue is that unit tests do not get a new Kernel, so the database service isn't available. It might be debatable whether DrupalUnitTestBase shouldn't create a new Kernel like WebTestBase does, but for now that isn't necessary, and I'd appreciate to discuss that in a separate follow-up issue.
On empty list of enabled modules:
Yeah, definitely a different problem. In the case of DrupalUnitTestBase, those lists are intentionally empty by default ;) so the module system operates on an empty list, and is perfectly capable of doing so. Pre {system} table removal, that apparently worked by design; the config conversion merely did not account for that possibility, so the returned NULL blows up with a fatal error due to the subsequent foreach().
On KeyValueFactory:
Yeah, but please note that the change I'm proposing here is only a minimal adjustment to get the keyvalue service to behave in tests. I do not think it is really clean and well thought-out - just rather a quick fix that unfucks the situation. The original system_list() config conversion should have filed a major bug as a follow-up issue to fix the mess it introduced, but apparently didn't yet, so:
#1809206: KeyValueFactory hard-codes DatabaseStorage
Comment #57
chx CreditAttribution: chx commentedthis is an approach that simply does not work. We did similar things before and we realized it's wrong. Although DatabaseStorage at this very moment does not yet take a database connection injected it does not mean it shouldnt and once the two-DIC issue is fixed it will. If you want to use a different backend, use a different factory, that simple. Because that backend will need its own configuration data injected. Say, a Redis connection or something like that. This approach with variable classnames goes against what is possible with the DIC currently.
This is why the K-V constructor was removed from the interface and this is why, once we have one DIC to rule them all we will need to revisit places like the new php loader and nuke their constructors too and change their factories too.
Comment #58
BerdirRight. No idea how I forgot that as it was me who made that argument ;)
The problem with how we use these factories currently is that we would actually have to add an interface for them, so that it's valid to replace them. And it should probably be named e.g. DatabaseStorageFactory or something like that.
Comment #59
chx CreditAttribution: chx commentedFiled #1810912: [meta] Decide on pluggability
Comment #60
sunThe last comments only tell me one thing:
Next time I see such a gross hack in a patch, it will need work until the hack no longer exists, regardless of how major/critical it is. You're blocking a completely unrelated, innocent issue from landing, just because it needs to fix utter nonsense that was introduced before.
Comment #61
sun#50: test.unit_.50.patch queued for re-testing.
Comment #63
sunMerged HEAD.
Is there any chance to move forward here, deferring a more sophisticated KeyValueFactory service registration solution to #1809206: KeyValueFactory hard-codes DatabaseStorage?
Comment #64
sun#1808220: Remove run-tests.sh dependency on existing/installed parent site is also blocked on the KeyValueFactory stop-gap fix here now.
Comment #65
chx CreditAttribution: chx commentedIf you add a @TODO to remove that, I guess this is good to go.
Comment #66
chx CreditAttribution: chx commentedI rerolled but I can't with a clear conscience RTBC this. I know we are in a hustle mode but are we in a hustle so much that we can't wait for the K-V code to be written (few days, I will get it tomorrow if noone else does) and also for #1764474: Make Cache interface and backends use the DIC to get in (which gets you proper database DIC support) ? Or can we split off the database from the latter and get it in? The amount of hackery that went into this patch is a little bit too much for my taste.
Note: I am pointing out actual existing and actionable problems. I am not opposing the patch, but these two hacks are really ick.
Is this patch blocking a chain of other waiting to happen?
Comment #67
Crell CreditAttribution: Crell commentedThe DB-DIC parts of #1764474: Make Cache interface and backends use the DIC have been split off to #1811730: Refactor Database to rely on the DIC solely.
Comment #68
sun#66: 1774388_66.patch queued for re-testing.
Comment #69
sunMerged HEAD for KeyValueFactory changes.
Comment #70
BerdirOk, I think the icky points that chx mentioned have been fixed in the other issue/commited with it. Either way, they're not part of this patch/issue anymore.
The only thing that I find a bit icky is the name DrupalUnitTestBase. I fail to see how this is more "Drupal" than UnitTestBase or WebTestBase and I thought that we shouldn't prefix class names with Drupal :) Can't we find a name that actually says what it is, something like MockTestBase, MockModuleTestBase or something along those lines?
Anyway, I am fine with getting this in as it is right now, as it will unblock #1780396: Namespaces of disabled modules are registered during test runs, which is causing serious pain for the big plugin conversion issues (entity types, blocks, ...) that they have to work around with ugly code. Given that it blocks/slows these down, I'd even say that this should be major, but that doesn't matter as long as we can get it in.
Let's fight over the name in a follow-up :)
Comment #71
catchThis generally looks fine, but I have a question about this line:
This should really happen in the config UI module anyway rather than the API function - I might want to manually flush caches during a config deployment - maybe I've got other stuff to worry about before the cache flush or similar.
So I'm OK with removing it, but why's it necessary here?
Comment #72
sunIt is removed here, because
config_import()
is called byconfig_install_default_config()
, which in turn is called bymodule_enable()
when a module is installed.That stack of triggered operations is explicitly wanted behavior, since, in fact, the module installation process is otherwise not touched at all here, but yet, a module can be installed as usual (if necessary) and this works perfectly fine.
However, to clarify once more (the phpDoc already does), DrupalUnitTestBase does not install modules unless that is explicitly asked for. By default, any specified modules are only loaded and defined as the fixed module list, so their code is available and hooks can be invoked.
drupal_flush_all_caches()
has to be removed, since it calls into way too many different layers, subsystems, and services to be able to work in a unit test. The original patch in #3 tried to work around this by checking for the existence of a database table, but further testing quickly revealed that there is no way to make dfac() work in an "empty" environment, because its entire purpose is to "rebuild each and everything from scratch by destroying everything we knew and rebuilding all data from all data and code that currently exists in the [file]system" in the first place, which presents a 100% conflict to the idea of an environment that is limited by design. Once we're done with eliminating all variables, the situation might look different. But then again not, because dfac() actually has been designed to effectively be the rebuild script that @Crell asked for in some other issues:That's it. That's the entire purpose of dfac(). It always was. Up until D7, this script was/is able to cope with pretty much every edge-case situation that can occur (even moved locations of modules/extensions). We improved its capabilities by separating cache flushing from rebuilding for D8. Our current built-in "last resort" in core,
update.php
, is essentially doing the same (even if there are no updates).Anyway, that's getting very off-topic for this issue. ;)
So in short, it needs to be removed here to allow DrupalUnitTestBase to install a module (in an otherwise empty environment).
Also, FWIW, in the meantime I've identified plethora of web tests that can be converted to DrupalUnitTestBase while working on other issues in the queue. Thus, once this patch has landed, we can heavily speed up a full range of tests.
Comment #73
catchYeah that's all fair enough. Committed/pushed to 8.x.
This needs a change notice to encourage people to use it for existing procedural code. We could also open a follow-up for the rename but I don't have better ideas at the moment.
Comment #74
tim.plunkettThis was never retested after the entity type plugin patch.
The entity manager has to be registered earlier.
I can confirm this fixes the failures
Comment #75
tim.plunkettThat patch was lifted straight from #1697256-111: Create a UI for importing new configuration, I only thought it might help because I'd just been discussing that issue with heyrocker. It's his fix, technically :)
Comment #76
tstoecklerI can verify this fixes the tests. I ran the three that are currently broken, and they are fixed.
Comment #77
webchickCommitted and pushed to 8.x. Thanks a TON! Hopefully this makes testbot happier. :D
Comment #78
webchickAnd by that I mean...
Comment #79
nikkubhai CreditAttribution: nikkubhai commentedDidn't catch commit it already in #73?
Comment #80
sunNew, separate, extended DrupalUnitTestBase for unit-testing functionality whose dependencies can't be injected
Comment #81
Crell CreditAttribution: Crell commentedNow that is a change notice!
Comment #82
sunSome quick follow-up corrections, discovered through actual usage of the
installSchema()
method in tests for another issue. The method wasn't used anymore with the PoC conversions of the committed patch in the end.Summary:
module_enable()
cannot (and must not) resolve dependencies. This is still a unit test.installSchema()
should blow up when trying to install the schema of a table that is unknown. (Typical mistake: Not having the owning module loaded through the $modules list.)drupal_get_schema_unprocessed()
apparently contains a logic error, and returns the full $schema in case $table is not found in $schema.(D'oh. I just realized this very issue effectively allows me to write a unit test against that function now, but I hope I don't have to. ;))
Comment #83
tstoecklerLooks very good. I don't feel up to RTBC'ing this myself, though. Have also yet to write tests based on the new DrupalUnitTestCase, so...
Comment #84
sunAdded test coverage for DrupalUnitTestBase.
Writing tests for this turned out to be a damn good idea. :)
Comment #86
sunFixed bogus comments and variable names.
Comment #87
Lars Toomre CreditAttribution: Lars Toomre commentedSince messages are user facing, my understanding is the messages should use format_string() rather than raw variable concatenation. Is my understanding wrong?
Comment #88
sunFixed installation of multiple modules and DIC/Kernel integration.
Victory.
Comment #89
sunAny chance to move forward here?
The follow-up patch fixes a range of problems with DrupalUnitTestBase, into which various core developers are running in other issues. With this patch, the test class meets all expectations and can be used in all situations.
Comment #90
sun#1831350: Break the circular dependency between bootstrap container and kernel was committed without tests. This patch adds the essential + required (implicit) test coverage for that code. Thus, this is a critical task.
Comment #91
chx CreditAttribution: chx commentedNote that I tried to convert the TaxonomyTermReferenceItemTest I submitted yesterday to this and the following two steps were required:
foreach (array('field', 'taxonomy', 'entity_test') as $module) { drupal_install_schema($module);}
(this was expected)Before I RTBC this I would like to talk to katbailey about the base container approach but I wanted to put this here to state: this is a viable test base class now. Literally yesterday I was unable to get my test working with DrupalUnitTestBase.
Comment #92
sunI consider the base container approach to be temporary until we revamped the DrupalKernel via #1837092: Make sure DrupalKernel can use alternate config storage — once the kernel/container stuff has been cleaned up, the
DrupalUnitTestBase::enableModules()
method should not contain any workarounds for the kernel/container anymore — However, we absolutely want to add this test coverage here before doing that.Comment #93
chx CreditAttribution: chx commentedFair enough, let's go ahead with this.
Comment #94
sunMinor/small comment tweaks to prevent this patch from getting blocked on those.
Comment #95
Fabianx CreditAttribution: Fabianx commentedPurely cosmetical nit-pick, but it should be drupal_container($base_container) instead of drupal_container($base_container, TRUE), because first resetting does not hurt, but is also not necessary if overwritten in the next line.
Besides that: +1 for RTBC
Comment #96
chx CreditAttribution: chx commentedSo doesn't matter. It's utterly provisional. I do not think it'll survive a single week cos I will work on #1837092: Make sure DrupalKernel can use alternate config storage as early as tomorrow.
Comment #97
catchAlright. Committed/pushed this one to 8.x. This will need a change notice for the new base class.
Comment #98
sunThanks!
This was only a follow-up patch to the original introduction of DrupalUnitTestBase, for which a elaborate change notice exists already:
http://drupal.org/node/1829160
I looked once more through the patch to see whether anything needs to be mentioned, and only added
setUpContainer()
to the Usage section of the existing chance notice.Comment #100
effulgentsia CreditAttribution: effulgentsia commentedFor everyone who worked on this, just letting you know it's being appreciated: #1833716-163: WYSIWYG: Introduce "Text editors" as part of filter format configuration.
Comment #101
plachWe should totally leverage this more. There are plenty of tests that could easily be converted to DrupalUnitTests. This would lower the testbot full session time dramatically.
Comment #102
Wim Leers#101: +9000! In my experience, it's more complex to get DUTB tests working, because the dependencies are not always very clear, and it will fail with very unhelpful messages, but once you've got the dependencies cover, the edit test-run test cycle becomes so much faster that it's totally worth it.
(I make it a case of personal pride to ensure that every test I add to core is a DUTB test if it's possible.)
Maybe we should have a new core policy: any new or significantly modified test should be converted to DUTB test if it's possible. That would improve the feedback cycle for patches uploaded to d.o as well…
Comment #103
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedIf you guys liked it, do not hesitate to help me on: #1801176: Deploy a PHPUnit system with a bottom-up approach and #1872006: Add parity test classes for lib/Core and lib/Component to make Drupal 9 (and possibly 8 backward compatible) tests more awesome. :)
Comment #104
alexpottSome followups:
#1893108: Convert most entity tests to DrupalUnitTestBase
#1895046: Convert some RDF tests to DrupalUnitTestBase
#1894960: Convert some lock tests to DrupalUnitTestBase
#1895018: Convert token API and node token tests to DrupalUnitTestBase
Comment #105
alexpottOne more:
#1895054: Convert an openid test to DrupalUnitTestBase
Comment #106
alexpottAnd another:
#1895090: Convert a Jsonld test to DrupalUnitTestBase
Comment #107
sun