Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dmitrii’s picture

Assigned: Unassigned » dmitrii
dmitrii’s picture

Assigned: dmitrii » Unassigned
Status: Active » Needs review
FileSize
4.49 KB
andypost’s picture

+++ b/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.phpundefined
@@ -88,7 +88,7 @@ protected function setUp() {
+    $this->containerBuild(\Drupal::getContainer());

@@ -156,7 +156,7 @@ public function containerBuild(ContainerBuilder $container) {
+      // \Drupal::getContainer() which is somewhat the mirror of the empty

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.phpundefined
@@ -862,7 +862,7 @@ protected function prepareEnvironment() {
+    $this->originalContainer = clone \Drupal::getContainer();

@@ -963,7 +963,7 @@ protected function prepareConfigDirectories() {
+   * Rebuild \Drupal::getContainer().

@@ -981,9 +981,9 @@ protected function prepareConfigDirectories() {
-    // DrupalKernel replaces the container in drupal_container() with a
+    // DrupalKernel replaces the container in \Drupal::getContainer() with a
...
+    $this->container = \Drupal::getContainer();

@@ -1026,7 +1026,7 @@ protected function tearDown() {
+    if (($container = \Drupal::getContainer()) && $container->has('keyvalue')) {

You should always use internal container $this->container in tests

andypost’s picture

Issue tags: +CodeSprintUA

tagged for rocketship

Berdir’s picture

This is the thing that we're creating the internal container with, so that correctly uses Drupal, $this->container = $this->container isn't going to work very well ;), with the exception of the last one maybe about keyvalue.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.phpundefined
@@ -963,7 +963,7 @@ protected function prepareConfigDirectories() {
-   * Rebuild drupal_container().
+   * Rebuild \Drupal::getContainer().

I'd just say "Rebuild the container". You can't rebuild a method or function :)

dmitrii’s picture

Status: Needs review » Needs work

The last submitted patch, drupal_container-2014015-6.patch, failed testing.

kgoel’s picture

Status: Needs work » Needs review
FileSize
7.43 KB

Status: Needs review » Needs work

The last submitted patch, simpletest-2014015-8.patch, failed testing.

kgoel’s picture

Status: Needs work » Needs review
FileSize
3.51 KB

Status: Needs review » Needs work

The last submitted patch, simpletest-2014015-10.patch, failed testing.

kgoel’s picture

Status: Needs work » Needs review
FileSize
708 bytes
3.51 KB

Status: Needs review » Needs work

The last submitted patch, simpletest-2014015-12.patch, failed testing.

kgoel’s picture

FileSize
713 bytes
3.51 KB

Ah, that was very stupid of me.

kgoel’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, simpletest-2014015-14.patch, failed testing.

kgoel’s picture

Status: Needs work » Needs review
FileSize
496 bytes
3.71 KB

and again....

Status: Needs review » Needs work
Issue tags: -Novice, -CodeSprintUA

The last submitted patch, simpletest-2014015-17.patch, failed testing.

dmitrii’s picture

Status: Needs work » Needs review
Issue tags: +Novice, +CodeSprintUA

#6: drupal_container-2014015-6.patch queued for re-testing.

kgoel’s picture

I have looked into it a bit more and found that Drupal::getContainer is deprecated. My guess would be to use Drupal::$container but I want someone else to confirm before I post another silly patch.

Berdir’s picture

Drupal::getContainer() is correct, that's just a documentation bug :)

kgoel’s picture

@Berdir - any idea why I am getting this fatal error - Undefined class constant 'getContainer' ?

Berdir’s picture

Status: Needs review » Needs work
+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.phpundefined
@@ -879,7 +880,7 @@ protected function prepareEnvironment() {
-    $this->originalContainer = clone drupal_container();
+    $this->originalContainer = clone \Drupal::getContainer;

Because it's a method, not a constant: \Drupal::getContainer();

kgoel’s picture

Status: Needs work » Needs review
FileSize
716 bytes
3.71 KB

Status: Needs review » Needs work

The last submitted patch, simpletest-2014015-24.patch, failed testing.

kgoel’s picture

So the patch failed and error seems to be here -

     simpletest_log_read($this->testId, $this->databasePrefix, get_class($this), TRUE);
-    if (($container = drupal_container()) && $container->has('keyvalue')) {
+    if (($container = $this->container) && $container->has('keyvalue')) {
       $captured_emails = \Drupal::state()->get('system.test_email_collector') ?: array();
       $emailCount = count($captured_emails);

error message in review log is - 'The service definition "state" does not exist. Does this mean it needs a create method to inject keyvalue properly?

Berdir’s picture

Have a look at #1786490: Add caching to the state system, I had to fix that too there, the problem is that it's called too late. While you move it around, please change the container id it is checking from keyvalue to state, as that's what it is using.

Also, if you check in $this->container, you also want to call $this->container->get('state')->get().

Berdir’s picture

Double post.

kgoel’s picture

Status: Needs work » Needs review
FileSize
2.48 KB
4.82 KB

Status: Needs review » Needs work

The last submitted patch, simpletest-2014015-29.patch, failed testing.

kgoel’s picture

Status: Needs work » Needs review
FileSize
740 bytes
4.82 KB

Status: Needs review » Needs work

The last submitted patch, simpletest-2014015-31.patch, failed testing.

kgoel’s picture

Status: Needs work » Needs review
FileSize
569 bytes
4.79 KB

Hopefully this is it.

Status: Needs review » Needs work

The last submitted patch, simpletest-2014015-33.patch, failed testing.

kgoel’s picture

@Berdir - I have followed what you recommended under comment but it seems like same error I was getting before.

kgoel’s picture

Status: Needs work » Needs review
FileSize
3.51 KB

Status: Needs review » Needs work

The last submitted patch, simpletest-2014015-36.patch, failed testing.

kgoel’s picture

Status: Needs work » Needs review
FileSize
3.51 KB

Status: Needs review » Needs work

The last submitted patch, simpletest-2014015-38.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: -Novice
FileSize
4.3 KB

That's what I meant, let's try this. And yes, this is not a Novice issue :)

Crell’s picture

#40: simpletest-2014015-40.patch queued for re-testing.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

#40 seems OK to me visually if the bot still approves. Mr. Roboto can CNW it if it wants.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

webchick’s picture

Issue summary: View changes

add link to related task

Status: Fixed » Closed (fixed)

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