Replace assertions involving calls to in_array() with assertContains()/assertNotContains()

Removing the custom assertion messages from the example.

a) If we keep them, they need to have logic inverted to describe the failure mode, not success.

b) We don't need to keep them. ;) Per @catch at #3130396-13: Replace assertions involving calls to is_file with assertFileExists()/assertFileNotExists():

fwiw I'm +1 to removing custom assertion messages from the majority of our assertions - a lot of the assertion messages were added when we originally added Simpletest to core, and have just been ported through to phpunit.

Otherwise, +1 to this. I think using the more targeted/specific assertions is more useful and maintainable than assertTrue(in_array(...)).

on this

Incomplete patch.

Patch. Deliberately leaving custom assertion messages untouched, that's for #3131946: [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages and follow-ups.

Addressing test failures.

Looks great, thanks! None of this is NW-material, but a few questions/concerns:

1. +++ b/core/modules/ckeditor/tests/src/Unit/Plugin/CKEditorPlugin/LanguageTest.php
   @@ -53,12 +53,12 @@ public function testGetConfig($language_list, $expected_number) {
        $this->assertEquals($expected_number, count($config['language_list']));
   

   How'd we miss this at the other issues? ;) Or is this going to be a patch conflict when something else lands?

2. +++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationLanguageChangeTest.php
   @@ -112,9 +112,8 @@ public function testLanguageChange() {
   -    $translation_languages = array_keys($node->getTranslationLanguages());
   -    $this->assertTrue(in_array('fr', $translation_languages));
   -    $this->assertTrue(in_array('de', $translation_languages));
   +    $this->assertArrayHasKey('fr', $node->getTranslationLanguages());
   +    $this->assertArrayHasKey('de', $node->getTranslationLanguages());
   

   Probably this is fine, but do we really want to keep calling $node->getTranslationLanguages() like this? Shouldn't we leave the $translation_languages variable as it is and use assertContains() for these (like all the other conversions in this patch)?

3. +++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationLanguageChangeTest.php
   @@ -159,9 +158,8 @@ public function testTitleDoesNotChangesOnChangingLanguageWidgetAndTriggeringAjax
   -    $translation_languages = array_keys($node->getTranslationLanguages());
   -    $this->assertTrue(in_array('fr', $translation_languages));
   -    $this->assertTrue(!in_array('de', $translation_languages));
   +    $this->assertArrayHasKey('fr', $node->getTranslationLanguages());
   +    $this->assertArrayNotHasKey('de', $node->getTranslationLanguages());
   

   And here.

4. +++ b/core/modules/dblog/tests/src/Kernel/Views/ViewsIntegrationTest.php
   @@ -93,10 +93,9 @@ public function testRelationship() {
   -    $tables = array_keys($view->getBaseTables());
   -    $this->assertTrue(in_array('users_field_data', $tables));
   -    $this->assertFalse(in_array('users', $tables));
   -    $this->assertTrue(in_array('watchdog', $tables));
   +    $this->assertArrayHasKey('users_field_data', $view->getBaseTables());
   +    $this->assertArrayNotHasKey('users', $view->getBaseTables());
   +    $this->assertArrayHasKey('watchdog', $view->getBaseTables());
   

   And here.

Finally, after the patch, I'm still getting:

egrep -r 'assert.*in_array' core | wc
      11     100    2482

Here's the full list:

core/tests/Drupal/Tests/Core/Database/SchemaIntrospectionTestTrait.php:    assert(in_array($index_type, ['index', 'unique', 'primary'], TRUE));
core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php:    assert(in_array($constraint_type, ['c', 'f', 'p', 'u', 't', 'x']));
core/modules/statistics/src/NodeStatisticsDatabaseStorage.php:    assert(in_array($order, ['totalcount', 'daycount', 'timestamp']), "Invalid order argument.");
core/modules/jsonapi/tests/src/Functional/ExternalNormalizersTest.php:    assert(in_array($expected_value_jsonapi_normalization, [static::VALUE_ORIGINAL, static::VALUE_OVERRIDDEN], TRUE));
core/modules/jsonapi/tests/src/Functional/ExternalNormalizersTest.php:    assert(in_array($expected_value_jsonapi_denormalization, [static::VALUE_ORIGINAL, static::VALUE_OVERRIDDEN], TRUE));
core/modules/comment/tests/src/Functional/CommentCSSTest.php:        $this->assertIdentical($expectedJS, isset($settings['ajaxPageState']['libraries']) && in_array('comment/drupal.comment-new-indicator', explode(',', $settings['ajaxPageState']['libraries'])), 'drupal.comment-new-indicator library is present.');
core/modules/comment/tests/src/Functional/CommentEntityTest.php:    $this->assertFalse(isset($settings['ajaxPageState']['libraries']) && in_array('comment/drupal.comment-new-indicator', explode(',', $settings['ajaxPageState']['libraries'])), 'drupal.comment-new-indicator library is present.');
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.');
core/modules/tracker/tests/src/Functional/TrackerTest.php:    $this->assertIdentical($library_is_present, isset($settings['ajaxPageState']) && in_array('tracker/history', explode(',', $settings['ajaxPageState']['libraries'])), 'drupal.tracker-history library is present.');
core/modules/field/tests/src/Functional/EntityReference/EntityReferenceIntegrationTest.php:      $this->assertFalse(isset($dependencies[$key]) && in_array($referenced_entities[0]->getConfigDependencyName(), $dependencies[$key]), new FormattableMarkup('@type dependency @name does not exist.', ['@type' => $key, '@name' => $referenced_entities[0]->getConfigDependencyName()]));
core/modules/field_ui/tests/src/Kernel/EntityDisplayTest.php:    $value = $assertion ? in_array($key, $dependencies) : !in_array($key, $dependencies);

Seems like at least these should be in scope here:

core/modules/jsonapi/tests/src/Functional/ExternalNormalizersTest.php:    assert(in_array($expected_value_jsonapi_denormalization, [static::VALUE_ORIGINAL, static::VALUE_OVERRIDDEN], TRUE));

Perhaps all of them can be addressed here. Thoughts?

Thanks!
-Derek

Thanks for review in #8:

1. We've done #3126965: [backport] Replace assert* involving count() and an integer literal with assertCount(), #3128814: Replace assert* involving count() and an equality operator with assertCount(), already, which were quite large patches. We need another one to cover the case of count() vs a variable - or in general a final catchall for checking anything asserting with use of count(). Regex could be like the one in the (later retracted) #3126965-60: [backport] Replace assert* involving count() and an integer literal with assertCount().

2, 3 and 4. - assertContains on the array_keys of the array is equivalent to asserting the existence of the key in the array, but the latter seems to me more easily readable. Anyway using a variable makes sense, fixed.

5.Calls to assert() are PHP runtime, not PHPUnit - different can of worms. Please use >assert.*in_array for regex checking.
The other PHPUnit assertions are not straightforward (for example, you can split easily an assertion like $this->assertTrue($a && $b); in two assertions for $a and $b, but not that easily an $this->assertFalse($a && $b);, because NOT (A AND B) equals to NOT A OR NOT B and if you assertFalse on A, and fail, PHPUnit stops, while assertFalse on B could have passed). So to say that we would need more refactoring to address those, which is OOS here IMO.

+1 to all of #9. Agreed on raw assert(), I was skimming the output a bit. >assert is better for searches, I'll use that in the future. Agreed on scope weirdness for the others with assertFalse(A && B). Interdiff looks good. Bot's happy. No CS worries. RTBC.

Thanks,
-Derek

Committed/pushed to 9.1.x and cherry-picked to 9.0.x

Needs a reroll for 8.9.x

backport lgtm

Committed b22c720 and pushed to 8.9.x. Thanks!