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.

#3 2263287.3.patch7.42 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,829 pass(es). View
#3 0-3-interdiff.txt2.56 KBalexpott
cachedstorage-test.patch8.5 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,664 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more


alexpott’s picture

Berdir’s picture

  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

2.56 KB
7.42 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,829 pass(es). View
  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

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

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.