| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | wscci |
| Category: | task |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
As I commented in the bundles thread...
If you were to call drupal_container() from within a WebTestCase test, you would get the container of the main environment, not the test environment. So, if using run_tests.sh all that would be in it would be the config system services. However, when an actual request is made, e.g. using drupalGet() or drupalPost(), the full container for the test environment gets booted up for that request.
This is bad. The tests should have access to a fully-loaded DIC, with the particular services required by the test environment. Here's a first pass at providing this. Unfortunately it makes drupal_container() even uglier than it already was :-/
Also, I can't figure out how the testing container ends up having a 'request' service - it shouldn't, because that only ever gets set by HttpKernel when it's handling a request, but the testing kernel never handles a request. I am confused. Anyway, that is the only reason for the change in path.inc (because now this also fixes the problem described in #1719936: Theme API tests and Theme functions tests broken if run in a browser).
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| webtestbase_kernel.patch | 4.88 KB | Idle | FAILED: [[SimpleTest]]: [MySQL] 39,804 pass(es), 3 fail(s), and 0 exception(s). | View details |
Comments
#1
The last submitted patch, webtestbase_kernel.patch, failed testing.
#2
Hmm, try this...
#3
#4
I tried it with Crell's failing patch from #1606794-23: Implement new routing system, turns out the testing kernel is needed earlier than where I had it or stuff still explodes. Made that change plus some other minor fixes.
#5
The last submitted patch, 1727538.webtestcase_kernel.4.patch, failed testing.
#6
OK, hopefully this will get us back to green.
#7
Why don't we simply save the old container in setUp() and restoring in tearDown()?
The approach of keeping stuff around from the parent environment in the child environment in this patch is very at odd with how we handle the rest of the environment switching in the testing framework. Also, if I read this correctly, you are shutting down the testing kernel in the parent environment, not in the child, which is also really odd (which could lead to stuff like caches being set or flushed in the incorrect environment).
#8
Ugh, yes it is quite possible I have over-complicated things here.
I guess we just need to make sure that the only difference between the DIC available from within the body of a test and the DIC being used when a drupalGet or drupalPost request is made, is that the latter has a request service (and therefore the request scope is active).
I'm not quite seeing what you mean - we create the kernel during setUp and destroy it during tearDown - why is this odd?
Anyway, I'll try to rework it tonight, unless someone else wants to have a go...
#9
Given the number of things this affects...
#10
Much simpler, but should achieve exactly the same result...
#11
The last submitted patch, 1727538.webtests.10.patch, failed testing.
#12
#13
To illustrate why we need this, I've changed the bundle test in this latest patch so that it is just asserting that the 'bundle_test_class' service exists, rather than what it currently does which is pretty crazy: it makes a drupalGet() request to a menu callback that checks the container for the service in question.
Also attaching that change to the bundles test as a separate patch to show that that test fails without the change introduced in this patch.
#14
The last submitted patch, bundle_test.patch, failed testing.
#15
Tempted to mark this rtbc...
#16
I'll succumb to temptation. :-) #13 looks good to me. Thanks, Kat!
#17
If I'm not mistaken, we are still shutting down the testing container while back inside the parent environment (with the parent database active, etc.):
<?php
// Rebuild caches.
$this->refreshVariables();
+ // Destroy the testing kernel.
+ if (isset($this->kernel)) {
+ $this->kernel->shutdown();
+ }
?>
This needs to move way higher in the tearDown() sequence.
#18
Going on the logic that tearDown() should done in the reverse order to setUp() I've moved the $this->kernel->shutdown(); to just before the simpletest database tables are deleted.
Also I can confirm that this patch fixes #1719936: Theme API tests and Theme functions tests broken if run in a browser
#19
#20
#1719936: Theme API tests and Theme functions tests broken if run in a browser is a critical and is postponed on this, so bumping this to critical.
#21
+++ b/core/includes/bootstrap.incundefined@@ -2644,7 +2644,7 @@ function language($type, $reset = FALSE) {
- if (drupal_container()->has('language_manager')) {
+ if (drupal_container()->isScopeActive('request')) {
So now we are use that if we are in the request scope then language_manager service exists?
+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined@@ -656,6 +657,12 @@ abstract class WebTestBase extends TestBase {
+ $kernel = new DrupalKernel('testing', FALSE);
I miss the wall of test explaining that while the 'prod' DrupalKernel in index.php has a request scope somehow the 'testing' kernel somehow doesn't and how this affects the DIC and so on.
+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined@@ -656,6 +657,12 @@ abstract class WebTestBase extends TestBase {
+ $kernel->boot();
The kernel is not booted in index.php. Is it necessary here?
#22
Oh and...
#23
Oh, per #13 it doesnt need tests. Thanks aspilicious for pointing this out. Still the black magic needs commenting.
#24
Yes - the language_manager service is only available within the scope of a request and this is a much more reliable method for checking that. I had noticed that checking for
$container->has('request')was returning TRUE even when no request was actually being handled.The names 'prod' and 'testing' don't actually get used by anything right now - we could put 'ihazakernel' in here and it wouldn't make any difference to anything. However, we will probably start to make use of this when we get to compiling the container, and I imagine we'd want to avoid dumping the testing container to disk.
When the kernel handles a request, a check is made to see whether it is booted and, if not, it gets booted. We don't want to explicitly boot it because the page might end up being served from the cache (but this only matters when we move to using Symfony's HttpCache). But in the case of the testing kernel, it's never actually going to handle a request, so we need to boot it explicitly.
Let me know if you feel any of this warrants comments in the code.
#25
> the language_manager service is only available within the scope of a request
But is it available always?
> somehow the 'testing' kernel somehow doesn't
This is not explained in your followup nor in code comment. Basically, I am asking why does your patch work??
#26
OK, I get it, thanks to the brief explanation by kat in IRC and a little digging in core. language_manager is registered byt the CoreBundle which also activates the request scope. The rest is explained in the patch. I daresay this is ready: I only added comments.
#27
Tweaked the second paragraph more.
#28
beejeebus tuned it further.
#29
RTBC +1 for the changes in #28
#30
I confess I don't quite follow this patch, but it has +1s from the right people and yay for unblocking testing. :)
Committed and pushed to 8.x. Yay!
#31
Automatically closed -- issue fixed for 2 weeks with no activity.