Copilot commented on code in PR #15961:
URL: https://github.com/apache/grails-core/pull/15961#discussion_r3560654626
##########
grails-cache/src/test/groovy/grails/plugin/cache/GrailsConcurrentLinkedMapCacheTests.groovy:
##########
@@ -18,107 +18,138 @@
*/
package grails.plugin.cache
+import java.util.concurrent.ConcurrentMap
+
import org.springframework.cache.support.SimpleValueWrapper
-import com.googlecode.concurrentlinkedhashmap.ConcurrentLinkedHashMap
-import org.junit.Test
+import spock.lang.Specification
/**
* @author Jakob Drangmeister
*/
-class GrailsConcurrentLinkedMapCacheTests {
-
- @Test
- void testCreateCache() {
- GrailsConcurrentLinkedMapCache smallCache = new
GrailsConcurrentLinkedMapCache("smallCache", 1000)
+class GrailsConcurrentLinkedMapCacheTests extends Specification {
+
+ void 'creates caches with configured capacity and null value policy'() {
+ when:
+ GrailsConcurrentLinkedMapCache smallCache = new
GrailsConcurrentLinkedMapCache('smallCache', 1000)
+
+ then:
+ smallCache.name == 'smallCache'
+ smallCache.nativeCache instanceof ConcurrentMap
+ smallCache.capacity == 1000
+ smallCache.allowNullValues
+
+ when:
+ GrailsConcurrentLinkedMapCache bigCache = new
GrailsConcurrentLinkedMapCache('bigCache', 5000000, false)
+
+ then:
+ bigCache.name == 'bigCache'
+ bigCache.nativeCache instanceof ConcurrentMap
+ bigCache.capacity == 5000000
+ !bigCache.allowNullValues
+ }
- assert smallCache.getName() == "smallCache"
- assert smallCache.getNativeCache() instanceof ConcurrentLinkedHashMap
- assert smallCache.getCapacity() == 1000
- assert smallCache.isAllowNullValues() == true
+ void 'exposes Caffeine native cache as concurrent map'() {
+ given:
+ GrailsConcurrentLinkedMapCache cache = new
GrailsConcurrentLinkedMapCache('cache', 10, true)
- GrailsConcurrentLinkedMapCache bigCache = new
GrailsConcurrentLinkedMapCache("bigCache", 5000000, false)
+ when:
+ cache.put("key", "value")
- assert bigCache.getName() == "bigCache"
- assert bigCache.getNativeCache() instanceof ConcurrentLinkedHashMap
- assert bigCache.getCapacity() == 5000000
- assert bigCache.isAllowNullValues() == false
+ then:
+ cache.nativeCache.get('key') == 'value'
+
cache.nativeCache.getClass().name.startsWith('com.github.benmanes.caffeine.cache.')
Review Comment:
This assertion depends on the concrete implementation class name returned by
Caffeine's asMap() view. That is not part of Caffeine's API contract and may
change across versions, making the test brittle for no functional gain. Prefer
asserting observable behavior (e.g., that hottestKeys includes the inserted
key).
##########
grails-doc/src/en/guide/cache/cacheUsage/cacheConfiguration.adoc:
##########
@@ -28,7 +28,7 @@ There are a few configuration options for the plugin; these
are specified in
*Property*,*Default*,*Description*
grails.cache.enabled,`true`,Whether to enable the plugin
grails.cache.clearAtStartup,`false`,Whether to clear all caches at startup
-grails.cache.cacheManager,GrailsConcurrentMapCacheManager,Cache Manager to
use. Default cache manager uses Spring Frameworks ConcurrentMapCache which
might grow limitless. If you cannot predict how many cache entries you are
going to generate use "GrailsConcurrentLinkedMapCacheManager" instead which
uses com.googlecode.concurrentlinkedhashmap.ConcurrentLinkedHashMap and limits
by default to 10000 entries per cache.
+grails.cache.cacheManager,GrailsConcurrentMapCacheManager,Cache Manager to
use. Default cache manager uses Spring Frameworks ConcurrentMapCache which
might grow limitless. If you cannot predict how many cache entries you are
going to generate use "GrailsConcurrentLinkedMapCacheManager" instead. In
Grails 8.1 this manager keeps the public Grails cache API while using
Caffeine-backed bounded caches; the old concurrentlinkedhashmap-lru dependency
path is deprecated.
Review Comment:
The docs mention "In Grails 8.1" even though this change is being merged
into the current branch; tying behavior to a specific future version makes the
guide inaccurate for the version these docs ship with. Also "Spring Frameworks"
is missing the possessive apostrophe. Consider phrasing this
version-agnostically and fixing the grammar.
##########
grails-cache/src/test/groovy/grails/plugin/cache/GrailsConcurrentLinkedMapCacheTests.groovy:
##########
@@ -18,107 +18,138 @@
*/
package grails.plugin.cache
+import java.util.concurrent.ConcurrentMap
+
import org.springframework.cache.support.SimpleValueWrapper
-import com.googlecode.concurrentlinkedhashmap.ConcurrentLinkedHashMap
-import org.junit.Test
+import spock.lang.Specification
/**
* @author Jakob Drangmeister
*/
-class GrailsConcurrentLinkedMapCacheTests {
-
- @Test
- void testCreateCache() {
- GrailsConcurrentLinkedMapCache smallCache = new
GrailsConcurrentLinkedMapCache("smallCache", 1000)
+class GrailsConcurrentLinkedMapCacheTests extends Specification {
+
+ void 'creates caches with configured capacity and null value policy'() {
+ when:
+ GrailsConcurrentLinkedMapCache smallCache = new
GrailsConcurrentLinkedMapCache('smallCache', 1000)
+
+ then:
+ smallCache.name == 'smallCache'
+ smallCache.nativeCache instanceof ConcurrentMap
+ smallCache.capacity == 1000
+ smallCache.allowNullValues
+
+ when:
+ GrailsConcurrentLinkedMapCache bigCache = new
GrailsConcurrentLinkedMapCache('bigCache', 5000000, false)
+
+ then:
+ bigCache.name == 'bigCache'
+ bigCache.nativeCache instanceof ConcurrentMap
+ bigCache.capacity == 5000000
+ !bigCache.allowNullValues
+ }
- assert smallCache.getName() == "smallCache"
- assert smallCache.getNativeCache() instanceof ConcurrentLinkedHashMap
- assert smallCache.getCapacity() == 1000
- assert smallCache.isAllowNullValues() == true
+ void 'exposes Caffeine native cache as concurrent map'() {
+ given:
+ GrailsConcurrentLinkedMapCache cache = new
GrailsConcurrentLinkedMapCache('cache', 10, true)
- GrailsConcurrentLinkedMapCache bigCache = new
GrailsConcurrentLinkedMapCache("bigCache", 5000000, false)
+ when:
+ cache.put("key", "value")
- assert bigCache.getName() == "bigCache"
- assert bigCache.getNativeCache() instanceof ConcurrentLinkedHashMap
- assert bigCache.getCapacity() == 5000000
- assert bigCache.isAllowNullValues() == false
+ then:
+ cache.nativeCache.get('key') == 'value'
+
cache.nativeCache.getClass().name.startsWith('com.github.benmanes.caffeine.cache.')
}
- @Test
- void testPutAndGet() {
- GrailsConcurrentLinkedMapCache cache = new
GrailsConcurrentLinkedMapCache("cache", 1000, true)
+ void 'puts and gets cache entries'() {
+ given:
+ GrailsConcurrentLinkedMapCache cache = new
GrailsConcurrentLinkedMapCache('cache', 1000, true)
- cache.put("key", "value");
+ when:
+ cache.put('key', 'value')
- assert cache.getSize() == 1
+ then:
+ cache.size == 1
GrailsValueWrapper value = cache.get("key")
- assert value.get().equals("value")
+ value.get() == 'value'
}
- @Test
- void testPutIfAbsent() {
- GrailsConcurrentLinkedMapCache cache = new
GrailsConcurrentLinkedMapCache("cache", 1000, true)
- cache.put("key", "value")
- cache.putIfAbsent("key", "value") instanceof SimpleValueWrapper
- assert cache.getSize() == 1
+ void 'putIfAbsent keeps existing cache value'() {
+ given:
+ GrailsConcurrentLinkedMapCache cache = new
GrailsConcurrentLinkedMapCache('cache', 1000, true)
+ cache.put('key', 'value')
+
+ expect:
+ cache.putIfAbsent('key', 'value') instanceof SimpleValueWrapper
+ cache.size == 1
}
- @Test
- void testEvict() {
- GrailsConcurrentLinkedMapCache cache = new
GrailsConcurrentLinkedMapCache("cache", 10, true)
- cache.put("key", "value");
- assert cache.getSize() == 1
+ void 'evicts cache entries'() {
+ given:
+ GrailsConcurrentLinkedMapCache cache = new
GrailsConcurrentLinkedMapCache('cache', 10, true)
+ cache.put('key', 'value')
- cache.evict("key")
- assert cache.getSize() == 0
+ expect:
+ cache.size == 1
+ when:
+ cache.evict('key')
+
+ then:
+ cache.size == 0
}
- @Test
- void testCacheCapacity() {
- GrailsConcurrentLinkedMapCache cache = new
GrailsConcurrentLinkedMapCache("cache", 1000, true)
- assert cache.getCapacity() == 1000
+ void 'limits cache size to configured capacity'() {
+ given:
+ GrailsConcurrentLinkedMapCache cache = new
GrailsConcurrentLinkedMapCache('cache', 1000, true)
- for(int i = 0; i < 2000; i++) {
- cache.put(i, i)
+ when:
+ for (int i = 0; i < 2000; i++) {
+ cache.put(i, i)
}
- assert cache.getSize() == 1000
+ then:
+ cache.capacity == 1000
+ cache.size == 1000
}
- @Test
- void testCacheGetHottestKeys() {
- GrailsConcurrentLinkedMapCache cache = new
GrailsConcurrentLinkedMapCache("cache", 10, true)
+ void 'returns hottest keys from cache eviction policy'() {
+ given:
+ GrailsConcurrentLinkedMapCache cache = new
GrailsConcurrentLinkedMapCache('cache', 10, true)
- for(int i = 0; i < 10; i++) {
- cache.put(i, i);
+ for (int i = 0; i < 10; i++) {
+ cache.put(i, i)
}
+ when:
cache.get(1)
cache.get(2)
Review Comment:
This test relies on Caffeine's eviction/admission heuristics to keep key `2`
resident after many new inserts, but the current access pattern (single
`get(2)`) can be borderline and may become flaky across Caffeine versions.
Making `2` meaningfully hotter (e.g., repeated reads) will stabilize the intent
of the test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
