Problem/Motivation

As title

Proposed resolution

For example:

-    $this->assertFalse(file_exists($file_path), 'Stale file is deleted.');
+    $this->assertFileNotExists($file_path, 'Stale file is deleted.');

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#21 interdiff_19-21.txt1.03 KBSpokje
#21 3131088-21.patch60.56 KBSpokje
#19 3131088-19.patch60.55 KBSpokje
#18 3131088-18.patch60.59 KBSpokje
#18 interdiff_14-18.txt555 bytesSpokje
#14 3131088-12.patch61.33 KBSpokje
#9 3131088-9.patch59.36 KBSpokje
#7 3131088-7.patch58.83 KBSpokje
#3 3131088-3.patch61.11 KBlongwave
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

longwave created an issue. See original summary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Title: Replace assertions involving calls to file_exists with assertFileExists()/assertFileNotExists() » Replace assertions involving calls to file_exists with assertFileExists()/assertFileNotExists() or assertDirectoryExists()/assertDirectoryNotExists()

file_exists() also returns TRUE for directories, so the tests can be more accurate here about what they are looking for.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Active » Needs review
FileSize
3131088-3.patch61.11 KB
msuthars’s picture

msuthars
Location Pune
CreditAttribution: msuthars as a volunteer and at QED42 for Drupal India Association commented
Assigned: Unassigned » msuthars
msuthars’s picture

msuthars
Location Pune
CreditAttribution: msuthars as a volunteer and at QED42 for Drupal India Association commented
Assigned: msuthars » Unassigned
Status: Needs review » Reviewed & tested by the community

@longwave Patch looks fine for me. I reviewed and tested it and it working as expected.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje at Annertech commented
FileSize
3131088-7.patch58.83 KB

Reroll of #3 against latest 9.1.x

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje at Annertech commented
Assigned: Unassigned » Spokje

Crap, missed a few, new reroll coming shortly.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje at Annertech commented
FileSize
3131088-9.patch59.36 KB

Re-reroll of #3 against latest 9.1.x

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje at Annertech commented
Assigned: Spokje » Unassigned
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll

Back to RTBC after TestBot Greenness.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 9.1.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

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

Needs a reroll for 8.9.x

  • catch committed 222e104 on 9.1.x 
    Issue #3131088 by Spokje, longwave: Replace assertions involving calls...

  • catch committed 811bcb9 on 9.0.x 
    Issue #3131088 by Spokje, longwave: Replace assertions involving calls...
Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje at Annertech commented
FileSize
3131088-12.patch61.33 KB

Reroll of #9 against latest 8.9.x

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje at Annertech commented
Status: Needs work » Needs review

TesBot Greenness on D8.9.x as well.

Note:

There are 2 instances of $this->assertTrue(file_exists($filename), $message); left.
In Drupal\simpletest\Tests\WebTestBaseInstallTest and Drupal\simpletest\Tests\SimpleTestTest, both are based on Drupal\simpletest\WebTestBase which apparently doesn't support assertFileExists/assertFileNotExists

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje at Annertech commented
Issue tags: -Needs reroll

Removed Reroll tag

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented 
+++ b/core/modules/simpletest/src/TestBase.php
@@ -734,7 +734,7 @@ protected function assertIdenticalObject($object1, $object2, $message = '', $gro
-    return $this->assertFalse(file_exists(DRUPAL_ROOT . '/' . $this->siteDirectory . '/error.log'), 'PHP error.log is empty.');
+    return $this->assertFileNotExists(DRUPAL_ROOT . '/' . $this->siteDirectory . '/error.log');

I don't think this exists in SimpleTest - the patch is still green because this won't be called any more by any core tests, but contrib might still use it.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje at Annertech commented
FileSize
interdiff_14-18.txt555 bytes
3131088-18.patch60.59 KB

@longwave Good catch! Got a bit too enthusiastic there...
I've removed it from this patch.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje at Annertech commented
FileSize
3131088-19.patch60.55 KB

Grmbl, #3125391 got in and made my patch invalid.

Reroll against latest 8.9.x

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Needs work 
  1. +++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerCustomConfigDirectoryCreateTest.php
@@ -38,12 +38,11 @@ protected function prepareEnvironment() {
-    $this->assertTrue(file_exists($this->publicFilesDirectory . '/config_custom') && is_dir($this->publicFilesDirectory . '/config_custom'), "The directory {$this->publicFilesDirectory}/custom_config exists.");
+    $this->assertFileExists($this->publicFilesDirectory . '/config_custom') && is_dir($this->publicFilesDirectory . '/config_custom');

    This isn't quite right, this needs to become assertDirectoryExists()

  2. +++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerCustomConfigDirectoryCreateTest.php
@@ -38,12 +38,11 @@ protected function prepareEnvironment() {
-    $this->assertTrue(file_exists($sync_directory) && is_dir($sync_directory), "The directory {$sync_directory} exists.");
-
+    $this->assertFileExists($sync_directory) && is_dir($sync_directory);

    Same here.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje at Annertech commented
Status: Needs work » Needs review
FileSize
3131088-21.patch60.56 KB
interdiff_19-21.txt1.03 KB

Right...

Seems like yesterday wasn't my best rerolling day ever.

@longwave You're absolutely right with your comments in #20
I've went through patch #19 but (of course) those two seem to be the only ones I've messed up.

Not sure why they weren't detected in the patch in #14 which included this "wrongness" and still turned out green.

Anyway: New patch addressing comments in #20 attached.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Reviewed & tested by the community

No worries, thanks for working on this - all looks good now.

  • catch committed 2fc0649 on 8.9.x 
    Issue #3131088 by Spokje, longwave: Replace assertions involving calls...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.9.x, thanks!

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

The 8.9.x patch here has some bad code, opened #3133103: [8.9 only] InstallerCustomConfigDirectoryCreateTest has bad assertions to address this.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.