Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jesse.d’s picture

I added the following test to GenericCacheBackendUnitTest.php:

 /**
   * Test Drupal\Core\Cache\CacheBackendInterface::removeBin().
   */
  public function testRemoveBin() {
    $backend = $this->getCacheBackend();
    $unrelated = $this->getCacheBackend('bootstrap');

    // Set both expiring and permanent keys.
    $backend->set('test1', 1, Cache::PERMANENT);
    $backend->set('test2', 3, time() + 1000);
    $unrelated->set('test3', 4, Cache::PERMANENT);

    $backend->removeBin();

    $this->assertFalse($backend->get('test1'), 'First key has been deleted.');
    $this->assertFalse($backend->get('test2'), 'Second key has been deleted.');
    $this->assertTrue($unrelated->get('test3'), 'Item in other bin is preserved.');
  }

The test initially failed, then passed after the patch was applied, but I'm a little wary that I reverse engineered an outcome so more reviewers would be good.

olli’s picture

Your test looks good to me.

olli’s picture

One small thing: $backend->get('test2') could pass the second parameter $backend->get('test2', TRUE).

olli’s picture

Title: PhpBackend::removeBin() does not remove the bin » Cache\PhpBackend::removeBin() does not remove the bin
FileSize
1.18 KB
1.61 KB

Here's the test from #1.

The last submitted patch, 4: 2267641-4-test_only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 4: 2267641-4.patch, failed testing.

olli’s picture

Status: Needs work » Needs review
FileSize
2.03 KB
430 bytes
Berdir’s picture

Status: Needs review » Needs work
Issue tags: +Novice
+++ b/core/modules/system/src/Tests/Cache/GenericCacheBackendUnitTestBase.php
@@ -666,4 +666,23 @@ public function testInvalidateAll() {
+    $backend = $this->getCacheBackend();
+    $unrelated = $this->getCacheBackend('bootstrap');
...
+    $this->assertFalse($backend->get('test1'), 'First key has been deleted.');
+    $this->assertFalse($backend->get('test2', TRUE), 'Second key has been deleted.');
+    $this->assertTrue($unrelated->get('test3'), 'Item in other bin is preserved.');

Test looks good, but $unrelated is a bit a strange name, maybe renamed to $backend_a and $backend_b?

Tagging novice to update this.

rpayanm’s picture

Status: Needs work » Needs review
FileSize
1.58 KB
3.28 KB

Please review :)

Berdir’s picture

Status: Needs review » Needs work

You renamed it in the existing testDeleteAll() method (now I see where that came from), but not in the new test method testRemoveBin() :)

rpayanm’s picture

Status: Needs work » Needs review
FileSize
3.18 KB
4.78 KB

On two more method! :D

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Great, looks good now.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 87be19c and pushed to 8.0.x. Thanks!

  • alexpott committed 87be19c on 8.0.x
    Issue #2267641 by olli, rpayanm: Cache\PhpBackend::removeBin() does not...

Status: Fixed » Closed (fixed)

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