Replace assert* involving an instanceof operator with assertInstanceOf()/assertNotInstanceOf()

#3.1 could be split into two separate assertions? The other ones can't due to the use of OR operator as far as I can see.

Thanks for the patch. Wow! Lots of changes... almost 700 lines o_O.

1) I went through every change and looked for:

• Class name was correct
• Value was correct
• Text message was unchanged
• False was handled correctly
• Split logic was correct (only one of these)

2) I didn't see anything "wrong" per se, but noticed a couple things:

a) Some have text messages and some do not. If there was a desire to standardize on this for this assert, then this would be the time to do it.

b) For the two with $group in the old code, why pass in NULL instead of not including the argument? See below.

+++ b/core/modules/system/tests/src/Functional/Theme/TwigRegistryLoaderTest.php
@@ -39,7 +39,7 @@ protected function setUp(): void {
-    $this->assertTrue($value instanceof TemplateWrapper, $message, $group);
+    $this->assertInstanceOf(TemplateWrapper::class, $value, $message, NULL);

+++ b/core/modules/system/tests/src/Kernel/Theme/TwigNamespaceTest.php
@@ -34,7 +34,7 @@ protected function setUp(): void {
-    $this->assertTrue($value instanceof TemplateWrapper, $message, $group);
+    $this->assertInstanceOf(TemplateWrapper::class, $value, $message, NULL);

3) Patch applies cleanly.

[mac:kristen:drupal-9.1.x-dev]$ patch -p1 < 3128905-7.patch 
patching file core/modules/action/tests/src/Kernel/Migrate/d6/MigrateActionsTest.php
patching file core/modules/action/tests/src/Kernel/Migrate/d7/MigrateActionsTest.php
patching file core/modules/block/tests/src/Kernel/BlockStorageUnitTest.php
patching file core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php
patching file core/modules/block/tests/src/Kernel/Migrate/d7/MigrateBlockTest.php
patching file core/modules/block_content/tests/src/Functional/BlockContentRevisionsTest.php
patching file core/modules/block_content/tests/src/Kernel/Migrate/MigrateBlockContentBodyFieldTest.php
patching file core/modules/block_content/tests/src/Kernel/Migrate/MigrateBlockContentTypeTest.php
patching file core/modules/block_content/tests/src/Kernel/Migrate/d7/MigrateCustomBlockTest.php
patching file core/modules/block_content/tests/src/Unit/Access/DependentAccessTest.php
patching file core/modules/ckeditor/tests/src/Functional/CKEditorAdminTest.php
patching file core/modules/comment/tests/src/Functional/CommentPreviewTest.php
patching file core/modules/comment/tests/src/Kernel/CommentItemTest.php
patching file core/modules/contact/tests/src/Kernel/Migrate/MigrateContactCategoryTest.php
patching file core/modules/datetime/tests/src/Kernel/DateTimeItemTest.php
patching file core/modules/field/tests/src/Kernel/Boolean/BooleanItemTest.php
patching file core/modules/field/tests/src/Kernel/ConfigFieldDefinitionTest.php
patching file core/modules/field/tests/src/Kernel/Email/EmailItemTest.php
patching file core/modules/field/tests/src/Kernel/EntityReference/EntityReferenceItemTest.php
patching file core/modules/field/tests/src/Kernel/FieldTypePluginManagerTest.php
patching file core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldFormatterSettingsTest.php
patching file core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldInstanceWidgetSettingsTest.php
patching file core/modules/field/tests/src/Kernel/Migrate/d7/MigrateViewModesTest.php
patching file core/modules/field/tests/src/Kernel/Number/NumberItemTest.php
patching file core/modules/field/tests/src/Kernel/ShapeItemTest.php
patching file core/modules/field/tests/src/Kernel/TestItemTest.php
patching file core/modules/field/tests/src/Kernel/TestObjectItemTest.php
patching file core/modules/field/tests/src/Kernel/Timestamp/TimestampItemTest.php
patching file core/modules/file/tests/src/Kernel/FileItemTest.php
patching file core/modules/file/tests/src/Kernel/Migrate/d6/MigrateFileTest.php
patching file core/modules/filter/tests/src/Kernel/FilterAPITest.php
patching file core/modules/image/tests/src/Kernel/ImageItemTest.php
patching file core/modules/image/tests/src/Kernel/Migrate/d7/MigrateImageStylesTest.php
patching file core/modules/jsonapi/tests/src/Kernel/Serializer/SerializerTest.php
patching file core/modules/language/tests/src/Kernel/Migrate/d6/MigrateLanguageTest.php
patching file core/modules/link/tests/src/Kernel/LinkItemTest.php
patching file core/modules/locale/tests/src/Kernel/LocaleConfigSubscriberTest.php
patching file core/modules/menu_link_content/tests/src/Kernel/MenuLinkContentDeriverTest.php
patching file core/modules/node/tests/src/Kernel/Migrate/d7/MigrateNodeTitleLabelTest.php
patching file core/modules/node/tests/src/Kernel/Migrate/d7/MigrateNodeTypeTest.php
patching file core/modules/node/tests/src/Kernel/NodeFieldOverridesTest.php
patching file core/modules/serialization/tests/src/Kernel/EntitySerializationTest.php
patching file core/modules/serialization/tests/src/Unit/Normalizer/TimestampItemNormalizerTest.php
patching file core/modules/shortcut/tests/src/Kernel/Migrate/d7/MigrateShortcutSetTest.php
patching file core/modules/shortcut/tests/src/Kernel/Migrate/d7/MigrateShortcutTest.php
patching file core/modules/system/tests/src/Functional/Theme/TwigRegistryLoaderTest.php
patching file core/modules/system/tests/src/Kernel/Action/ActionTest.php
patching file core/modules/system/tests/src/Kernel/PhpStorage/PhpStorageFactoryTest.php
patching file core/modules/system/tests/src/Kernel/Theme/TwigNamespaceTest.php
patching file core/modules/taxonomy/tests/src/Functional/Views/TaxonomyRelationshipTest.php
patching file core/modules/taxonomy/tests/src/Kernel/Migrate/d7/MigrateNodeTaxonomyTest.php
patching file core/modules/taxonomy/tests/src/Kernel/Migrate/d7/MigrateTaxonomyVocabularyTest.php
patching file core/modules/telephone/tests/src/Kernel/TelephoneItemTest.php
patching file core/modules/text/tests/src/Kernel/TextWithSummaryItemTest.php
patching file core/modules/user/tests/src/Functional/Views/AccessRoleTest.php
patching file core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserPictureFieldInstanceTest.php
patching file core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserPictureFieldTest.php
patching file core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserRoleTest.php
patching file core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserTest.php
patching file core/modules/views/tests/src/Functional/Handler/HandlerTest.php
patching file core/modules/views/tests/src/Functional/Plugin/ArgumentDefaultTest.php
patching file core/modules/views/tests/src/Functional/Plugin/ContextualFiltersBlockContextTest.php
patching file core/modules/views/tests/src/Functional/Plugin/DisplayTest.php
patching file core/modules/views/tests/src/Functional/Plugin/FilterTest.php
patching file core/modules/views/tests/src/Kernel/Handler/FieldFieldTest.php
patching file core/modules/views/tests/src/Kernel/ModuleTest.php
patching file core/modules/views/tests/src/Kernel/Plugin/ArgumentValidatorTest.php
patching file core/modules/views/tests/src/Kernel/Plugin/CacheTest.php
patching file core/modules/views/tests/src/Kernel/Plugin/DisplayExtenderTest.php
patching file core/modules/views/tests/src/Kernel/Plugin/DisplayKernelTest.php
patching file core/modules/views/tests/src/Kernel/Plugin/FieldOrLanguageJoinTest.php
patching file core/modules/views/tests/src/Kernel/Plugin/JoinTest.php
patching file core/modules/views/tests/src/Kernel/ViewExecutableTest.php
patching file core/modules/views/tests/src/Kernel/ViewStorageTest.php
patching file core/modules/views/tests/src/Kernel/Wizard/WizardPluginBaseKernelTest.php
patching file core/modules/views/tests/src/Unit/Controller/ViewAjaxControllerTest.php
patching file core/modules/views/tests/src/Unit/Plugin/display/PathPluginBaseTest.php
patching file core/modules/views_ui/tests/src/Functional/ViewEditTest.php
patching file core/tests/Drupal/FunctionalJavascriptTests/Tests/JSWebAssertTest.php
patching file core/tests/Drupal/KernelTests/Core/Config/ConfigSchemaTest.php
patching file core/tests/Drupal/KernelTests/Core/Database/FetchTest.php
patching file core/tests/Drupal/KernelTests/Core/Element/PathElementFormTest.php
patching file core/tests/Drupal/KernelTests/Core/Entity/EntityAccessControlHandlerTest.php
patching file core/tests/Drupal/KernelTests/Core/Entity/EntityFieldTest.php
patching file core/tests/Drupal/KernelTests/Core/Entity/EntityRepositoryTest.php
patching file core/tests/Drupal/KernelTests/Core/Entity/EntityTypedDataDefinitionTest.php
patching file core/tests/Drupal/KernelTests/Core/Entity/EntityValidationTest.php
patching file core/tests/Drupal/KernelTests/Core/Routing/MatcherDumperTest.php
patching file core/tests/Drupal/KernelTests/Core/TempStore/TempStoreDatabaseTest.php
patching file core/tests/Drupal/KernelTests/Core/TypedData/TypedDataDefinitionTest.php
patching file core/tests/Drupal/KernelTests/Core/TypedData/TypedDataTest.php
patching file core/tests/Drupal/KernelTests/Core/Url/LinkGenerationTest.php
patching file core/tests/Drupal/Tests/Component/Annotation/Doctrine/DocParserTest.php
patching file core/tests/Drupal/Tests/Core/Access/AccessResultTest.php
patching file core/tests/Drupal/Tests/Core/Database/EmptyStatementTest.php
patching file core/tests/Drupal/Tests/Core/DependencyInjection/ContainerBuilderTest.php
patching file core/tests/Drupal/Tests/Core/Layout/LayoutPluginManagerTest.php
patching file core/tests/Drupal/Tests/Core/Routing/TrustedRedirectResponseTest.php
patching file core/tests/Drupal/Tests/Core/StringTranslation/TranslationWrapperTest.php
patching file core/tests/Drupal/Tests/Core/Utility/LinkGeneratorTest.php

4) Tests pass so I'm inclined to RTBC but am curious about thoughts on 2b above first.

I grepped the 9.1 dev codebase after applying the patch and found some stragglers for assertTrue that could be addressed so marking this back to "Needs work". Please review comments above as well. Thanks.

[mac:kristen:core]$ grep -r instanceof . | grep assertTrue
./tests/Drupal/KernelTests/Core/Entity/EntityFieldTest.php:          $this->assertTrue(!isset($value) || is_scalar($value) || $value instanceof EntityInterface, $entity_type . ": Value $value_name of item $delta of field $name is a primitive or an entity.");
./modules/config_translation/tests/src/Functional/ConfigTranslationUiTest.php:    $this->assertTrue($textarea instanceof NodeElement, new FormattableMarkup('Disabled field @id exists.', [
./modules/system/tests/src/Kernel/Theme/ThemeTest.php:      $this->assertTrue($output instanceof MarkupInterface || is_string($output), new FormattableMarkup('\Drupal::theme() returns an object that implements MarkupInterface or a string for data type @type.', ['@type' => $type]));
./modules/views/tests/src/Functional/Handler/HandlerAllTest.php:            $this->assertTrue($handler instanceof HandlerBase, new FormattableMarkup(

Thanks @jungle for the original patch and @Kristen Pol for the review!

Re #9.2b I agree that $group should be removed and have done so in this patch. I also wonder if we should refactor away assertTwigTemplate entirely as it is only called three times but that is perhaps out of scope as this patch is already quite big.

Re #10.2 and #10.4 I have fixed these in this patch but #10.1 and #10.3 cannot be refactored due to the OR operator, there is no way of saying assertInstanceOf() with multiple types (or scalar types) as far as I know.

Re #9.2a I think we probably could remove *some* of the messages in this patch, e.g. the ones in TwigNamespaceTest seem helpful, whereas the one HandlerAllTest does not. Removing them all seems a step too far and probably needs separate discussion perhaps in a followup? There is also a separate argument for removing FormattableMarkup from the messages we do want to keep, but that is definitely out of scope here.

Thanks for the update!

1) The changes look good and address items in #9.2b, #10.2, and #10.4. I agree that #10.1 and #10.3 can't be addressed with the same approach because of the OR logic.

2) Re #9.2a, I don't have strong opinions on whether any messages should be added or remove in this patch... only that if that should be cleaned up, it might be good to do it in this patch... though I can see the argument that they be handled on their own.

3) Patch applied cleanly to 9.1 dev and included the 2 new files:

• core/modules/config_translation/tests/src/Functional/ConfigTranslationUiTest.php
• core/modules/views/tests/src/Functional/Handler/HandlerTest.php

4) Checking the code after the patch is applied, we're just left with the two that won't be changed from #10.1 and #10.3:

./core/tests/Drupal/KernelTests/Core/Entity/EntityFieldTest.php:          $this->assertTrue(!isset($value) || is_scalar($value) || $value instanceof EntityInterface, $entity_type . ": Value $value_name of item $delta of field $name is a primitive or an entity.");
./core/modules/system/tests/src/Kernel/Theme/ThemeTest.php:      $this->assertTrue($output instanceof MarkupInterface || is_string($output), new FormattableMarkup('\Drupal::theme() returns an object that implements MarkupInterface or a string for data type @type.', ['@type' => $type]));

5) Given all the above, if tests pass, then I'd mark this RTBC.

Spotted a number of assert(... instanceof ...) in tests, which I think are missing a call to $this? Should we fix these here?

core/modules/config/tests/src/Functional/ConfigExportUITest.php:    assert($file_system instanceof FileSystemInterface);
core/modules/file/tests/src/Kernel/SaveDataTest.php:    assert($stream_wrapper_manager instanceof StreamWrapperManagerInterface);
core/modules/file/tests/src/Kernel/SaveDataTest.php:    assert($stream_wrapper_manager instanceof StreamWrapperManagerInterface);
core/modules/file/tests/src/Kernel/SaveDataTest.php:    assert($stream_wrapper_manager instanceof StreamWrapperManagerInterface);
core/modules/file/tests/src/Kernel/SaveDataTest.php:    assert($stream_wrapper_manager instanceof StreamWrapperManagerInterface);
core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php:        assert($created_entity instanceof RevisionableInterface);
core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php:        assert($created_entity instanceof RevisionableInterface);
core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php:      assert($entity instanceof RevisionableInterface);
core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php:    assert(is_null($via_link) || $via_link instanceof Url);
core/modules/jsonapi/tests/src/Kernel/EventSubscriber/ResourceObjectNormalizerCacherTest.php:    assert($normalized_links instanceof CacheableNormalization);
core/modules/jsonapi/tests/src/Kernel/Normalizer/RelationshipNormalizerTest.php:    assert($actual instanceof CacheableNormalization);
core/modules/search/tests/src/Functional/SearchMultilingualEntityTest.php:    assert($search_index instanceof SearchIndexInterface);
core/modules/search/tests/src/Functional/SearchNodeUpdateAndDeletionTest.php:    assert($search_index instanceof SearchIndexInterface);
core/modules/search/tests/src/Functional/SearchRankingTest.php:    assert($search_index instanceof SearchIndexInterface);
core/modules/search/tests/src/Kernel/SearchMatchTest.php:    assert($search_index instanceof SearchIndexInterface);
core/tests/Drupal/KernelTests/Core/File/StreamWrapperTest.php:    assert($file_system instanceof FileSystemInterface);

Right. I did notice that before but then made the assumption that only the assertTrue/assertFalse were supposed to be handled. If those need to be updated, I'd vote for doing it here.

#14 it's not clear why assert() is being used in tests, honestly. AFAIK it should be used in runtime code and 'switched off' from productiive execution via php.ini settings. I just git blamed the last from the list in #14 and it was introduced with #3039026: Deprecate file_directory_temp() and move to FileSystem service.

IMHO we should deal with #14 separately so that if anyone sees a reason to have that in tests, that discussion wouldn't hold this cleanup here.

Looking more closely I agree that we should move #14 to a possible followup, some of them look to be valid runtime assertions - but not all - so discussion would be needed.

Looks good but needs a reroll. By the way +1 to doing one assertion type at a time, good way to split these up.

The assert() from #14 are definitely out of scope here - I assume they were added for an easier time debugging failing tests, but either way should be a separate issue.

@jungle if you just reroll resolving patch conflicts without adding additional code, I've seen it's OK and accepted by committers when you just say so in the comment and set back directly to RTBC.

#21 I think it's more a matter of trust in cases like this... so no strict rules. But maybe @catch can provide his view. Would be good to know anyway. Thx

Generally I don't count re-RTBCs as self-RTBCing if it's a re-roll for something small.

If it's a pure re-roll definitely OK. If it's fixing a typo or small coding style issue, usually OK (good to have an interdiff for transparency).

Also if you worked on a patch, someone else RTBCs it, there's some changes implemented by someone else, then you RTBC those changes, this is usually OK but it needs some judgement - i.e. the more people involved in an issue and the older it is, the more distributed the responsibility.

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

Filed #3130811: Remove redundant $message from assert(Not)InstanceOf calls as a possible follow up. See you there.

This should probably go back to 8.9.x if we are also to backport #3130811: Remove redundant $message from assert(Not)InstanceOf calls

Quick attempt at a backport, there might be some more calls we can do here.

Found some more in tests that have been removed or refactored in 9.0.x.

So this D8.9 overall is going to be covered combined in #3130811-19: Remove redundant $message from assert(Not)InstanceOf calls, so marking this back to fixed.