We should be testing the CachedStorage class using ConfigStorageTestBase since we no longer use it for the active configuration. This has the added benefit of making #2262861: Add concept of collections to config storages smaller since that introduces changes to CachedStorage that need testing.

CommentFileSizeAuthor
#3 2263287.3.patch7.42 KBalexpott
#3 0-3-interdiff.txt2.56 KBalexpott
cachedstorage-test.patch8.5 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Related issues: +#2262861: Add concept of collections to config storages
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented 
  1. +++ b/core/modules/config/lib/Drupal/config/Tests/Storage/CachedStorageTest.php
@@ -0,0 +1,120 @@
+    // Try writing to the file storage first catch exceptions so that we can do
+    // the invalid data tests.
+    try {
+      $this->filestorage->write($name, $data);
+      $this->cache->set($name, $data);
+    }
+    catch (\Exception $e) {
+    }

    A few lines above it says that we don't do the invalid data tests? 

  2. +++ b/core/modules/config/lib/Drupal/config/Tests/Storage/ConfigStorageTestBase.php
@@ -150,6 +158,22 @@ function testCRUD() {
   /**
+   * Test that invalid data can not be read from the storage.
+   */
+  public function testInvalidData() {
+    $name = 'config_test.storage';
+
+    // Reading a name containing non-decodeable data returns FALSE.
+    $this->insert($name, '');
+    $data = $this->storage->read($name);
+    $this->assertIdentical($data, FALSE);
+
+    $this->update($name, 'foo');
+    $data = $this->storage->read($name);
+    $this->assertIdentical($data, FALSE);
+  }

    Why are we even testing something like this? I noticed a lot of weird over-protective code in the CachedStorage that verified that the things we got back from the cache are valid and so on, we don't do that anywhere else...

    that's like writing a test that inserts bogus values in the the node table and then tests if the entity storage can deal with that?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
FileSize
0-3-interdiff.txt2.56 KB
2263287.3.patch7.42 KB
  1. A hang up from trying to get them to work :)
  2. I agree the invalid data tests are bogus - let's get rid.

In IRC @Berdir asked me if the PHPUnit test Drupal\Tests\Core\Config\CachedStorageTest covers this. I think we gain consistency since all classes that implement StorageInterface are tested with the same base test. So if we add further test coverage to this (as we do in #2262861: Add concept of collections to config storages) then CachedStorage automatically gets tested.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Priority: Major » Critical
Status: Needs review » Reviewed & tested by the community

Two criticals are postponed on this (#2262861: Add concept of collections to config storages and #2224887: Language configuration overrides should have their own storage). The later one is even a beta blocker. So elevating to critical.

Looks to me like all changes proposed by Berdir have been fixed and the patch makes sense to me too :)

  • Commit 5be9d3e on 8.x by catch: 
    Issue #2263287 by alexpott: Test the CachedStorage class using...
catch’s picture

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

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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