Problem/Motivation

It's standard practice in PHPUnit tests to mark tests that don't make an assertion as risky. This is because without making any assertions you are not really testing any assumptions beyond any code being executed is not triggering an error.

However at the moment the \Drupal\KernelTests\KernelTestBase ensures that there is always one assertion. Because it make one of every tests behalf.

Proposed resolution

Remove the assertion so that issues like #2942529: Text Editor module assumes that a fieldable entity is also revisionable aren't tempted to add a test with no assertions.

With this fix running the test in #2942529-23: Text Editor module assumes that a fieldable entity is also revisionable results in the following output.

There was 1 risky test:

1) Drupal\Tests\editor\Kernel\EditorEntityTest::testEditorFunctions
This test did not perform any assertions

OK, but incomplete, skipped, or risky tests!
Tests: 1, Assertions: 0, Risky: 1.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

Previously \Drupal\KernelTests\KernelTestBase always made one assertion which would hide tests that did not preform any assertions themselves. KernelTestBase will now no longer perform one assertion by default which means that kernel tests that don't perform any assertions will now be marked as risky and will fail on Drupal CI.

CommentFileSizeAuthor
#39 3059332-39.patch11.6 KBmondrake
#39 interdiff_38-39.txt817 bytesmondrake
#38 interdiff_36-38.txt2.75 KBmondrake
#38 3059332-38.patch11.59 KBmondrake
#36 3059332-36.patch11.64 KBmondrake
#32 3059332-32.patch16.32 KBLendude
#28 interdiff.txt582 bytesMile23
#28 3059332_28.patch19.65 KBMile23
#25 3059332-25.patch19.4 KBalexpott
#18 3059332-18.patch30.24 KBalexpott
#18 16-18-interdiff.txt867 bytesalexpott
#16 3059332-16.patch30.26 KBalexpott
#16 13-16-interdiff.txt7.02 KBalexpott
#13 3059332-13.patch28.79 KBalexpott
#13 11-13-interdiff.txt900 bytesalexpott
#11 3059332-11.patch28.8 KBalexpott
#11 8-11-interdiff.txt6.8 KBalexpott
#8 3059332-7.patch22.01 KBalexpott
#8 5-7-interdiff.txt7.2 KBalexpott
#5 3059332-5.patch14.8 KBalexpott
#5 2-5-interdiff.txt12.58 KBalexpott
#5 29-32-interdiff.txt2.49 KBalexpott
#2 3059332-2.patch2.22 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Title: Mark Kernel tests that perform no assertions as risky » Mark kernel tests that perform no assertions as risky
Status: Active » Needs review
FileSize
2.22 KB

I've tried to cause this to occur but even reregistering shutdown functions in shutdown functions doesn't cause the assertion to fail. The only other way this could fail is if we introduce a shutdown function into the kernel shutdown method. But that really does not make sense - here's the current implementation.

  /**
   * {@inheritdoc}
   */
  public function shutdown() {
    if (FALSE === $this->booted) {
      return;
    }
    $this->container->get('stream_wrapper_manager')->unregister();
    $this->booted = FALSE;
    $this->container = NULL;
    $this->moduleList = NULL;
    $this->moduleData = [];
  }

So I've decided to remove the code entirely and add a test that KTB does call shutdown functions. And that shutdown functions registered in shutdown functions are called as expected.

alexpott’s picture

If we want we can replace the original assertion with something like

    // Fail in case any (new) shutdown functions exist. Note that this is not an
    // assertion to ensure that kernel tests with no assertions are marked as
    // risky.
    if (count(drupal_register_shutdown_function()) !== 0) {
      throw new \RuntimeException('Unexpected Drupal shutdown callbacks exist after running shutdown functions.');
    }

But I couldn't work out how to test this or cause this - see #2.

Status: Needs review » Needs work

The last submitted patch, 2: 3059332-2.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.49 KB
12.58 KB
14.8 KB

Well that's revealing.

Some stuff will still fail... working through it.

Status: Needs review » Needs work

The last submitted patch, 5: 3059332-5.patch, failed testing. View results

Mile23’s picture

alexpott’s picture

Priority: Normal » Critical
Status: Needs work » Needs review
FileSize
7.2 KB
22.01 KB

Some more fixes. I guess I'm going to break out some of these into their own issue.

I think this issue is probably critical as we think we have test coverage of things we don't currently have test coverage of.

Mile23’s picture

I can't quite suss why we care about the number of error handlers, because in assertPostCondition() we already executed all of them. Do we think that maybe calling $this->container->get('kernel')->shutdown() added some when it shouldn't?

Also, assuming we want to leave this all in place, the way to do it without adding an assertion is:

    // Fail in case any (new) shutdown functions exist.
    if (count(drupal_register_shutdown_function()) !== 0) {
      throw new \PHPUnit\Framework\ExpectationFailedException('Unexpected Drupal shutdown callbacks exist after running shutdown functions.');
    }

Also it's weird that in assertPostCondition() we call $this->container->get('kernel')->shutdown(), and then in tearDown() we call:

    if (isset($this->kernel)) {
      $this->kernel->shutdown();
    }

...particularly when there's not a $this->kernel.

Status: Needs review » Needs work

The last submitted patch, 8: 3059332-7.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
6.8 KB
28.8 KB

So the deprecation fails are interesting. These are tricky because the test listener is supposed to take care of this. Doesn't work though because of test isolation. There's not really a simple way to change this or fix this as far as I can see. I will raise an upstream issue about this.

re #9 it's not error handlers it's shutdown functions and what we're trying to achieve is that all shutdown functions have been run before the test is complete and therefore are run as part of the test. However as pointed out by #2 its not really possible that extra shutdown functions are registered.

Status: Needs review » Needs work

The last submitted patch, 11: 3059332-11.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
900 bytes
28.79 KB

Whoops. Yay for deprecation testing!

Mile23’s picture

"Yay for deprecation testing!" Famous last words. :-)

  1. +++ b/core/modules/field/tests/src/Kernel/FieldDefinitionIntegrityTest.php
    @@ -168,6 +168,12 @@ protected function checkDisplayOption($entity_type_id, $field_id, BaseFieldDefin
    +        $this->pass(sprintf(
    

    pass() comes from AssertLegacyTrait, which isn't such a huge sin.

    I'd suggest $this->assertNotEmpty($plugin_manager->getDefinition($display_options['type']), 'Plugin found...'); since the fail state here is an empty array from getDefinition().

  2. +++ b/core/modules/views/tests/src/Kernel/Handler/FieldCounterTest.php
    @@ -88,6 +88,7 @@ public function testSimple() {
    +    $this->markTestSkipped('@todo: Write tests for pager');
    

    Needs an issue.

  3. +++ b/core/tests/Drupal/KernelTests/Core/Config/Storage/CachedStorageTest.php
    @@ -44,7 +44,7 @@ protected function setUp() {
    +    $this->markTestSkipped('No-op as this test does not make sense');
    

    Needs an issue to remove, maybe? It comes from here: #2263287: Test the CachedStorage class using ConfigStorageTestBase

  4. +++ b/core/tests/Drupal/KernelTests/Core/Config/Storage/StorageReplaceDataWrapperTest.php
    @@ -55,7 +55,7 @@ protected function delete($name) {
    +    $this->markTestSkipped('No-op as this test does not make sense');
    

    Needs an issue to remove? Comes from here: #2740983: Configuration system doesn't allow importing a single item from a non-default collection Copypasta from the previous one or vice-versa?

  5. +++ b/core/tests/Drupal/KernelTests/KernelTestBaseTest.php
    @@ -16,6 +16,20 @@
    +  /**
    +   * Indicates testing shutdown functions.
    +   *
    +   * @var bool
    +   */
    +  protected $shutdownTest = FALSE;
    

    If it's me, this test gets its own test class so its readable, but that's a style nit type thing.

Lendude’s picture

Hard to review if all the changes are actually needed with everything in one big patch, but splitting this up into tiny bits doesn't seem very useful either ¯\_(ツ)_/¯. Some things I see:

+++ /dev/null
@@ -1,62 +0,0 @@
-class FilterSettingsTest extends KernelTestBase {
...
-    foreach ($filter_defaults_format->filters() as $name => $filter) {
...
-    foreach ($filter_defaults_format->filters() as $name => $filter) {

Removing FilterSettingsTest completely seems a little excessive, the foreach both need to be

foreach ($filter_defaults_format->filters()->getAll() as $name => $filter) {

Which will then actually test what it should I believe

+++ b/core/modules/node/tests/src/Kernel/Action/UnpublishByKeywordActionTest.php
@@ -13,19 +16,55 @@ class UnpublishByKeywordActionTest extends KernelTestBase {
+    // Install system

add a period at the end or maybe just leave the comment out.

#14.3 and #14.4, not sure we can remove those since they override test cases in the parent class, so this is their way of disabling those cases for these specific scenarios. I do believe the message can be updated to reflect this better, something like 'No-op as this test does not make sense for this setup.', or something along those line. Or maybe add a comment to clear this up.

alexpott’s picture

Title: Mark kernel tests that perform no assertions as risky » [PP-2] Mark kernel tests that perform no assertions as risky
Related issues: +#3059543: Remove \Drupal\Tests\filter\Kernel\FilterSettingsTest it tests nothing, +#3059545: Improve \Drupal\KernelTests\Config\DefaultConfigTest to install all optional configuration
FileSize
7.02 KB
30.26 KB

I'm going to carve 2 things from this into other issues. To keep the scope a bit easier.

Thanks for the reviews @Mile23 and @Lendude
Re #14

  1. See attached patch for a much nicer resolution.
  2. We should remove this method we don't add empty methods to denote todos for test coverage.
  3. This comes from the test harness in \Drupal\KernelTests\Core\Config\Storage\ConfigStorageTestBase but this particular storage can never be invalid so we have to skip.
  4. This comes from the test harness in \Drupal\KernelTests\Core\Config\Storage\ConfigStorageTestBase but this particular storage can never be invalid so we have to skip.
  5. Makes sense. Done in attached patch.

Re #15
See #3059543: Remove \Drupal\Tests\filter\Kernel\FilterSettingsTest it tests nothing
Added the fullstop in UnpublishByKeywordActionTest

Note the patch attached still includes the blocking issues so we can see that we've completed the entire task.

Status: Needs review » Needs work

The last submitted patch, 16: 3059332-16.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
867 bytes
30.24 KB
Lendude’s picture

Title: [PP-2] Mark kernel tests that perform no assertions as risky » [PP-1] Mark kernel tests that perform no assertions as risky

Question: how can we do this without causing (major) disruption in contrib? Looking at the number of fails in core, I think it is safe to say that committing this is going to break stuff in contrib tests.

Mile23’s picture

PHPUnit CLI (6.5+) can say --dont-report-useless-tests.

So we could leave phpunit.xml.dist as it is, and then rig up run-tests.sh to have a flag for whether to say --dont-report-useless-tests on PHPUnit CLI. DrupalCI could force this flag for contrib much the way it does now for deprecation errors.

Or we could just enforce it here in 8.8.x with no backport to 8.7.x, and send up flares so people could try out their tests.

alexpott’s picture

Issue summary: View changes
Issue tags: +Needs change record

Question: how can we do this without causing (major) disruption in contrib? Looking at the number of fails in core, I think it is safe to say that committing this is going to break stuff in contrib tests.

Yes but, much like core this is going to break in a good way. It will remove the sense of false confidence the current test is giving the author. We can add a change record and and release note mention so people are forewarned and only put this into 8.8.x.

Mile23’s picture

Or we could just enforce it here in 8.8.x with no backport to 8.7.x, and send up flares so people could try out their tests.

Or, you know, backport the improved tests without changing 8.7.x's KernelTestBase.

Mile23’s picture

Title: [PP-1] Mark kernel tests that perform no assertions as risky » Mark kernel tests that perform no assertions as risky

Issues from #16 are in.

borisson_’s picture

Or we could just enforce it here in 8.8.x with no backport to 8.7.x, and send up flares so people could try out their tests.

Or, you know, backport the improved tests without changing 8.7.x's KernelTestBase.

I think this solution makes most sense. Adding the test improvements to reduce how much the two versions diverge.

I also think that it might be a good idea to ask/notify in #maintainers on slack so that people know this is coming up.

alexpott’s picture

Rerolled.

Mile23’s picture

+++ b/core/modules/views/tests/src/Kernel/Handler/FieldCounterTest.php
@@ -84,10 +84,4 @@ public function testSimple() {
-  /**
-   * @todo: Write tests for pager.
-   */
-  public function testPager() {
-  }

Harkening back to #14, I can't find any test that directly covers views counters with pagers, so that's why I mentioned this needs an issue. We clearly meant to add that test, and Counter::getValue() overrides the parent method.

Hmm.... Research says this @todo started life in 2013. :-) #1872876: Turn role permission assignments into configuration.

Please add a @todo in the class docblock for #3063179: Add test coverage for Drupal\views\Plugin\views\field\Counter::getValue() that I just filed.

The rest LGTM.

Still needs a CR.

Lendude’s picture

Mile23’s picture

Status: Needs work » Needs review
FileSize
19.65 KB
582 bytes

Adding @todo per #26. This is a docblock-only change.

Lendude’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

All blockers are in, follow-ups are filed, CR added, I think this is ready. Added release note snippet to the IS.

catch’s picture

Version: 8.8.x-dev » 8.7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

I think this is fine to commit to 8.8.x - it might result in some contrib tests failing but in a way that's easy to fix.

For 8.7.x agreed with backporting the individual test improvements only.

Committed 6abefc5 and pushed to 8.8.x. Thanks!

  • catch committed 6abefc5 on 8.8.x
    Issue #3059332 by alexpott, Mile23, Lendude: Mark kernel tests that...
Lendude’s picture

Status: Patch (to be ported) » Needs review
FileSize
16.32 KB

Here is a 8.7.x version

+++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
@@ -588,9 +588,6 @@ protected function assertPostConditions() {
-    // Fail in case any (new) shutdown functions exist.
-    $this->assertCount(0, drupal_register_shutdown_function(), 'Unexpected Drupal shutdown callbacks exist after running shutdown functions.');
-

+++ b/core/tests/Drupal/KernelTests/KernelTestBaseShutdownTest.php
@@ -0,0 +1,75 @@
+class KernelTestBaseShutdownTest extends KernelTestBase {

without these changes

mondrake’s picture

Version: 8.7.x-dev » 8.8.x-dev
Status: Needs review » Needs work

Postgres https://www.drupal.org/pift-ci-job/1334205 and SQLite https://www.drupal.org/pift-ci-job/1334206 test runs now have failures on risky tests, is this the culprit?

mondrake’s picture

Assigned: Unassigned » mondrake

Working on a fix

Mile23’s picture

Both tests use if() to make sure the tests are for MySQL. So each of those need an else that says markTestAsSkipped().

mondrake’s picture

Assigned: mondrake » Unassigned
Status: Needs work » Needs review
FileSize
11.64 KB

Let's try and do sth more sophisticated, since these tests are only valid for MySQL. This patch is introducing a namespace for driver-specific tests, and moving the two tests to the 'mysql' driver tests namespace. IMHO this is how it should be in OOP world...

Done some more cleanup of old simpletest methods like ::verbose and ::assertEqual.

Mile23’s picture

+++ b/core/tests/Drupal/KernelTests/Core/Database/Driver/mysql/LargeQueryTest.php
@@ -0,0 +1,48 @@
+    if (Environment::checkMemoryLimit($max_allowed_packet + (16 * 1024 * 1024))) {
...
+    else {
+      $this->markTestSkipped('The configured max_allowed_packet exceeds the php memory limit. Therefore the test is skipped.');
+    }

We really should say if ( skip_condition ) { skip } at the top, so it's easy to read.

The rest is +1. :-)

mondrake’s picture

mondrake’s picture

Mile23’s picture

Status: Needs review » Reviewed & tested by the community

Thanks.

LGTM.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 0dd7576 and pushed to 8.8.x. Thanks!

+++ b/core/tests/Drupal/KernelTests/Core/Database/Driver/mysql/MySqlDriverTestBase.php
@@ -0,0 +1,26 @@
+<?php
+
+namespace Drupal\KernelTests\Core\Database\Driver\mysql;
+
+use Drupal\Core\Database\Database;
+use Drupal\KernelTests\Core\Database\DatabaseTestBase;
+
+/**
+ * Base class for MySql driver-specific database tests.
+ */
+class MySqlDriverTestBase extends DatabaseTestBase {
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function setUp() {
+    parent::setUp();
+
+    // Only run this test for the 'mysql' driver.
+    $driver = $this->connection->driver();
+    if ($driver !== 'mysql') {
...
+    }
+  }
+
+}

I'm not a huge fan of this. It's unnecessary class hierarchy making people think. And we have to remember to use it in future and check if anything should be using it now. But fixing the test fails is more important. I might file an issue to refactor this.

diff --git a/core/tests/Drupal/KernelTests/Core/Database/Driver/mysql/MySqlDriverTestBase.php b/core/tests/Drupal/KernelTests/Core/Database/Driver/mysql/MySqlDriverTestBase.php
index 16c6a73a6c..abff7d07ab 100644
--- a/core/tests/Drupal/KernelTests/Core/Database/Driver/mysql/MySqlDriverTestBase.php
+++ b/core/tests/Drupal/KernelTests/Core/Database/Driver/mysql/MySqlDriverTestBase.php
@@ -2,7 +2,6 @@
 
 namespace Drupal\KernelTests\Core\Database\Driver\mysql;
 
-use Drupal\Core\Database\Database;
 use Drupal\KernelTests\Core\Database\DatabaseTestBase;
 
 /**

Removed unused use on commit.

  • alexpott committed 0dd7576 on 8.8.x
    Issue #3059332 follow-up by mondrake, Mile23, Lendude: Mark kernel tests...
alexpott’s picture

Created #3065101: Remove MySqlDriverTestBase - another reason why doing that is correct is that test requirements shouldn't dictate class structure so much - it means that if we work out or need to do the same test for another DB driver - say MariaDB if we split them or can extend this coverage to SQLite or PostgreSQL then we don't need to change everything.

Status: Fixed » Closed (fixed)

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

Gábor Hojtsy’s picture

As this would change behaviour of extending contrib tests, adding release notes tag.

@catch wrote above:

I think this is fine to commit to 8.8.x - it might result in some contrib tests failing but in a way that's easy to fix.

Gábor Hojtsy’s picture

Issue tags: +8.8.0 release notes