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

See #1938068: Convert UnitTestBase to PHPUnit.

Files: 
CommentFileSizeAuthor
#11 interdiff.txt1.28 KBjhedstrom
#11 phpstorage-phpunit-2051385-11.patch3.23 KBjhedstrom
PASSED: [[SimpleTest]]: [MySQL] 59,371 pass(es).
[ View ]
#9 interdiff.txt936 bytesjhedstrom
#9 phpstorage-phpunit-2051385-09.patch3.03 KBjhedstrom
PASSED: [[SimpleTest]]: [MySQL] 59,095 pass(es).
[ View ]
#5 interdiff.txt1.26 KBjhedstrom
#5 phpstorage-phpunit-2051385-04.patch2.99 KBjhedstrom
PASSED: [[SimpleTest]]: [MySQL] 57,918 pass(es).
[ View ]
#1 phpstorage-phpunit-2051385-01.patch2.49 KBjhedstrom
PASSED: [[SimpleTest]]: [MySQL] 57,570 pass(es).
[ View ]

Comments

jhedstrom’s picture

Status:Active» Needs review
StatusFileSize
new2.49 KB
PASSED: [[SimpleTest]]: [MySQL] 57,570 pass(es).
[ View ]

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

+++ 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

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

dawehner’s picture

Status:Needs review» Needs work

Well, without that testing delete files is quite pointless.

jhedstrom’s picture

Status:Needs work» Needs review
StatusFileSize
new2.99 KB
PASSED: [[SimpleTest]]: [MySQL] 57,918 pass(es).
[ View ]
new1.26 KB

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

dawehner’s picture

I miss a check that the files are deleted.

ParisLiakos’s picture

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

Status:Needs review» Needs work
jhedstrom’s picture

Status:Needs work» Needs review
StatusFileSize
new3.03 KB
PASSED: [[SimpleTest]]: [MySQL] 59,095 pass(es).
[ View ]
new936 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

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

Status:Needs work» Needs review
StatusFileSize
new3.23 KB
PASSED: [[SimpleTest]]: [MySQL] 59,371 pass(es).
[ View ]
new1.28 KB

Added groups.

Mile23’s picture

Status:Needs review» Reviewed & tested by the community

Applies, passes. Yay.

alexpott’s picture

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.