Problem/Motivation

Child of #3123120: Properly deprecate AssertLegacyTrait::pass to keep scope limited.
AssertLegacyTrait::pass is meant for removal in Drupal 10.

Proposed resolution

In this issue, remove its usage from base test classes.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mondrake created an issue. See original summary.

mondrake’s picture

Issue summary: View changes
mondrake’s picture

mondrake’s picture

Many of the calls to ::pass are actually are there just to log a test entry for display in Simpletest, which is no-op in PHPUnit though. Here converting to ::verbose, that is to be deprecated itself, but potentially replaceable in #2795567: Use Symfony's VarDumper for easier test debugging with dump(), so I thought not to just drop them, but kick them forward.

mondrake’s picture

Status: Active » Needs review
FileSize
19.91 KB

Status: Needs review » Needs work

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

mondrake’s picture

Status: Needs work » Needs review
longwave’s picture

Not really convinced any of these verbose() are actually useful, I think we should just consider removing them now. The exception-related refactoring looks fine.

mondrake’s picture

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

Let's do #8 then. Good task for novice contributors.

prabha1997’s picture

Assigned: Unassigned » prabha1997
prabha1997’s picture

Assigned: prabha1997 » Unassigned
Status: Needs work » Needs review
FileSize
2.64 KB

Kindly review new patch

Status: Needs review » Needs work

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

Lal_’s picture

Status: Needs work » Needs review
FileSize
3.78 KB
1.5 KB
mondrake’s picture

Issue tags: -Novice
FileSize
18.13 KB

Doing #8, starting from #5.

EDIT - xpost with #13

mondrake’s picture

FileSize
14.22 KB

Forgot an interdiff.

mondrake’s picture

longwave’s picture

Status: Needs review » Reviewed & tested by the community

I don't think any of these pass() calls add any value now we are in PHPUnit - they might have been useful in the Simpletest UI but that is long gone.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/tests/Drupal/KernelTests/Core/Cache/GenericCacheBackendUnitTestBase.php
    @@ -214,13 +214,8 @@ public function testSetGet() {
    -    try {
    -      $backend->set('assertion_test', 'value', Cache::PERMANENT, ['node' => [3, 5, 7]]);
    -      $this->fail('::set() was called with invalid cache tags, runtime assertion did not fail.');
    -    }
    -    catch (\AssertionError $e) {
    -      $this->pass('::set() was called with invalid cache tags, runtime assertion failed.');
    -    }
    +    $this->expectException(\AssertionError::class);
    +    $backend->set('assertion_test', 'value', Cache::PERMANENT, ['node' => [3, 5, 7]]);
    
    @@ -411,18 +406,13 @@ public function testSetMultiple() {
    -    try {
    -      $items = [
    -        'exception_test_1' => ['data' => 1, 'tags' => []],
    -        'exception_test_2' => ['data' => 2, 'tags' => ['valid']],
    -        'exception_test_3' => ['data' => 3, 'tags' => ['node' => [3, 5, 7]]],
    -      ];
    -      $backend->setMultiple($items);
    -      $this->fail('::setMultiple() was called with invalid cache tags, runtime assertion did not fail.');
    -    }
    -    catch (\AssertionError $e) {
    -      $this->pass('::setMultiple() was called with invalid cache tags, runtime assertion failed.');
    -    }
    +    $items = [
    +      'exception_test_1' => ['data' => 1, 'tags' => []],
    +      'exception_test_2' => ['data' => 2, 'tags' => ['valid']],
    +      'exception_test_3' => ['data' => 3, 'tags' => ['node' => [3, 5, 7]]],
    +    ];
    +    $this->expectException(\AssertionError::class);
    +    $backend->setMultiple($items);
    

    These changes will reduce test coverage. \Drupal\KernelTests\Core\Cache\ApcuBackendTest::testSetGet for example will end now once the exception is thrown in the call to parent::testSetGet();. We can't use expectException() in a base class like this - we need to catch the exception as before.

  2. +++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
    @@ -745,12 +745,7 @@ protected function installEntitySchema($entity_type_id) {
    +      $this->assertTrue($all_tables_exist, sprintf('Installed entity type tables for the %s entity type: %s', $entity_type_id, '{' . implode('}, {', $tables) . '}'));
    

    This can be removed. It's pointless. $all_tables_exist = FALSE; will never be reached because once a phpunit test fails it ends.

Lal_’s picture

Status: Needs work » Needs review
FileSize
16.48 KB
2.75 KB
mondrake’s picture

Status: Needs review » Needs work

Thanks @Lal_

+++ a/core/tests/Drupal/KernelTests/Core/Cache/GenericCacheBackendUnitTestBase.php
@@ -214,8 +214,13 @@
+    catch (\AssertionError $e) {
+      $this->pass('::set() was called with invalid cache tags, runtime assertion failed.');
+    }

@@ -406,13 +411,18 @@
+    catch (\AssertionError $e) {
+      $this->pass('::setMultiple() was called with invalid cache tags, runtime assertion failed.');
+    }

still we need to remove the calls to pass, which is the purpose of this issue. How about catching a \Throwable, and within the catch block, assert the exception is an instance of \AssertionError?

mondrake’s picture

Title: Replace usage of AssertLegacyTrait::pass from base test classes » Remove usage of AssertLegacyTrait::pass from base test classes
kishor_kolekar’s picture

Status: Needs work » Needs review
FileSize
19.05 KB
2.4 KB

Status: Needs review » Needs work

The last submitted patch, 22: 3123253-22.patch, failed testing. View results

mondrake’s picture

mondrake’s picture

Status: Needs work » Needs review
jungle’s picture

Status: Needs review » Needs work

File core/tests/Drupal/KernelTests/KernelTestBase.php, line 9, Unused use statement

mondrake’s picture

Status: Needs work » Needs review
FileSize
596 bytes
18.52 KB

Thanks @jungle

longwave’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/tests/Drupal/FunctionalTests/Installer/ConfigAfterInstallerTestBase.php
@@ -32,13 +32,7 @@ protected function assertInstalledConfig(array $skipped_config) {
+      $this->assertConfigDiff($result, $config_name, $skipped_config);

I checked what this method does and it doesn't actually perform assertions, rather it just throws an exception on failure. This means we have to use the $this->addToAssertionCount(1); workaround at the top of the method as there are now no assertions here. Worth opening a new issue to make it more PHPUnit-y?

Otherwise this all looks good and concerns from #18 have been addressed.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Oh hey, I remember being flummoxed back in the day by all the noise that comment tests spat out.

I looked carefully over the removals here to make sure the information from the old pass() calls was available somewhere else (e.g., in inline comments). Just a couple points to fix:

  1. +++ b/core/tests/Drupal/KernelTests/Core/Cache/GenericCacheBackendUnitTestBase.php
    @@ -216,10 +216,10 @@ public function testSetGet() {
    +      // Do nothing, continue testing in extending classes.
    
    @@ -418,10 +418,10 @@ public function testSetMultiple() {
    +      // Do nothing, continue testing in extending classes.
    
    +++ b/core/tests/Drupal/KernelTests/Core/Config/Storage/ConfigStorageTestBase.php
    @@ -138,8 +138,7 @@ public function testInvalidStorage() {
    +      // An exception occurred as expected, just continue.
    

    Very-nit: Comma splice. (The comma should be replaced by a semicolon in each case.)

  2. +++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
    @@ -735,21 +734,8 @@ protected function installEntitySchema($entity_type_id) {
    +        $this->assertTrue($db_schema->tableExists($table), "Failed entity type table installation for the $entity_type_id entity type: $table");
    

    The assertion message looks wrong here. If the assertion passes, the installation of the table was successful, right? Then The message should be "Successfully installed entity type table for..."

Thanks!

xjm’s picture

Title: Remove usage of AssertLegacyTrait::pass from base test classes » Remove usage of AssertLegacyTrait::pass() from base test classes
Issue tags: +Needs followup

For #28.

mondrake’s picture

Assigned: Unassigned » mondrake

On this.

mondrake’s picture

Assigned: mondrake » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs followup
FileSize
18.55 KB
2.55 KB

Thanks.

#28: filed #3133137: Convert AssertConfigTrait::assertConfigDiff() into a real assertion.

#29:

1. Done.
2. There is #3131946: [META] [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages where the format of to-be remaining custom $messages is being discussed and hopefully normed. Here, adjusting the message to the latest suggestion in that thread, {context} should {verb} {subject}.

longwave’s picture

Status: Needs review » Reviewed & tested by the community

All concerns addressed, back to RTBC.

  • xjm committed 7b1f3e1 on 9.1.x
    Issue #3123253 by mondrake, Lal_, prabha1997, longwave, xjm, alexpott,...

  • xjm committed 72976b6 on 9.0.x
    Issue #3123253 by mondrake, Lal_, prabha1997, longwave, xjm, alexpott,...

  • xjm committed f721377 on 8.9.x
    Issue #3123253 by mondrake, Lal_, prabha1997, longwave, xjm, alexpott,...

  • xjm committed 5e36ee8 on 8.8.x
    Issue #3123253 by mondrake, Lal_, prabha1997, longwave, xjm, alexpott,...
xjm’s picture

Version: 9.1.x-dev » 8.8.x-dev
Status: Reviewed & tested by the community » Fixed

Committed to all four branches. Thanks!

Status: Fixed » Closed (fixed)

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

jungle’s picture

Issue tags: +Deprecated assertions