Task to expand phpunit coverage for the \Drupal\Component\PhpStorage classes.

See #1938068: Convert UnitTestBase to PHPUnit.

CommentFileSizeAuthor
#11 interdiff.txt1.28 KBjhedstrom
#11 phpstorage-phpunit-2051385-11.patch3.23 KBjhedstrom
#9 interdiff.txt936 bytesjhedstrom
#9 phpstorage-phpunit-2051385-09.patch3.03 KBjhedstrom
#5 interdiff.txt1.26 KBjhedstrom
#5 phpstorage-phpunit-2051385-04.patch2.99 KBjhedstrom
#1 phpstorage-phpunit-2051385-01.patch2.49 KBjhedstrom

Comments

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Status: Active » Needs review
StatusFileSize
new phpstorage-phpunit-2051385-01.patch2.49 KB

Here's a start. It adds 100% coverage to FileStorage and FileReadOnlyStorage. Some of these components are quite messy, with calls to global $conf and other Drupal procedural functions.

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/tests/Drupal/Tests/Component/PhpStorage/FileStorageTest.phpundefined
@@ -72,4 +72,34 @@ function testReadOnly() {
+  public function testDeleteAll() {
...
+
+    $php = $this->storageFactory->get('simpletest');
+    $this->assertTrue($php->deleteAll());
+    $this->assertFalse(file_exists(sys_get_temp_dir() . '/php/simpletest'), 'File storage directory still exists after call to deleteAll().');
...
+    // Should still return TRUE if directory has already been deleted.
+    $this->assertTrue($php->deleteAll());

What kind of code actually takes care about creating the test files? I could not figure that out to be honest.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented

This test currently doesn't add any files there, but that would be easy enough to do.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Needs work

Well, without that testing delete files is quite pointless.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Status: Needs work » Needs review
StatusFileSize
new phpstorage-phpunit-2051385-04.patch2.99 KB
new interdiff.txt1.26 KB

This patch writes some data out prior to calling deleteAll().

dawehner’s picture

dawehner
Primary language German
commented

I miss a check that the files are deleted.

ParisLiakos’s picture

ParisLiakos commented

a seriously nitpicky addition to #6:

+++ b/core/tests/Drupal/Tests/Component/PhpStorage/FileStorageTest.php
@@ -2,7 +2,7 @@
+ * Contains Drupal\Tests\Component\PhpStorage\FileStorageTest.

Missing backslash in front of Drupal

ParisLiakos’s picture

ParisLiakos commented
Status: Needs review » Needs work
jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Status: Needs work » Needs review
StatusFileSize
new phpstorage-phpunit-2051385-09.patch3.03 KB
new interdiff.txt936 bytes

This should address #6 and #7. Note that there was a line already checking that the *directory* was removed, this adds a check to ensure that the file is not load-able.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs review » Needs work

Yet another use case for vfsStream :-) #2095037: Add vfsStream as a dependency in composer.json.

Also needs @group Drupal and @group PhpStorage

Other than that, RTBC.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Status: Needs work » Needs review
StatusFileSize
new phpstorage-phpunit-2051385-11.patch3.23 KB
new interdiff.txt1.28 KB

Added groups.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs review » Reviewed & tested by the community

Applies, passes. Yay.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 25f081a and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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