Properly deprecate AssertLegacyTrait::pass

Adding the "Deprecated assertions" tag to add this into the kanban board https://contribkanban.com/board/Deprecatedassertions

1. +++ b/core/tests/Drupal/KernelTests/Core/Database/TransactionTest.php
   @@ -545,10 +545,10 @@ public function testQueryFailureInTransaction() {
   +      $this->fail('Upset query should have failed.');
   

   Let's fix upset -> upsert while we are here

2. +++ b/core/tests/Drupal/Tests/Core/Assert/AssertLegacyTraitTest.php
   @@ -202,6 +202,14 @@ public function testAssertElementNotPresent() {
   +   * @expectedDeprecation AssertLegacyTrait::pass() is deprecated in drupal:8.0.0 and is removed from drupal:10.0.0. PHPUnit interrupts a test as soon as a test assertion fails, so there is usually no need to call this method. If a test\'s logic relies on this method, refactor the test. See https://www.drupal.org/node/3129738
   

   Does ' need escaping in the annotation?

Quite a few places where the failure message can be merged straight into the fail() method call to improve test readability, a couple of other questions.

Do you think it's worth opening a followup to refactor some of "Expected exception; just continue testing." into individual test methods that test only one thing and so can use expectException?

1. +++ b/core/modules/block_content/tests/src/Functional/BlockContentTranslationUITest.php
   @@ -120,7 +120,6 @@ protected function doTestBasicTranslation() {
        try {
          $message = 'Blocks can have translations with the same "info" value.';
          $entity->save();
   -      $this->pass($message);
        }
        catch (\Exception $e) {
          $this->fail($message);
   

   Here we can refactor away $message.

2. +++ b/core/modules/config/tests/src/Functional/SchemaConfigListenerWebTest.php
   @@ -35,7 +35,6 @@ public function testConfigSchemaChecker() {
          $this->fail($msg);
        }
        catch (SchemaIncompleteException $e) {
   -      $this->pass($msg);
   

   Here we can refactor away $msg.

3. +++ b/core/modules/config/tests/src/Functional/SchemaConfigListenerWebTest.php
   @@ -44,7 +43,6 @@ public function testConfigSchemaChecker() {
   -      $this->pass($msg);
   

   Same here

4. +++ b/core/modules/config/tests/src/Functional/SchemaConfigListenerWebTest.php
   @@ -60,7 +58,6 @@ public function testConfigSchemaChecker() {
   -      $this->pass($msg);
   

   Same here

5. +++ b/core/tests/Drupal/KernelTests/Core/Asset/LibraryDiscoveryIntegrationTest.php
   @@ -257,7 +256,7 @@ protected function assertAssetInLibrary($asset, $extension, $library_name, $sub_
   -        return $this->pass($message);
   +        return TRUE;
   

   pass() returns void but this actually fixes this method to match the docblock, so this is better!

6. +++ b/core/tests/Drupal/KernelTests/Core/Command/DbDumpTest.php
   @@ -93,8 +84,9 @@ public function register(ContainerBuilder $container) {
   -    // Determine what database backend is running, and set the skip flag.
   -    $this->skipTests = Database::getConnection()->databaseType() !== 'mysql';
   +    if (Database::getConnection()->databaseType() !== 'mysql') {
   +      $this->markTestSkipped("Skipping test since the DbDumpCommand is currently only compatible with MySql");
   +    }
   

   Much nicer :)

7. +++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigCRUDTest.php
   @@ -209,8 +209,8 @@ public function testNameValidation() {
          $this->fail($message);
        }
   -    catch (ConfigNameException $e) {
   -      $this->pass($message);
   

   $message can be refactored away here.

8. +++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigCRUDTest.php
   @@ -235,7 +235,6 @@ public function testNameValidation() {
   -      $this->pass($message);
        }
        catch (ConfigNameException $e) {
          $this->fail($message);
   

   Same here.

9. +++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigCRUDTest.php
   @@ -253,8 +252,8 @@ public function testValueValidation() {
          $this->fail($message);
        }
   -    catch (ConfigValueException $e) {
   -      $this->pass($message);
   

   Same here.

10. +++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigCRUDTest.php
    @@ -263,8 +262,8 @@ public function testValueValidation() {
           $this->fail($message);
         }
    -    catch (ConfigValueException $e) {
    -      $this->pass($message);
    

    And again.

11. +++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigSchemaTest.php
    @@ -566,7 +566,6 @@ public function testConfigSchemaInfoAlter() {
           $this->fail($message);
         }
         catch (ConfigSchemaAlterException $e) {
    -      $this->pass($message);
    

    $message can be refactored away here.

12. +++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigSchemaTest.php
    @@ -577,7 +576,6 @@ public function testConfigSchemaInfoAlter() {
           $this->fail($message);
         }
         catch (ConfigSchemaAlterException $e) {
    -      $this->pass($message);
    

    And again.

13. +++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigSchemaTest.php
    @@ -588,7 +586,6 @@ public function testConfigSchemaInfoAlter() {
           $this->fail($message);
         }
         catch (ConfigSchemaAlterException $e) {
    -      $this->pass($message);
    

    And again.

14. +++ b/core/tests/Drupal/KernelTests/Core/Database/ConnectionTest.php
    @@ -144,33 +144,17 @@ public function testMultipleStatementsForNewPhp() {
    -    catch (DatabaseExceptionWrapper $e) {
    -      $this->pass('PDO exception thrown for multiple statements.');
    -    }
    +    $this->expectException(DatabaseExceptionWrapper::class);
    +    Database::getConnection('default', 'default')->query('SELECT * FROM {test}; SELECT * FROM {test_people}', [], ['allow_delimiter_in_query' => TRUE]);
    

    Much cleaner!

15. +++ b/core/tests/Drupal/KernelTests/Core/Database/SelectTest.php
    @@ -577,12 +574,11 @@ public function testInvalidSelectCount() {
         catch (\Exception $e) {
    -      $this->pass('Exception thrown for invalid query.');
    -      return;
    +      $this->assertInstanceOf(\Exception::class, $e);
    

    When wouldn't $e be an Exception inside this catch block?

16. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityAutocompleteTest.php
    @@ -129,12 +129,7 @@ public function testSelectionSettingsHandling() {
         catch (AccessDeniedHttpException $e) {
    -      if ($e->getMessage() == 'Invalid selection settings key.') {
    -        $this->pass('Invalid selection settings key throws an exception.');
    -      }
    -      else {
    -        $this->fail('Invalid selection settings key throws an exception.');
    -      }
    +      $this->assertSame('Invalid selection settings key.', $e->getMessage());
    

    Can this use expectedException and expectedExceptionMessage?

17. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityTranslationTest.php
    @@ -68,7 +68,7 @@ protected function doTestEntityLanguageMethods($entity_type) {
           $this->fail($message);
         }
         catch (\InvalidArgumentException $e) {
    -      $this->pass($message);
    

    All the $message variables can be refactored away here too.

Thanks! The expectedException vs try/catch stuff is tricky and I wish we could just continue after an exception but I also see why we can't actually do that, so let's leave that all alone for now. Otherwise all issues look to be resolved so this is RTBC for me.

Needs another re-roll.

Rerolling for 9.1.x!

Thanks for rerolling!

Needs yet another re-roll.

On reviewing this - I agree we should open the follow-up to refactor tests that could potentially use expectException() - the current ::pass() calls inside catch are giving us the illusion of test coverage and we don't lose anything by removing them, but refactoring the tests would improve things.

core/tests/Drupal/KernelTests/Core/Database/TransactionTest.php:502:33 - Unknown word (sould)

Fixed this typo (via cspell) on commit.

It would be good to backport the test changes (but not the deprecation itself) to 9.0.x and 8.9.x

Added patch for 9.0.x and 8.9.x branch, please review.

Please ignore previous patch for 8.9.x branch. Here is the updated patch after fixing PHPLint issue in #46, please review.

Committed/pushed the respective patches to 9.0.x and 8.9.x, thanks!