Replace assertions involving calls to is_file with assertFileExists()/assertFileNotExists()

General +1. Haven't reviewed yet, but fixing the title.

1. +++ b/core/modules/file/tests/src/Functional/FileManagedTestBase.php
   @@ -194,7 +194,7 @@ public function createUri($filepath = NULL, $contents = NULL, $scheme = NULL) {
   -    $this->assertTrue(is_file($filepath), t('The test file exists on the disk.'), 'Create test file');
   +    $this->assertFileExists($filepath, t('The test file exists on the disk.'));
   

   The previous code was wrong, too, but the assertion message shouldn't be in t().

2. +++ b/core/modules/file/tests/src/Functional/SaveUploadFormTest.php
   @@ -71,10 +71,10 @@ protected function setUp(): void {
   +    $this->assertFileExists($this->image->getFileUri(), "The image file we're going to upload exists.");
   ...
   +    $this->assertFileExists($this->phpfile->uri, 'The PHP file we are going to upload exists.');
   

   Possible scope creep, but it's a bit weird the first message uses "we're" and wraps whole thing in double quotes, while the 2nd message uses "we are" and single quotes. I think we should standardize on 'we are' with single quotes...

3. +++ b/core/modules/file/tests/src/Functional/SaveUploadTest.php
   @@ -68,10 +68,10 @@ protected function setUp(): void {
   +    $this->assertFileExists($this->image->getFileUri(), "The image file we're going to upload exists.");
   

   Again here.

4. +++ b/core/modules/system/tests/src/Functional/Form/StateValuesCleanAdvancedTest.php
   @@ -47,7 +47,7 @@ public function testFormStateValuesCleanAdvanced() {
   +    $this->assertFileExists($this->image->uri, "The image file we're going to upload exists.");
   

   And here.

   I dunno. Don't listen to me. I almost never get scope questions right. If it were me, I'd fix these to be consistent while we're already touching the files. Someone else will probably say we should jump through all the hoops to have a separate issue for this. /shrug

5. +++ b/core/tests/Drupal/KernelTests/Core/File/DirectoryTest.php
   @@ -110,15 +110,15 @@ public function testFileCheckDirectoryHandling() {
   +    $this->assertFileExists($default_scheme . '://.htaccess', 'Successfully re-created the .htaccess file in the files directory.');
   

   More scope creep, but to disambiguate this message from the one a few lines up, I think it should mention "cron". Something like:

   "Running cron successfully re-created the .htaccess file in the files directory."

   /shrug

6. % egrep -r 'assert.*is_file' core
   

   After applying the patch (on the 8.9.x branch), I'm still seeing these:

   core/modules/file/src/Tests/FileManagedTestBase.php:    $this->assertTrue(is_file($filepath), t('The test file exists on the disk.'), 'Create test file');
   core/modules/file/src/Tests/FileFieldTestBase.php:    $this->assertTrue(is_file($file->getFileUri()), $message);
   core/modules/file/src/Tests/FileFieldTestBase.php:    $this->assertFalse(is_file($file->getFileUri()), $message);
   

   However, on the 9.1.x branch, this patch removes everything. Maybe that's good enough. Unclear how far this will be backported.

Maybe needs work for the last point (if this will be backported that far). The other points are possibly scope creep that may require follow-ups. I hope not. But please don't spend time on them until we get confirmation from core folks on desired scope.

Thanks!
-Derek

For the same reasons outlined in #3130811: Remove redundant $message from assert(Not)InstanceOf calls, for points 1-5 in #5, my suggestion would be to just remove the messages and let standard PHPUnit manage them. This will take most complexity away.

With regards to #5.6, IMHO we should just target 9.1 and 9.0 now. If committers later want so, they will ask for a backport patch. See #3128905: Replace assert* involving an instanceof operator with assertInstanceOf()/assertNotInstanceOf().

As suggested in #6 I agree that most of the assertion messages are useless here and generally it would be better to use the default PHPUnit failure message, so I have removed them all with the exception of ResolvedLibraryDefinitionsFilesMatchTest which I think is still a helpful message. However, PHPUnit messages describe the failure as opposed to the success, so I have inverted the message here.

RTBC, removes occurrences of assert* calls using calls of is_file().

We will need a followup here to check if the remaining usages of the $message argument in calls to assertFile(Not)Exists can be removed, searching for assertFile(?:Not)?Exists\(.*,.*[\'"] returns 27 occurrences.

Committed/pushed to 9.1.x and backported back to 8.9.x, thanks!

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.