[meta] Replace assertions involving calls to isset() with more appropriate assertions

Nice one. Imho

A) need to make sure this is for arrays only (isset on scalar will not apply here)
B) besides checking the existence ofthe array key, we need to ensure also that the key value is not null, since it could be. Maybe we can just assertNotNull instead, if the array key is missing we will get an exception anyway

Just a wee start. This makes the changes to Unit tests. Used this to find the files to edit, egrep -r "this->assert.+isset" core | grep Unit | awk -F: '{print $1}' | sort -u.

Quick "Reroll-n-Run" patch attached (hopefully) fixing Test Failures and addressing #6

reviewed patch - Looks good for me.

no, in last patch missing return

 -    return $this->assertArrayHasKey($id, $list);
+    $this->assertArrayNotHasKey($id, $list);

@AndrewsizZ All assert functions return void. A return isn't needed IMHO, if it fails it will throw an Exception. So I removed it on purpose.

Another installment, still much work to do. I was working off the patch in #4 so the interdiff is against that.

For now I have restored the assertions in ConnectionUnitTest.php

@Junge, than you man
Do we have list of issues where we still need work?

I want to fix the tests before going further. So, fixing typos and stray characters.

Another installments and this is enough for today on this.

Per #2.B,

+++ b/core/modules/aggregator/tests/src/Functional/AggregatorRenderingTest.php
@@ -119,7 +119,7 @@ public function testFeedPage() {
-    $this->assertTrue(isset($links[0]), new FormattableMarkup('Link to href %href found.', ['%href' => $href]));
+    $this->assertArrayHasKey(0, $links);

in case you have

  $links = [
    0 => NULL,
  ];

will result in isset($links[0]) return FALSE. So assertTrue will fail. But, $links DOES have a 0 key, so assertArrayHasKey(0, $links) will pass, which is wrong vs the original intention.

I think in this case $this->assertNotNull($links[0]) would fit better here, because if the array has not the key 0 we will have an exception, if the key is there but associated to NULL, we would fail as well.

Wow, much work done there @quietone!

I've (tried to) fix(ed) 4 out of the 5 fails and the codesniffer issue.

The remaining failure is addressed by @mondrake in #19. I happily leave that to people with Bigger Brains than mine :)

Here's a patch based on #18

@mondrake, thanks for getting me to think more about this. I don't like the idea of getting an exception in a test, I'd rather have assertions catch the errors.

But the point is that replacing assertions on isset() with just arrayHasKey/arrayNotHasKey won't work. isset does two checks, that the variable exists and that it is not NULL whereas arrayHasKey/arrayNotHasKey just checks that they key exists, using array_key_exists. To remove the isset() there needs to be two assertions, arrayHasKey/arrayNotHasKey followed by an assertNull/assertNotNull. However, that too is problematic. One needs to know if the test always fails on the existence or absence of the key or the value of the variable.

Just to see I changed two tests in ConnectionUnitTest.php:

    */
   protected function assertConnection($id) {
     $list = $this->monitor->query('SHOW PROCESSLIST')->fetchAllKeyed(0, 0);
-    return $this->assertTrue(isset($list[$id]), new FormattableMarkup('Connection ID @id found.', ['@id' => $id]));
+    $this->assertArrayHasKey($id, $list);
+    $this->assertNotNull($list[$id]);
   }
 
   /**
@@ -89,7 +89,7 @@ protected function assertConnection($id) {
    */
   protected function assertNoConnection($id) {
     $list = $this->monitor->query('SHOW PROCESSLIST')->fetchAllKeyed(0, 0);
-    return $this->assertFalse(isset($list[$id]), new FormattableMarkup('Connection ID @id not found.', ['@id' => $id]));
+    $this->assertArrayNotHasKey($id, $list);
   }

But that works for this file. There could be other tests, in a loop, where the isset() fails in one case on the existence of the key and in other where it fails because the fail it NULL.

Patch looks good. I have 2 remarks:

1. +++ b/core/modules/content_moderation/tests/src/Functional/ViewsModerationStateFilterTest.php
   @@ -347,7 +347,7 @@ protected function assertWorkflowDependencies(array $workflow_ids, ViewEntityInt
   -      $this->assertTrue(!isset($dependencies['config']));
   +      $this->assertArrayHasKey('config', $dependencies);
   

   Change is wrong. We should use assertArrayNotHasKey().

2. In most code changes the assert message text has been removed, but not in all of them. Why?

The removal of the return value for the methods Drupal\KernelTests\Core\Database\ConnectionUnitTest::assertConnection(), Drupal\KernelTests\Core\Database\ConnectionUnitTest::assertNoConnection() and
Drupal\Tests\system\Functional\Form\ConfirmFormTest::assertCancelLinkUrl() is not a problem, because those returns values are never used.

@jungle: Thank you for the answer.

I have looked at the issue and I also looked again at the patch from comment #21. The changes for the assert message text are for me correct.
Still open are comments #22 and #23.1.

Reroll of patch in #21
Also addresses #23.1
Still needs addressing #22

Reroll of patch in #26
Still needs addressing #22

Addressed #22.

Also includes change recommended by @xjm, https://www.drupal.org/project/drupal/issues/3128815#comment-13593362 [#25.1],

Here we're losing information provided by the assertion messages. Can we move them to inline comments above the respective assertions?

Updated the patch to fix a nitpick. Updated inline comments, included // Verify that... (etc.).

@jungle addressed your review points.

Thanks!

@jungle,

Thanks for the review and suggestion.

I have addressed

Lines containing comments (including docblocks) must wrap as close to 80 characters as possible without going over, with a few exceptions (noted in the Tag Reference below).

#33 and updated the patch accordingly.

Please review.

@jungle,

I have gone through all the added strings.

I have fixed the #35.1, #35.3, and other related strings.

Haven't fixed #35.2 as the change was leading to 81 characters.

Hopefully, this change fixes all the issues.

Thanks!

Rerolled patch #39, please review.

Patch was rerolled.
I have only 1 remark:

+++ b/core/modules/block_content/tests/src/Functional/BlockContentTypeTest.php
@@ -82,11 +85,15 @@ public function testBlockContentTypeCreation() {
-    $this->assertTrue(isset($field_definitions['body']), "Body field for 'basic' block type created when using the testing API to create block content types.");
+    // Verify that body field for 'basic' block type created when using the
+    // testing API to create block content types.
+    $this->assertArrayHasKey('body', $field_definitions);

Missing the "assertNotNull()" for this change.

Some more remarks:

1. When I do a code base search for "$this->assert(", I still find testing for array elements.
2. The same for when I search for "$this->assertTrue(isset(".
3. And for "$this->assertTrue(!isset(".
4. Also for "$this->assertFalse(isset("

Adding assertNotNull()

+++ b/core/modules/block_content/tests/src/Functional/BlockContentTypeTest.php
@@ -82,11 +85,15 @@ public function testBlockContentTypeCreation() {
-    $this->assertTrue(isset($field_definitions['body']), "Body field for 'basic' block type created when using the testing API to create block content types.");
+    // Verify that body field for 'basic' block type created when using the
+    // testing API to create block content types.
+    $this->assertArrayHasKey('body', $field_definitions);

as suggested at #43.

Addressed many of
assertTrue(isset*, assert(isset*
as per #44 comment still there are occurrences which needs to be fixed

Please ignore #47 patch....it messed existing changes of #45...
Kindly consider #45 whoever picking and going ahead.

Working on remaining replacements

I'm working on it.

Tagging for DIA contribution sprint.

Apologies. Couldnt complete it earlier.
I did few more replacements and fixed failing tests.
@msuthars, Please review if possible.

Fixed lint errors.

Fixed failing tests and CS errors.

I understand that in this patch assertTrue(isset()) is generally converted to two separate assertions - a assertArrayHasKey and a assertNotNull. Although a bit redundant (assertNotNull would fail if the array has not the key), fine with me. That needs to be done conistsently though.

1. +++ b/core/modules/block/tests/src/Kernel/BlockViewBuilderTest.php
   @@ -205,9 +205,13 @@ public function testBlockViewBuilderViewAlter() {
   +    $this->assertTrue($build['#prefix'] === 'Hiya!<br>', 'A cached block without content is altered.');
   

   assertSame (invert the order of arguments)

2. +++ b/core/modules/block_content/tests/src/Functional/BlockContentTypeTest.php
   @@ -73,7 +73,10 @@ public function testBlockContentTypeCreation() {
   +    // Verify that body field created when using the UI to create block content
   +    // types.
   

   'was' or 'is' created

3. +++ b/core/modules/config/tests/src/Functional/ConfigImportUITest.php
   @@ -220,7 +221,8 @@ public function testImport() {
   +    // Verify that bartik theme uninstalled during import.
   

   same

4. +++ b/core/modules/config/tests/src/Functional/ConfigImportUITest.php
   @@ -508,11 +510,11 @@ public function testExtensionValidation() {
   +    $this->assertNotNull($module_data['node']->requires['text'], 'The Node module depends on the Text module.');
   ...
   +    $this->assertNotNull($theme_data['test_subtheme']->requires['test_basetheme'], 'The Test Subtheme theme depends on the Test Basetheme theme.');
   

   needs a assertArrayHasKey also, for consistency

5. +++ b/core/modules/dblog/tests/src/Kernel/Views/ViewsIntegrationTest.php
   @@ -97,8 +97,10 @@ public function testRelationship() {
   +    $this->assertNotNull($base_tables['users_field_data']);
   ...
   +    $this->assertNotNull($base_tables['watchdog']);
   

   out of scope here

6. +++ b/core/modules/field/tests/src/Functional/Views/FieldUITest.php
   @@ -98,7 +98,7 @@ public function testHandlerUI() {
   -    $this->assertTrue(isset($dependencies['views.view.test_view_fieldapi']), 'The view is dependent on the field storage.');
   +    $this->assertArrayHasKey('views.view.test_view_fieldapi', $dependencies, 'The view is dependent on the field storage.');
   

   needs a assertNotNull also, for consistency

7. +++ b/core/modules/field/tests/src/Kernel/BulkDeleteTest.php
   @@ -364,7 +364,7 @@ public function testPurgeField() {
   -    $this->assertTrue(isset($storages[$field_storage->uuid()]), 'The field storage exists and is not deleted');
   +    $this->assertArrayHasKey($field_storage->uuid(), $storages, 'The field storage exists and is not deleted');
   

   same

8. +++ b/core/modules/field/tests/src/Kernel/FieldImportDeleteUninstallTest.php
   @@ -102,8 +102,8 @@ public function testImportDeleteUninstall() {
   +    $this->assertArrayHasKey($unrelated_field_storage->uuid(), $deleted_storages);
   

   same

9. +++ b/core/modules/file/tests/src/Functional/FileFieldWidgetTest.php
   @@ -107,7 +107,7 @@ public function testSingleValuedWidget() {
   +    $this->assertArrayHasKey(0, $label, 'Label for upload found.');
   

   same

10. +++ b/core/modules/filter/tests/src/Functional/FilterFormatAccessTest.php
    @@ -157,9 +157,9 @@ public function testFormatPermissions() {
    +    $this->assertArrayHasKey($this->allowedFormat->id(), $options);
    

    same

11. +++ b/core/modules/forum/tests/src/Functional/ForumTest.php
    @@ -375,7 +375,7 @@ private function doAdminTests($user) {
    +    $this->assertArrayHasKey(0, $relations_widget, 'Relations widget element found.');
    

    same

12. +++ b/core/modules/locale/tests/src/Functional/LocaleJavascriptTranslationTest.php
    @@ -100,7 +100,7 @@ public function testFileParsing() {
    +        $this->assertArrayHasKey($str, $source_strings, new FormattableMarkup('Found source string: %source', $args));
    

    same

13. +++ b/core/modules/menu_link_content/tests/src/Kernel/PathAliasMenuLinkContentTest.php
    @@ -84,7 +84,7 @@ public function testPathAliasChange() {
    +    $this->assertArrayHasKey($menu_link_content->getPluginId(), $tree);
    

    same

14. +++ b/core/modules/node/tests/src/Functional/NodeLoadMultipleTest.php
    @@ -57,9 +57,11 @@ public function testNodeMultipleLoad() {
    +    $this->arrayHasKey($node1->id(), $nodes);
    

    assertArrayHasKey - wrong method name, and misses assertNull

15. +++ b/core/modules/node/tests/src/Functional/NodeTranslationUITest.php
    @@ -421,7 +421,7 @@ protected function doTestAlternateHreflangLinks(Node $node) {
    +            $this->assertNotNull($links[0], new FormattableMarkup('The %langcode node translation has the correct alternate hreflang link for %alternate_langcode: %link.', ['%langcode' => $langcode, '%alternate_langcode' => $alternate_langcode, '%link' => $url->toString()]));
    

    needs a assertArrayHasKey also, for consistency

16. +++ b/core/modules/node/tests/src/Kernel/NodeAccessLanguageTest.php
    @@ -219,7 +219,9 @@ public function testNodeAccessQueryTag() {
    +    $this->assertNotNull($nids[$node_public->id()]);
    ...
    +    $this->assertNotNull($nids[$node_no_language->id()]);
     
    

    out of scope

17. +++ b/core/modules/search/tests/src/Functional/SearchAdvancedSearchFormTest.php
    @@ -102,11 +102,17 @@ public function testFormRefill() {
    +        $this->assertTrue($elements[0]->getValue() == $value, "Field $key is set to $value");
    

    assertEquals

18. +++ b/core/modules/search/tests/src/Functional/SearchAdvancedSearchFormTest.php
    @@ -102,11 +102,17 @@ public function testFormRefill() {
    +        $this->assertTrue(!empty($elements[0]->getAttribute('checked')), "Field $key is checked");
    

    assertNotEmpty

19. +++ b/core/modules/system/tests/src/Functional/Form/ConfirmFormTest.php
    @@ -78,14 +78,12 @@ public function testConfirmFormWithExternalDestination() {
    -   *
    -   * @return bool
    -   *   Result of the assertion.
        */
       public function assertCancelLinkUrl(Url $url, $message = '', $group = 'Other') {
         $links = $this->xpath('//a[@href=:url]', [':url' => $url->toString()]);
         $message = ($message ? $message : new FormattableMarkup('Cancel link with URL %url found.', ['%url' => $url->toString()]));
    -    return $this->assertTrue(isset($links[0]), $message, $group);
    +    $this->assertArrayHasKey(0, $links, $message);
    +    $this->assertNotNull($links[0]);
       }
    

    add a return TRUE; at the end, keep the return @bool in the doc. Removing the return is a good idea, but there's another issue for that.

20. +++ b/core/modules/update/tests/src/Kernel/UpdateReportTest.php
    @@ -32,6 +32,7 @@ public function testTemplatePreprocessUpdateReport($variables) {
    +    $this->assertNotNull($variables['no_updates_message']);
    

    out of scope

Working on this.

Here I have tried to made changes as suggested in comment #59, please review.

1. +++ b/core/modules/block/tests/src/Kernel/BlockViewBuilderTest.php
   @@ -211,7 +211,7 @@ public function testBlockViewBuilderViewAlter() {
   -    $this->assertTrue($build['#prefix'] === 'Hiya!<br>', 'A cached block without content is altered.');
   +    $this->assertSame('A cached block without content is altered.', $build['#prefix'] === 'Hiya!<br>');
   

   should be $this->assertSame('Hiya!<br>', $build['#prefix'], 'A cached block without content is altered.');

2. +++ b/core/modules/block_content/tests/src/Functional/BlockContentTypeTest.php
   @@ -73,7 +73,7 @@ public function testBlockContentTypeCreation() {
   +    // Verify that body field is created when using the UI to create block content
   

   exceeds the 80 chars per row comment limit

3. +++ b/core/modules/search/tests/src/Functional/SearchAdvancedSearchFormTest.php
   @@ -105,14 +105,14 @@ public function testFormRefill() {
   -        $this->assertTrue($elements[0]->getValue() == $value, "Field $key is set to $value");
   +        $this->assertEquals($elements[0]->getValue() == $value, "Field $key is set to $value");
   

   $this->assertEquals($value, $elements[0]->getValue(), "Field $key is set to $value");

4. +++ b/core/modules/search/tests/src/Functional/SearchAdvancedSearchFormTest.php
   @@ -105,14 +105,14 @@ public function testFormRefill() {
   -        $this->assertTrue(!empty($elements[0]->getAttribute('checked')), "Field $key is checked");
   +        $this->assertNotEmpty(!empty($elements[0]->getAttribute('checked')), "Field $key is checked");
   

   $this->assertNotEmpty($elements[0]->getAttribute('checked'), "Field $key is checked");

I'm working on the suggested changes and failed test cases.

Updated the patch with suggested changes. Please review.

I tried grepping the code base with the following regexp, >assert.*\(.*isset\(, and found 232 more results in 87 files. Maybe some are false positives, but at a first glance there are many that still can be converted.

Nit:

+++ b/core/modules/search/tests/src/Functional/SearchAdvancedSearchFormTest.php
@@ -105,14 +105,14 @@
+        $this->assertNotEmpty($elements[0]->getAttribute('checked'), "Field $key is checked"); ¶

This adds a whitespace at the end of the line.

Covers almost isset(), but there some use case in which we can't replace them.

Revert some isset() changes. Fixed test cases issue.

@msuthars I started reviewing #71 and there were too many unexpected changes. Here restarting from #65 that was almost ready IMHO. 137 more instances to check based on #67.

Thanks. Approx ~90 more cases to check.

Alternative fixes for #78

This should be it, the remaining ones IMHO should stay as is.

Some fixes.

When $this->xpath is used, it's enough to check if an element with the specific index exists in the array - no need to also check if it's not null.

on this

./core/tests/Drupal/KernelTests/Core/Database/SelectComplexTest.php: $this->assertFalse(isset($tables['test']['all_fields']), 'Count query correctly unsets \'all_fields\' statement.');

./core/modules/views/tests/src/Kernel/EventSubscriber/ViewsEntitySchemaSubscriberIntegrationTest.php: $this->assertFalse(isset($display['display_options']['fields']['id_broken']));

./core/modules/user/tests/src/Kernel/UserAccountFormFieldsTest.php:      $this->assertFalse(isset($form['account'][$key]['#attributes']['autocomplete']), "'$key' field: 'autocomplete' attribute not found."); 
./core/modules/user/tests/src/Kernel/UserAccountFormFieldsTest.php:      $this->assertFalse(isset($form['account'][$key]['#attributes']['autocomplete']), "'$key' field: 'autocomplete' attribute not found."); 

./core/modules/system/tests/src/Functional/System/TokenScanTest.php:    $this->assertFalse(isset($token_wannabes['']['empty token type']), 'An empty token type has not been matched.'); 
./core/modules/system/tests/src/Functional/System/TokenScanTest.php:    $this->assertFalse(isset($token_wannabes['']['']), 'An empty token and type has not been matched.'); 

./core/modules/views/tests/src/Unit/EntityViewsDataTest.php: $this->assertFalse(isset($data['entity_test_mul']['type']['relationship']));

./core/modules/views/tests/src/Kernel/EventSubscriber/ViewsEntitySchemaSubscriberIntegrationTest.php: $this->assertFalse(isset($display['display_options']['fields']['id_broken']));

in these cases, the array would miss even keys earlier than the last sub-key, the test would fail on missing index, so I preferred to keep as is for readibility

./core/modules/comment/tests/src/Functional/CommentEntityTest.php: $this->assertFalse(isset($settings['history']['lastReadTimestamps']) && in_array($term->id(), array_keys($settings['history']['lastReadTimestamps'])), 'history.lastReadTimestamps is present.');

this and the other ones that have an ANDed assertion I'd rather leave to a follow-up

Reviewed about half this before I ran out of steam. Detailed comments are below.

I have a feeling that this is getting too big and complicated to try and solve in one issue as there is a lot of subtle changes here and reviewing it is already hard. I suggest that we can break some of this out into separate issues:

1. Changes to helper methods only
2. Calls to assertTrue(... && ...) which can be broken into multiple assertions
3. Functional test patterns similar to
   

   $something = $this->xpath(...);
   $this->assertArrayHasKey(0, $something);
   

   which would be much better off as $this->assertSession()->elementExists() or similar

The latter case I feel most strongly about, as all we are doing in this issue is rearranging assertTrue to assertArrayHasKey when in fact the assertion really feels like it should be rewritten entirely - elementExists is far more readable.

If we don't do any of those, or if it is still too big after that, how about breaking it down by test type, e.g. splitting out Kernel and Functional tests?

1. +++ b/core/modules/aggregator/tests/src/Functional/AggregatorRenderingTest.php
   @@ -62,12 +62,12 @@ public function testBlockLinks() {
        $links = $this->xpath('//a[@href = :href]', [':href' => reset($items)->getLink()]);
   -    $this->assert(isset($links[0]), 'Item link found.');
   +    $this->assertArrayHasKey(0, $links, 'Item link found.');
   

   Do we need a followup to change this sort of thing to $this->assertSession()->linkByHrefExists()?

2. +++ b/core/modules/block/tests/src/Kernel/BlockViewBuilderTest.php
   @@ -205,9 +205,13 @@ public function testBlockViewBuilderViewAlter() {
   +    $this->assertArrayHasKey('#prefix', $build);
   +    $this->assertNotNull($build['#prefix']);
   +    $this->assertSame('Hiya!<br>', $build['#prefix'], 'A cached block without content is altered.');
   

   assertNotNull seems redundant here

3. +++ b/core/modules/block/tests/src/Kernel/BlockViewBuilderTest.php
   @@ -266,7 +270,8 @@ public function testBlockViewBuilderBuildAlter() {
   +      $this->assertArrayHasKey('#create_placeholder', $build);
   +      $this->assertNotNull($build['#create_placeholder']);
          $this->assertIdentical($value, $build['#create_placeholder']);
   

   assertNotNull seems redundant here

4. +++ b/core/modules/block_content/tests/src/Functional/BlockContentTypeTest.php
   @@ -73,7 +73,10 @@ public function testBlockContentTypeCreation() {
   -    $this->assertTrue(isset($field_definitions['body']), 'Body field created when using the UI to create block content types.');
   +    // Verify that body field is created when using the UI to create block
   +    // content types.
   +    $this->assertArrayHasKey('body', $field_definitions);
   +    $this->assertNotNull($field_definitions['body']);
   

   Is the not null check strictly necessary here? Feels better to either just assertArrayHasKey or also assert a specific type rather than just "not null".

5. +++ b/core/modules/ckeditor/tests/src/Functional/CKEditorLoadingTest.php
   @@ -207,10 +207,14 @@ public function testLoadingWithoutInternalButtons() {
   +    $this->assertArrayHasKey('customConfig', $editor_settings);
   +    $this->assertNotNull($editor_settings['customConfig']);
   +    $this->assertArrayHasKey('entities', $editor_settings);
   +    $this->assertNotNull($editor_settings['entities']);
   +    $this->assertArrayHasKey('allowedContent', $editor_settings);
   +    $this->assertNotNull($editor_settings['allowedContent']);
   +    $this->assertArrayHasKey('disallowedContent', $editor_settings);
   +    $this->assertNotNull($editor_settings['disallowedContent']);
   

   Same

6. +++ b/core/modules/config/tests/src/Functional/ConfigImportAllTest.php
   @@ -103,9 +103,15 @@ public function testInstallUninstall() {
   +    // Verify that the comment module will be disabled.
   +    $this->assertArrayHasKey('comment', $modules_to_uninstall);
   +    $this->assertNotNull($modules_to_uninstall['comment']);
   +    // Verify that the File module will be disabled.
   +    $this->assertArrayHasKey('file', $modules_to_uninstall);
   +    $this->assertNotNull($modules_to_uninstall['file']);
   +    // Verify that editor module will be disabled.
   +    $this->assertArrayHasKey('editor', $modules_to_uninstall);
   +    $this->assertNotNull($modules_to_uninstall['editor']);
   

   Same

7. +++ b/core/modules/config/tests/src/Functional/ConfigImportUITest.php
   @@ -149,7 +149,8 @@ public function testImport() {
   -    $this->assertTrue(isset($GLOBALS['hook_cache_flush']));
   +    $this->assertArrayHasKey('hook_cache_flush', $GLOBALS);
   +    $this->assertNotNull($GLOBALS['hook_cache_flush']);
   

   Same

8. +++ b/core/modules/field/tests/src/Kernel/BulkDeleteTest.php
   @@ -266,8 +266,10 @@ public function testPurgeWithDeletedAndActiveField() {
   +    $this->assertNotNull($fields[$deleted_field_uuid]);
   

   This feels like it should be assertInstanceOf

9. +++ b/core/modules/field/tests/src/Kernel/BulkDeleteTest.php
   @@ -397,7 +399,9 @@ public function testPurgeFieldStorage() {
   +    $this->assertNotNull($fields[$field->uuid()]);
   

   Same

10. +++ b/core/modules/field/tests/src/Kernel/BulkDeleteTest.php
    @@ -407,7 +411,9 @@ public function testPurgeFieldStorage() {
         $storages = \Drupal::entityTypeManager()->getStorage('field_storage_config')->loadByProperties(['uuid' => $field_storage->uuid(), 'include_deleted' => TRUE]);
    -    $this->assertTrue(isset($storages[$field_storage->uuid()]) && !$storages[$field_storage->uuid()]->isDeleted(), 'The field storage exists and is not deleted');
    +    $this->assertArrayHasKey($field_storage->uuid(), $storages);
    +    $this->assertNotNull($storages[$field_storage->uuid()]);
    +    $this->assertFalse($storages[$field_storage->uuid()]->isDeleted(), 'The field storage exists and is not deleted');
    

    Extracting $uuid to a variable might feel cleaner

11. +++ b/core/modules/field/tests/src/Kernel/EntityReference/Views/EntityReferenceRelationshipTest.php
    @@ -359,8 +359,9 @@ public function testEntityReferenceConfigEntity() {
    +    $this->assertArrayHasKey('relationship', $views_data['entity_test__field_test_data']['field_test_data']);
    +    $this->assertNotNull($views_data['entity_test__field_test_data']['field_test_data']['relationship']);
    

    Again I don't understand the not null check here. I realise that assertTrue(isset()) will do a non NULL check and assertArrayHasKey doesn't care if the value is NULL, but while that is the semantics of the existing test, is that really what was intended?

12. +++ b/core/modules/file/tests/src/Functional/FileFieldWidgetTest.php
    @@ -107,7 +107,7 @@ public function testSingleValuedWidget() {
         $label = $this->xpath('//label[@for="' . $input[0]->getAttribute('id') . '"]');
    -    $this->assertTrue(isset($label[0]), 'Label for upload found.');
    +    $this->assertArrayHasKey(0, $label, 'Label for upload found.');
    

    More like this that I think should just be e.g. $this->assertSession()->elementExists()?

13. +++ b/core/modules/language/tests/src/Functional/LanguageNegotiationInfoTest.php
    @@ -156,7 +158,9 @@ public function testInfoAlterations() {
         // Check that unavailable language negotiation methods are not present in
         // the negotiation settings.
         $negotiation = $this->config('language.types')->get('negotiation.' . $type . '.enabled');
    -    $this->assertFalse(isset($negotiation[$test_method_id]), 'The disabled test language negotiation method is not part of the content language negotiation settings.');
    +    // Verify that the disabled test language negotiation method is not part of
    +    // the content language negotiation settings.
    +    $this->assertArrayNotHasKey($test_method_id, $negotiation);
    

    Second comment just duplicates the first IMO

14. +++ b/core/modules/locale/tests/src/Kernel/LocaleStringTest.php
    @@ -147,11 +147,17 @@ public function testStringSearchApi() {
    -    $this->assertTrue($found && isset($found->language) && isset($found->translation) && !$found->isNew(), 'Translation not found searching by source and context.');
    +    $this->assertNotNull($found);
    +    $this->assertNotNull($found->language);
    +    $this->assertNotNull($found->translation);
    +    $this->assertFalse($found->isNew(), 'Translation not found searching by source and context.');
    

    I wonder if a way of breaking this down is to do assertTrue(... && ...) in a separate issue? These are all quite similar and would simpler to get in.

15. +++ b/core/modules/node/tests/src/Functional/NodeLoadMultipleTest.php
    @@ -57,9 +57,12 @@ public function testNodeMultipleLoad() {
    +    $this->assertArrayHasKey($node1->id(), $nodes);
    +    $this->assertNotNull($nodes[$node1->id()]);
    +    $this->assertArrayHasKey($node2->id(), $nodes);
    +    $this->assertNotNull($nodes[$node2->id()]);
    +    $this->assertArrayHasKey($node4->id(), $nodes);
    +    $this->assertNotNull($nodes[$node4->id()]);
         foreach ($nodes as $node) {
           $this->assertIsObject($node);
         }
    

    I don't think we need to care about assertNotNull here if we are checking the type in the following loop.

16. +++ b/core/modules/rest/tests/src/Functional/Views/StyleSerializerTest.php
    @@ -611,7 +612,8 @@ public function testLivePreview() {
    -    $this->assertTrue(!isset($build['#markup']) && $rendered_json == $expected, 'Ensure the previewed json is escaped.');
    +    $this->assertArrayNotHasKey('#markup', $build, 'Ensure the previewed json is escaped.');
    +    $this->assertEquals($expected, $rendered_json, 'Ensure the previewed json is escaped.');
    

    Should we drop the $message arguments here?

17. +++ b/core/modules/system/tests/src/Functional/Routing/RouterTest.php
    @@ -97,17 +97,20 @@ public function testFinishResponseSubscriber() {
    +    $this->assertArrayHasKey('X-Drupal-Cache-Contexts', $headers);
    +    $this->assertNotNull($headers['X-Drupal-Cache-Contexts']);
    +    $this->assertArrayHasKey('X-Drupal-Cache-Tags', $headers);
    +    $this->assertNotNull($headers['X-Drupal-Cache-Tags']);
    +    $this->assertArrayHasKey('X-Drupal-Cache-Max-Age', $headers);
    +    $this->assertNotNull($headers['X-Drupal-Cache-Max-Age']);
    

    Can headers even be NULL? I think they are always strings?

18. +++ b/core/modules/system/tests/src/Functional/System/SiteMaintenanceTest.php
    @@ -64,7 +64,8 @@ public function testSiteMaintenance() {
         $links = $this->xpath('//script[contains(@src, :href)]', [':href' => '/core/misc/drupal.js']);
    -    $this->assertFalse(isset($links[0]), 'script /core/misc/drupal.js not in page');
    +    // Verify that the script /core/misc/drupal.js not in page.
    +    $this->assertArrayNotHasKey(0, $links);
    

    This would be so much more readable as some kind of elementExists check.

19. +++ b/core/modules/system/tests/src/Functional/System/TokenScanTest.php
    @@ -30,13 +30,20 @@ public function testTokenScan() {
         $this->assertFalse(isset($token_wannabes['']['empty token type']), 'An empty token type has not been matched.');
         $this->assertFalse(isset($token_wannabes['']['']), 'An empty token and type has not been matched.');
    -    $this->assertTrue(isset($token_wannabes['node']), 'An existing valid token has been matched.');
    +    // Verify that an existing valid token has been matched.
    +    $this->assertArrayHasKey('node', $token_wannabes);
    +    $this->assertNotNull($token_wannabes['node']);
    

    Did we miss the assertFalse() here?

Thanks a lot @longwave.

I agree, this has become too large for a single issue.

I’ll think a way to spin off some separate issues, the one for xpath to start with.

AFK at the moment, if anyone feels like taking that feel free.

Unpostponed #3129002: Replace usages of deprecated AssertLegacyTrait::assert() since I think that's the better currently open issue where to start tackling

Functional test patterns similar to

$something = $this->xpath(...);
$this->assertArrayHasKey(0, $something);

which would be much better off as $this->assertSession()->elementExists() or similar

when that's agreed a lot of similar instance could be addressed similarly.

Just a reroll.

#3166450: Split assertTrue using && into multiple assertions is in, maybe this can be worked on again

Actually no, this one is still too much overlapping with #3167880: [meta] Convert assertions involving use of xpath to WebAssert, where possible.

A reroll anyway.

Rerolled and chenged to MR workflow

Rerolled

Fixed all cases that I can find by looking for >assert.*isset, except EntityFieldTest which I think makes more sense left alone and a few with complex boolean assertions.

CommentEntityTest has been simplified to such an extent I'm not sure it was ever testing anything useful.

I started reviewing the MR, then I realized we probably better discuss here first:

IMHO we should:
a) avoid using assertArrayHasKey() with an immediately following assertNotNull(). The latter also includes check for the former.
b) revert the removals of the $message argument where it is supported by the method - we do not have any consensus to remove them massively and when we could get there we could have a separate issue.

Agree with both points. There is no need for two assertions; assertNotNull will also fail if the key doesn't exist. And yes, let's deal with the messages elsewhere. I think it's fair to replace t() or FormattableMarkup with plain strings if you find it here though.

I think all issues are addressed.

Pleased that we also found a bug in UserAccountFormFieldsTest here where we were testing a nonexistent form element all along.

The testbot is failing.

Fixed test failures, I hope.

Just a few more checks please, then this is pretty much ready.

Found a few more instances, in

core/tests/Drupal/KernelTests/Core/Entity/EntityFieldTest.php and
core/modules/views/tests/src/Unit/EntityViewsDataTest.php

Thanks for reviewing. I think I've fixed up everything except EntityFieldTest. I noted this in #105; to me this makes sense to leave as it is, as the comment above the section in question states "Test using isset(), empty() and unset()" and this is explicitly checking the magic methods __isset() and friends on the entity field objects. Apart from the use of FormattableMarkup the test reads well to me and I think would lose readability by changing it to use other assertions.

EntityViewsDataTest was a strange one, I have just deleted this assertion. It wasn't testing anything and should have been removed in https://git.drupalcode.org/project/drupal/-/commit/376fb615d7f862aade6e7... as far as I can see - the type property was removed from entity_test_mul here as were two other assertions checking this, but the negative assertion was left in place.

The errors we have found while fixing this makes me think a good use of static analysis would be to detect asserting for non-existence in arrays when we don't assert for existence of the parent item first. This would have caught at least two bugs that were discovered here.

Thanks for the explanations @longwave. This looks great to me now. I'd be +1 on RTBC but I do not think I can set it myself.

I went through the whole patch as of #104 and fixed or upgraded parts that I thought needed to be changed, so to me that counts as a review of an earlier version. I think if you have reviewed the whole patch again since I made the recent changes, it's OK for you to mark RTBC - as it's quite a large set of changes I'm not sure if we will recruit anyone else to look at it before it ends up with merge conflicts.

Agreed. Let’s see if core committers do as well.

Needs a reroll.

Rerolled, waiting for test run as recent runs were red

on this

Fixed the test failure.

Changes by @mondrake look good to me. We only care about the header keys and not their values in the debug_cacheability_headers test (the header values themselves are tested in a number of elsewhere) so I think this change is OK.

Rerolled

Rerolled

Rerolled

There's one fail now

Fixed, one line was changed to comment in EntityViewsDataTest before being moved from Unit to Kernel, reflected here.

There's no explict input from core committers here, but I assume given the size of this patch it has to wait for a beta disruptive patch window.

Bumping for visibility, patch still applies cleanly for me.

This one needs a re-roll after the other three disruptive issues went in.

Tried a 3-way merge but to no avail.

	both modified:   core/modules/system/tests/src/Functional/Form/ElementsTableSelectTest.php
	both modified:   core/modules/system/tests/src/Functional/Form/FormTest.php
	both modified:   core/modules/system/tests/src/Functional/System/SiteMaintenanceTest.php

Corrected IS example, thanks @berdir on Slack

Rerolled. I agree with @Berdir a lot of these assertions should be improved with more strict checks, but I am afraid it will too big a change to be done here.

My suggestion would be to wait for #3131348: Replace assertions involving calls to empty() with assertEmpty()/assertNotEmpty()/assertArrayNotHasKey(), too, then have appropriate issues to convert instances of assertNotNull, assertArrayHasKey, assertNotEmpty with more strict ones.

Rome was not built in a day. :)

Had another scan through the MR and added some comments to ones. I think I agree with @Berdir here that the assertNotNull() is not really meaningful in a lot of cases; it feels like either assertArrayHasKey() is what we really want in the simple case (especially if nearby we assertArrayNotHasKey() as well) or an actual value assertion if we want a more thorough test.

There is a lot of good cleanup here - the response header and any xpath ones for example - but I wonder if we could maybe split the scope of this and that would make it easier to get in outside of a disruptive patch window and also give us more time to think about the non-trivial cases and fix them properly?

Alright, let's make a meta of this issue then, and split issues with smaller scope.

I have split the WebAssert changes into an issue of its own, #3253715: Replace assertions involving calls to isset() on xpath results in functional tests with WebAssert calls.

A few things were fixed in spin-offs, and what's left out is probably not worth pursuing based on the comments. Wontfix then, if anyone disagrees please open a new issue with new scope. Thx

Probably outdated is a better status.

Closing as fixed and assigning credit to all contributors.