Problem/Motivation

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#8 interdiff.txt42.04 KBdawehner
#8 2684095-8.patch71.2 KBdawehner
#6 interdiff.txt8.52 KBdawehner
#6 2684095-6.patch50.3 KBdawehner
#4 interdiff.txt8.11 KBdawehner
#4 2684095-4.patch47.96 KBdawehner
#2 2684095-2.patch47.73 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner created an issue. See original summary.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Assigned: dawehner » Unassigned
Status: Active » Needs review
FileSize
2684095-2.patch47.73 KB

This contains #2605956: Port #2605684 to the new KernelTest at the moment.

While working on this issue, #2684123: \Drupal\Core\Routing\RouteProvider::preLoadRoutes doens't handle the exception case correctly popped up.

Status: Needs review » Needs work

The last submitted patch, 2: 2684095-2.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs work » Needs review
FileSize
2684095-4.patch47.96 KB
interdiff.txt8.11 KB

Some less failures, I guess

Status: Needs review » Needs work

The last submitted patch, 4: 2684095-4.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs work » Needs review
FileSize
2684095-6.patch50.3 KB
interdiff.txt8.52 KB

There we go.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Needs review » Needs work 
  1. --- a/core/modules/comment/src/Tests/CommentItemTest.php
+++ b/core/modules/comment/src/Tests/CommentItemTest.php

--- a/core/modules/datetime/src/Tests/DateTimeItemTest.php
+++ b/core/modules/datetime/src/Tests/DateTimeItemTest.php

    These two are missing a rename and a namespace change. 

  2. +++ b/core/modules/field/tests/src/Kernel/FieldImportChangeTest.php
@@ -20,7 +21,7 @@ class FieldImportChangeTest extends FieldUnitTestBase {
+   * \Drupal\Tests\field\Kernel\FieldUnitTestBase::setUp() when it installs field
    * configuration.

    This comment needs to be re-wrapped to 80 cols. 

  3. +++ b/core/modules/field/tests/src/Kernel/FieldImportCreateTest.php
@@ -97,8 +97,8 @@ function testImportCreate() {
-    $target_dir = $this->configDirectories[CONFIG_SYNC_DIRECTORY];
...
+    $target_dir = config_get_config_directory(CONFIG_SYNC_DIRECTORY);

    \Drupal\simpletest\KernelTestBase::prepareConfigDirectories() was doing a bunch of things, including a sanity check that the config directory exists and is writable.

    We don't need that anymore with the phpunit-based test class? 

  4. +++ b/core/modules/field/tests/src/Kernel/FieldImportDeleteTest.php
@@ -22,7 +23,7 @@ class FieldImportDeleteTest extends FieldUnitTestBase {
+   * \Drupal\Tests\field\Kernel\FieldUnitTestBase::setUp() when it installs field
    * configuration.

    Another one :) 

  5. +++ b/core/modules/field/tests/src/Kernel/FieldUnitTestBase.php
@@ -2,17 +2,17 @@
- * Contains \Drupal\field\Tests\FieldUnitTestBase.
+ * Contains \Drupal\Tests\field\Kernel\FieldUnitTestBase.

    Shouldn't this be called FieldKernelTestBase? 

  6. +++ b/core/modules/field/tests/src/Kernel/FieldValidationTest.php
@@ -2,10 +2,11 @@
+namespace Drupal\Tests\field\Kernel;
+use Drupal\Tests\field\Kernel\FieldUnitTestBase;

    Missing empty line. 

  7. +++ b/core/modules/field/tests/src/Kernel/FormatterPluginManagerTest.php
@@ -2,12 +2,13 @@
+namespace Drupal\Tests\field\Kernel;
 use Drupal\Core\Field\BaseFieldDefinition;

    Same here. 

  8. +++ b/core/modules/field/tests/src/Kernel/TranslationTest.php
@@ -193,7 +194,7 @@ function testTranslatableFieldSaveLoad() {
-        $this->assertEqual($entity->getTranslation($langcode)->{$field_name_default}->getValue(), $empty_items, format_string('Empty value correctly populated for language %language.', array('%language' => $langcode)));
+        $this->assertEquals([], $entity->getTranslation($langcode)->{$field_name_default}->getValue(), format_string('Empty value correctly populated for language %language.', array('%language' => $langcode)));

    $empty_items can also be NULL, why are we dropping that assertion? 

  9. --- a/core/modules/file/src/Tests/FileItemTest.php
+++ b/core/modules/file/src/Tests/FileItemTest.php

--- a/core/modules/image/src/Tests/ImageItemTest.php
+++ b/core/modules/image/src/Tests/ImageItemTest.php

--- a/core/modules/link/src/Tests/LinkItemTest.php
+++ b/core/modules/link/src/Tests/LinkItemTest.php

--- a/core/modules/options/src/Tests/OptionsFieldUnitTestBase.php
+++ b/core/modules/options/src/Tests/OptionsFieldUnitTestBase.php

+++ b/core/modules/options/src/Tests/OptionsFieldUnitTestBase.php
@@ -8,13 +8,13 @@
+abstract class OptionsFieldUnitTestBase extends \Drupal\Tests\field\Kernel\FieldUnitTestBase {

--- a/core/modules/rdf/src/Tests/Field/FieldRdfaTestBase.php
+++ b/core/modules/rdf/src/Tests/Field/FieldRdfaTestBase.php

--- a/core/modules/telephone/src/Tests/TelephoneItemTest.php
+++ b/core/modules/telephone/src/Tests/TelephoneItemTest.php

--- a/core/modules/text/src/Tests/TextWithSummaryItemTest.php
+++ b/core/modules/text/src/Tests/TextWithSummaryItemTest.php

    Missing file move and namespace update. 

  10. +++ b/core/modules/rdf/src/Tests/Field/FieldRdfaTestBase.php
@@ -45,7 +45,7 @@
-  protected $debug = TRUE;
+  protected $debug = FALSE;

    Any reason for this change?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs work » Needs review
FileSize
2684095-8.patch71.2 KB
interdiff.txt42.04 KB

Thanks a lot for this super quick review @amateescu!

These two are missing a rename and a namespace change.

To be clear, kernel tests afaik still run when they are in the old directory, as they are discovered by our custom test scanning, but you are absolute right, let's move them all.

\Drupal\simpletest\KernelTestBase::prepareConfigDirectories() was doing a bunch of things, including a sanity check that the config directory exists and is writable.

We don't need that anymore with the phpunit-based test class?

Well, we ensure that the file directory exists, see 

    mkdir($this->siteDirectory . '/files', 0775);
    mkdir($this->siteDirectory . '/files/config/' . CONFIG_SYNC_DIRECTORY, 0775, TRUE);
Shouldn't this be called FieldKernelTestBase?

Good point!

Another one :)

Is there any chance to configure that in PHPSTorm automatically?

$empty_items can also be NULL, why are we dropping that assertion?

Well, its always an empty array, when you execute it. PHPUnit is a bit more strict on assertEquals by default

Any reason for this change?

Well, phpstorm doesn't allow one to provide logging like that by default ...

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Needs review » Reviewed & tested by the community

Well, phpstorm doesn't allow one to provide logging like that by default ...

I think you mean phpunit, not phpstorm :) And that's a shame really, maybe we should try to add some debugging helpers in our base test classes or, why not, directly upstream. But certainly not in this issue.

The patch looks good to me now, let's hope the testbot agrees.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

Awesome!

Well, I'm not sure about the point. You can just use your debugger and call it a day. For BrowserTestBase

But sure, let's discuss that in a follow up.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed c2f6eb6 and pushed to 8.1.x. Thanks!

  • alexpott committed daa52e9 on 8.2.x 
    Issue #2684095 by dawehner, amateescu: Convert field kernel tests to...

Status: Fixed » Closed (fixed)

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