Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, drupal8.test-unit-remastered.0.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
6.21 KB

So 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.

sun’s picture

Slightly adjusted to use the prepared $this->configDirectories['active'], and do not output a verbose backtrace for debug() messages.

sun’s picture

I 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).

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

This 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.

catch’s picture

Status: Reviewed & tested by the community » Needs work
For example, API functions being invoked in
+  // unit tests may trigger a call to drupal_flush_all_caches(),

That's not a unit test?

sun’s picture

Status: Needs work » Needs review
FileSize
18 KB

@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.)

Status: Needs review » Needs work

The last submitted patch, drupal8.test-unit-remastered.7.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
22.28 KB

hah, 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. ;)

sun’s picture

FileSize
21.64 KB

Reverted left-over debugging code in drupal_alter().

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Looks great to me. The drupal_flush_all_caches hack is gone. Was that deliberate?

catch’s picture

Status: Reviewed & tested by the community » Needs work

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.

No, 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.

sun’s picture

Status: Needs work » Needs review

I'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.

Sylvain Lecoy’s picture

I 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:

  • Mock the DB layer for hook invocation, and use the custom hook implementer to test the single "concrete class".
  • Remove the hook from the tested function, that imply an architectural change in the drupal design.

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

Crell’s picture

I 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.

Sylvain Lecoy’s picture

I 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.

boombatower’s picture

Doing 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.

Sylvain Lecoy’s picture

@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.

Crell’s picture

No, 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.

boombatower’s picture

Restructuring 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.

sun’s picture

I'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.

Crell’s picture

sun: 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:

Restructuring code with half the reason being testing always makes me wonder if our testing tools are lacking.

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.

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.

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. :-)

boombatower’s picture

I 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?

moshe weitzman’s picture

Status: Needs review » Needs work

This patch moves us forward for sure.

+ $cache_objects = &drupal_static(__FUNCTION__, array());
Please justify new static caches.

+  protected function enableModules(array $modules) {
+    // Add the new modules to the fixed module_list() first, since callbacks
+    // invoked by module_enable() may need to access information provided by
+    // info hooks of the new modules already. module_enable() enables the new
+    // modules in {system}, but that has no effect, since we are operating with
+    // a fixed module list.
+    foreach ($modules as $module) {
+      $this->moduleList[$module] = drupal_get_path('module', $module);
+    }
+    module_list(NULL, $this->moduleList);
+
+    module_enable($modules);

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?

sun’s picture

Status: Needs work » Needs review
FileSize
3.39 KB
21.64 KB

Merged latest HEAD.
Improved documentation for enableModules(). [re @moshe]

I did not move the code into a new base class yet.

@Crell:

I'm open to a "SemiUnitTest" middle-ground, given our current code base. I'd like more information on how that would work, exactly.

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.

Crell’s picture

That 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.

Sylvain Lecoy’s picture

FileSize
97.85 KB

I 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.

sun’s picture

FileSize
21.64 KB

Merged in HEAD.

Sylvain Lecoy’s picture

Maybe 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 ?

moshe weitzman’s picture

#28: test.unit_.28.patch queued for re-testing.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Re #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.

Crell’s picture

Status: Reviewed & tested by the community » Needs review

Moshe: "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.)

sun’s picture

FileSize
12.02 KB
23.3 KB

Thoughts 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.

Status: Needs review » Needs work

The last submitted patch, test.unit_.33.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
902 bytes
23.7 KB

Added (central) documentation for DrupalUnitTestBase::$modules.

Status: Needs review » Needs work

The last submitted patch, test.unit_.34.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
23.75 KB

Merged in HEAD.

sun’s picture

FileSize
4.37 KB
20.95 KB

Removed backup/restore of global variables. Solution is too hacky; separate issue.

Everyone happy now? :)

plach’s picture

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigInstallTest.php
@@ -21,6 +21,15 @@ public static function getInfo() {
+    // Ensure the global variable being asserted by this test does not exist;
+    // a previous test executed in this request/process might have set it.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.php
@@ -0,0 +1,136 @@
+   * Unlike Drupal\simpletest\WebTestBase::setUp(), UnitTestBase::setUp() does not

Hm, comments not wrapping properly ;)

sun’s picture

Created separate follow-up issue to properly discuss and address: #1805808: Global variables are leaking into tests, and from one test case into the next

sun’s picture

FileSize
1.45 KB
20.96 KB

heh, that's what you get for copypasting core code ;) Thanks!

Fixed phpDoc.

Status: Needs review » Needs work

The last submitted patch, test.unit_.41.patch, failed testing.

Crell’s picture

Much nicer, sun, thank you!

+++ b/core/modules/simpletest/lib/Drupal/simpletest/UnitTestBase.php
@@ -55,7 +54,12 @@ protected function setUp() {
+    // Provide a minimal, partially mocked environment for unit tests.
     $conf['file_public_path'] = $this->public_files_directory;
+    $conf['lock_backend'] = 'Drupal\Core\Lock\NullLockBackend';
+    $conf['cache_classes'] = array('cache' => 'Drupal\Core\Cache\NullBackend');
+    $this->container->register('config.storage', 'Drupal\Core\Config\FileStorage')
+      ->addArgument($this->configDirectories[CONFIG_ACTIVE_DIRECTORY]);

Why do we need to add this in UnitTestBase rather than DrupalTestBase?

catch’s picture

The 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.

sun’s picture

Status: Needs work » Needs review

#41: test.unit_.41.patch queued for re-testing.

sun’s picture

FileSize
1.92 KB
20.69 KB

re: #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.

Status: Needs review » Needs work

The last submitted patch, test.unit_.46.patch, failed testing.

sun’s picture

Fallback page title for user pages is 'User account' for anonymous users.
Other UserAccountLinksTests.php 118 Drupal\user\Tests\UserAccountLinksTests->testAccountPageTitles()

sun’s picture

Status: Needs work » Needs review

#46: test.unit_.46.patch queued for re-testing.

sun’s picture

FileSize
9.3 KB
25.24 KB

Thanks 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().

Crell’s picture

Sun, 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.

sun’s picture

I 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

Crell’s picture

Well crap. Separate issue.

So does this patch have any impact on performance against HEAD? If not, I think we can move forward with it.

sun’s picture

Yes, this patch only has a positive impact on performance. Would love to move forward :)

Berdir’s picture

Looks 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.

@@ -2437,6 +2437,16 @@ function drupal_container(Container $new_container = NULL, $rebuild = FALSE) {
     // requests.
     $container = new ContainerBuilder();
 
+    // Database.
+    $container->register('database', 'Drupal\Core\Database\Connection')
+      ->setFactoryClass('Drupal\Core\Database\Database')
+      ->setFactoryMethod('getConnection')
+      ->addArgument('default');
+    $container->register('database.slave', 'Drupal\Core\Database\Connection')
+      ->setFactoryClass('Drupal\Core\Database\Database')
+      ->setFactoryMethod('getConnection')

#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.

+++ b/core/includes/module.incundefined
@@ -182,7 +182,7 @@ function system_list($type) {
       // locations in the file system. The ordering here must also be
       // consistent with the one used in module_implements().
-      $enabled_modules = config('system.module')->get('enabled');
+      $enabled_modules = (array) config('system.module')->get('enabled');
       $module_files = state()->get('system.module.files');
       foreach ($enabled_modules as $name => $weight) {
         // Build a list of all enabled modules.
@@ -196,7 +196,7 @@ function system_list($type) {

@@ -196,7 +196,7 @@ function system_list($type) {
       }
 
       // Build a list of themes.
-      $enabled_themes = config('system.theme')->get('enabled');
+      $enabled_themes = (array) config('system.theme')->get('enabled');
       // @todo Themes include all themes, including disabled/uninstalled. This

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.

sun’s picture

On 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

chx’s picture

      ->register('keyvalue', 'Drupal\Core\KeyValueStore\KeyValueFactory')
      ->addArgument('Drupal\Core\KeyValueStore\DatabaseStorage');

this 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.

Berdir’s picture

Right. 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.

chx’s picture

sun’s picture

The 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.

sun’s picture

Issue tags: -Testing system

#50: test.unit_.50.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Testing system

The last submitted patch, test.unit_.50.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
25.24 KB

Merged HEAD.

Is there any chance to move forward here, deferring a more sophisticated KeyValueFactory service registration solution to #1809206: KeyValueFactory hard-codes DatabaseStorage?

sun’s picture

#1808220: Remove run-tests.sh dependency on existing/installed parent site is also blocked on the KeyValueFactory stop-gap fix here now.

chx’s picture

If you add a @TODO to remove that, I guess this is good to go.

chx’s picture

FileSize
23.83 KB

I 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?

Crell’s picture

sun’s picture

#66: 1774388_66.patch queued for re-testing.

sun’s picture

FileSize
19.15 KB

Merged HEAD for KeyValueFactory changes.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Ok, 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 :)

catch’s picture

This generally looks fine, but I have a question about this line:

-    // Flush all caches and reset static variables after a successful import.
-    drupal_flush_all_caches();

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?

sun’s picture

It is removed here, because config_import() is called by config_install_default_config(), which in turn is called by module_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:

# /rebuild.php
define('DRUPAL_ROOT', getcwd());
require_once DRUPAL_ROOT . '/core/includes/bootstrap.inc';
drupal_bootstrap(DRUPAL_BOOTSTRAP_CODE);
drupal_flush_all_caches();

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.

catch’s picture

Title: Unit Tests Remastered™ » Change notice: Unit Tests Remastered™
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active

Yeah 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.

tim.plunkett’s picture

Title: Change notice: Unit Tests Remastered™ » HEAD BROKEN: Change notice: Unit Tests Remastered™
Category: task » bug
Status: Active » Needs review
FileSize
1.21 KB

This was never retested after the entity type plugin patch.

The entity manager has to be registered earlier.

I can confirm this fixes the failures

tim.plunkett’s picture

That 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 :)

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

I can verify this fixes the tests. I ran the three that are currently broken, and they are fixed.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks a TON! Hopefully this makes testbot happier. :D

webchick’s picture

Title: HEAD BROKEN: Change notice: Unit Tests Remastered™ » Change notice: Unit Tests Remastered™
Category: bug » task
Status: Fixed » Active

And by that I mean...

nikkubhai’s picture

Didn't catch commit it already in #73?

sun’s picture

Title: Change notice: Unit Tests Remastered™ » Unit Tests Remastered™
Priority: Critical » Normal
Status: Active » Fixed
Crell’s picture

Now that is a change notice!

sun’s picture

Status: Fixed » Needs review
FileSize
2.8 KB

Some 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:

  1. module_enable() cannot (and must not) resolve dependencies. This is still a unit test.
  2. 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.)
  3. 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. ;))
tstoeckler’s picture

Looks 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...

sun’s picture

FileSize
10.28 KB

Added test coverage for DrupalUnitTestBase.

Writing tests for this turned out to be a damn good idea. :)

Status: Needs review » Needs work

The last submitted patch, test.unit_.84.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
1.5 KB
10.3 KB

Fixed bogus comments and variable names.

Lars Toomre’s picture

Since messages are user facing, my understanding is the messages should use format_string() rather than raw variable concatenation. Is my understanding wrong?

sun’s picture

FileSize
8.91 KB
17.89 KB

Fixed installation of multiple modules and DIC/Kernel integration.

Victory.

sun’s picture

Any 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.

sun’s picture

Priority: Normal » Critical
FileSize
1.58 KB
18.26 KB

#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.

  1. Merged HEAD.
  2. Updated for #1831350: Break the circular dependency between bootstrap container and kernel
chx’s picture

Note that I tried to convert the TaxonomyTermReferenceItemTest I submitted yesterday to this and the following two steps were required:

  1. Add system to the list of modules (this was unexpected)
  2. Add 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.

sun’s picture

I 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.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Fair enough, let's go ahead with this.

sun’s picture

FileSize
2.88 KB
18.43 KB

Minor/small comment tweaks to prevent this patch from getting blocked on those.

Fabianx’s picture

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -204,7 +204,7 @@ public function updateModules(array $module_list, array $module_paths = array())
     if ($this->booted) {
-      drupal_container(NULL, TRUE);
+      drupal_container($base_container, TRUE);
       $this->booted = FALSE;

Purely 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

chx’s picture

So 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.

catch’s picture

Title: Unit Tests Remastered™ » Change notice:Unit Tests Remastered™
Status: Reviewed & tested by the community » Active

Alright. Committed/pushed this one to 8.x. This will need a change notice for the new base class.

sun’s picture

Title: Change notice:Unit Tests Remastered™ » Unit Tests Remastered™
Priority: Critical » Normal
Status: Active » Fixed

Thanks!

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.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

effulgentsia’s picture

For 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.

plach’s picture

We 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.

Wim Leers’s picture

#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…

Sylvain Lecoy’s picture

If 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. :)

alexpott’s picture

alexpott’s picture

sun’s picture

Issue tags: +Test suite performance