Various tests in HEAD need to be corrected:

  1. Various code + test code depends on System module, but does not specify a dependency.

    (Because run-tests.sh itself requires functionality of System module, system.module is magically pre-loaded for all tests currently.)

  2. Various tests are calling enableModules() from setUp(), which causes an unnecessary container rebuild for every test method. Use public static $modules instead.

  3. Tons of tests are relying on overloaded class properties without declaring them.

  4. Test methods are always public. Some test methods are not.

  5. menu_link module is gone, but various tests still try to enable it.

  6. email module is gone for a long time already, but various tests still try to enable it.

  7. field_sql_storage module is gone for a long time already, but various tests still try to enable it.

CommentFileSizeAuthor
#15 test.bogus_.15.patch46.7 KBsun
#15 interdiff.txt3.14 KBsun
#8 test.bogus_.8.patch48.26 KBsun
#7 test.bogus_.7.patch44.65 KBsun
#5 test.bogus_.5.patch40.89 KBsun
#2 test.bogus_.2.patch40.89 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Issue summary: View changes
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Active » Needs review
FileSize
test.bogus_.2.patch40.89 KB

Status: Needs review » Needs work

The last submitted patch, 2: test.bogus_.2.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Issue summary: View changes
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
test.bogus_.5.patch40.89 KB

Status: Needs review » Needs work

The last submitted patch, 5: test.bogus_.5.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
test.bogus_.7.patch44.65 KB
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
test.bogus_.8.patch48.26 KB

Re-rolled against latest HEAD.

Also added PHPDoc for this previous hunk:

+++ b/core/modules/text/src/Tests/Formatter/TextPlainUnitTest.php
+  protected $entity_type;
+  protected $bundle;
+  protected $field_name;
+  protected $field_type;
+  protected $field_settings;
+  protected $instance_settings;
+  protected $formatter_type;
+  protected $formatter_settings;
+  protected $fieldStorage;
+  protected $instance;
+  protected $view_mode;
+  protected $display;
+  protected $langcode;
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Parent issue: » #2304461: KernelTestBaseTNG™
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Title: Lots of bogus tests » Fix various tests
Issue summary: View changes

Considering the fact that you've not removed a single test entitling this issue "Lots of bogus tests" is unnecessarily confrontational and aggressive.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Apologies, it wasn't my intention to make this sound confrontational or aggressive; language barriers - learn something new every day. :)

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Issue tags: +Quick fix

The size of this patch is a bit misleading; it contains trivial quick fixes only.

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented 
  1. +++ b/core/modules/block/src/Tests/BlockConfigSchemaTest.php
@@ -32,6 +32,7 @@ class BlockConfigSchemaTest extends KernelTestBase {
+    'system', // BlockManager->getModuleName() -> system_get_info()

+++ b/core/modules/comment/src/Tests/CommentStringIdEntitiesTest.php
@@ -29,6 +29,7 @@ class CommentStringIdEntitiesTest extends KernelTestBase {
+    'system', // EMAIL_MAX_LENGTH constant.

    would be nice to have comments in separate lines 

  2. +++ b/core/modules/rdf/src/Tests/Field/FieldRdfaTestBase.php
@@ -43,14 +43,19 @@
-  protected $debug = TRUE;
+  protected $debug = FALSE;

    cant see why this is related 

  3. +++ b/core/modules/system/src/Tests/File/DirectoryTest.php
@@ -30,8 +30,8 @@ function testFileCheckLocalDirectoryHandling() {
-    $parent_path = $directory . DIRECTORY_SEPARATOR . $parent;
-    $child_path = $parent_path . DIRECTORY_SEPARATOR . $child;
+    $parent_path = $directory . '/' . $parent;
+    $child_path = $parent_path . '/' . $child;

@@ -46,7 +46,7 @@ function testFileCheckLocalDirectoryHandling() {
-    $absolute_path = drupal_realpath($directory) . DIRECTORY_SEPARATOR . $this->randomMachineName() . DIRECTORY_SEPARATOR . $this->randomMachineName();
+    $absolute_path = drupal_realpath($directory) . '/' . $this->randomMachineName() . '/' . $this->randomMachineName();

    why getting rid of DIRECTORY_SEPARATOR ?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Looks mostly fine to me, not sure why in some places properties are converted to camel case and in others not.

I'd rather clean up TextPlainUnitTest and stop using most of those IMHO pretty useless properties, will probably attempt to do that in #2313757: Remove text_processing option from text fields, expose existing string field types as plain text in UI, which currently removes the test but as mentioned by @yched, there is some useful test coverage, just need to switch to string/string.

But I'm also OK with moving forward with this to unblock the other issue.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
interdiff.txt3.14 KB
test.bogus_.15.patch46.7 KB

Resolved #13 — Sorry for 2) + 3), those changes are only required for #2304461: KernelTestBaseTNG™ and not here.

@Berdir:

re: Property name casing: Yeah, but it was painful enough to locate and figure out all of these instances (which required a few days of work), so I didn't have the energy to additionally change the property names everywhere. IMO, this patch is large enough already. If we care enough about rigidly applying coding standards to tests (I don't), then we can adjust affected tests in separate follow-up issue(s).

re: TextPlainUnitTest: Yes, that one is ugly. Would be great to refactor that test in that follow-up issue, as you suggested.

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Status: Needs review » Reviewed & tested by the community

great cleanup, thanks

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs review 
  1. +++ b/core/modules/editor/src/Tests/QuickEditIntegrationTest.php
@@ -25,6 +25,11 @@
   /**
+   * {@inheritdoc}
+   */
+  public static $modules = array('editor', 'editor_test');
+
+  /**
    * The manager for editor plug-ins.
    *
    * @var \Drupal\Component\Plugin\PluginManagerInterface
@@ -65,9 +70,6 @@ public function setUp() {

@@ -65,9 +70,6 @@ public function setUp() {
     // Install the Filter module.
     $this->installSchema('system', 'url_alias');
 
-    // Enable the Text Editor and Text Editor Test module.
-    $this->enableModules(array('editor', 'editor_test'));
-

    Should we not also be enabling the system module too? 

  2. +++ b/core/modules/system/src/Tests/Cache/GenericCacheBackendUnitTestBase.php
@@ -94,7 +94,7 @@ public function tearDownCacheBackend() {
-  final function getCacheBackend($bin = null) {
+  protected function getCacheBackend($bin = null) {

    How come?

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community

re: 1) system module is already enabled by the base class of QuickEditIntegrationTest.

re: 2) Technically this belongs to #2304461: KernelTestBaseTNG™; methods declared as final cannot be invoked from ReflectionClass (PHPUnit invokes every test method via ReflectionClass). There's no need for this test class method to be final.

Tentatively moving back to RTBC.

alexpott’s picture

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

Committed f9c1ff2 and pushed to 8.0.x. Thanks!

  • alexpott committed f9c1ff2 on 8.0.x 
    Issue #2314123 by sun: Fixed various tests.
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Closely related: #2322889: Various setUp() and tearDown() methods are not protected

Status: Fixed » Closed (fixed)

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