CacheClearCase::testClearArray() sets a persistent variable that has no effect

(This is a bug, even if minor.)

Not even Drupal 6.x nor Drupal 5.x uses such value. I was thinking of code left-over from previous versions, but it is not so.

Thanks for reporting and working on this.

I did some search and found out that the changes in test were committed in #333171: Optimize the field cache → . If we read the discussion in that issue, we can see that it was proposed to add the new variable cache_clear_threshold and it was in patch in #18. But then it was removed, but was left in the test (seems like unintentionally). So I agree we can remove this safely.

I think there is one question left here - do we need the rest of the test, or can we delete all other line from cache_clear_threshold to the end of the function? I do not see any difference between:

    // Clear two entries using an array.
    cache_clear_all(array('test_cid_clear1', 'test_cid_clear2'), $this->default_bin);
    $this->assertFalse($this->checkCacheExists('test_cid_clear1', $this->default_value)
                       || $this->checkCacheExists('test_cid_clear2', $this->default_value),
                       'Two cache entries removed after clearing with an array.');

and

    cache_clear_all(array('test_cid_clear1', 'test_cid_clear2', 'test_cid_clear3'), $this->default_bin);
    $this->assertFalse($this->checkCacheExists('test_cid_clear1', $this->default_value)
                       || $this->checkCacheExists('test_cid_clear2', $this->default_value)
                       || $this->checkCacheExists('test_cid_clear3', $this->default_value),
                       'All cache entries removed when the array exceeded the cache clear threshold.');

Just that the first is deleting two entries and second three entries. If I have not missed anything, I think we can safely remove the remaining part of the test in this cleanup as well. What do you think?

I was a bit confused by the following lines in the test.

// Set the cache clear threshold to 2 to confirm that the full bin is cleared
// when the threshold is exceeded.
variable_set('cache_clear_threshold', 2);
cache_set('test_cid_clear1', $this->default_value, $this->default_bin);
cache_set('test_cid_clear2', $this->default_value, $this->default_bin);
$this->assertTrue($this->checkCacheExists('test_cid_clear1', $this->default_value)

Reading it now after reading your comment, I think the last three lines are merely testing the effect of variable_set('cache_clear_threshold', 2); and I agree they should be removed.

Actually, all these lines can be removed.

    cache_set('test_cid_clear1', $this->default_value, $this->default_bin);
    cache_set('test_cid_clear2', $this->default_value, $this->default_bin);
    $this->assertTrue($this->checkCacheExists('test_cid_clear1', $this->default_value)
                      && $this->checkCacheExists('test_cid_clear2', $this->default_value),
                      'Two cache entries were created.');
    cache_clear_all(array('test_cid_clear1', 'test_cid_clear2', 'test_cid_clear3'), $this->default_bin);
    $this->assertFalse($this->checkCacheExists('test_cid_clear1', $this->default_value)
                       || $this->checkCacheExists('test_cid_clear2', $this->default_value)
                       || $this->checkCacheExists('test_cid_clear3', $this->default_value),
                       'All cache entries removed when the array exceeded the cache clear threshold.');

The last line makes clear what their purpose is: All cache entries removed when the array exceeded the cache clear threshold.
Since there is no longer a cache clear threshold, they are not useful.

Yes, that is what I meant (sorry if I was not clear enough). The clear cache behavior with an array is still being tested on lines which are left in that function. I think this change looks good now. Thanks!