cruftex
commented
8 years ago Current state of the readme I am going to address:
PutTest.putAll_NullKey()
// It disallows partial success, even though this is not explicitly required, instead the Spec reads:
//
// " * The effect of this call is equivalent to that of calling
// * {@link #put(Object, Object) put(k, v)} on this cache once for each mapping
// * from key <tt>k</tt> to value <tt>v</tt> in the specified map.
// *
// * The order in which the individual puts occur is undefined."
//
// There is no mention of a "rollback" for partial failures. In contrast CacheWriter has explicit documentation ("remove succesful writes from the Map").
// The API / Spec should be changed to reflect the TCK test or vice versa.CacheManagerTest createCacheSameName() and createCacheSame()
// Observation: Mandates to throw CacheException when "the same" cache is to be created twice.
// Issue: Javadocs has no "@throws CacheException". It is only in the text. CacheManagerTest getNullTypeCacheRequest()
// Observation: Mandates to throw NullPointerException when passing null as keyClass or valueClass
// Issue: Javadocs and Spec do not mention behavior on null.org.jsr107.tck.event.CacheListenerTest.testFilteredListener(CacheListenerTest.java:396)
// Observation: Unclear spec for Cache.remove(key): We need to use oldValue as value in the event. The Spec is not clear about this, but the TCK bombs
// us with NPE when we would use null as "value" and oldValue as "old value". The JSR107 RI uses oldValue as "value" and leaves the "old value" unset.
// org.jsr107.tck.event.CacheListenerTest$MyCacheEntryEventFilter.evaluate(CacheListenerTest.java:344) // <<< Location of NPECacheMBStatisticsBeanTest
// Three wrong assertEquals() checks, where the "expected" and "actual" parameters are exchanged.
// Has no influence on test result, but wrong test output.
assertEquals(result, "Sooty");
assertEquals(result, "Trinity");org.jsr107.tck.processor.CacheInvokeTest.noValueException()
// Observation: This test checks for IllegalAccessError, but the ThrowExceptionEntryProcessor class wraps it and throws "new EntryProcessorException(t);"
// An implementation should wrap EntryProcessor Exceptions also in EntryProcessorException, which means the IllegalAccessError gets double wrapped.
// Issue: The noValueException() test treats double wrapping as wrong, but IMO the Spec says to wrap ALL EntryProcessor Exceptions.
// Proposed solution: Change the TCK, or change the Spec to explicitly say that "exc instanceof EntryProcessorException" should not be wrapped again.
// See: https://github.com/jsr107/jsr107tck/issues/85org.jsr107.tck.processor.CacheInvokeTest
// Observation: nullProcessor() and invokeAll_nullProcessor() require a NullPointerException.
// Issue: It is not mentioned in the Spec. An alternative would be to ignore a null processor.
// Proposed solution: Add "@throws NullPointerException if entryProcessor is null" to Javadocsorg.jsr107.tck.integration.CacheLoaderTest
// Observation: shouldPropagateExceptionUsingLoadAll() requires a CacheLoaderException
// Issue: Javadocs do not mandate this for Cache.loadAll().
// a) Javadoc states: "If a problem is
// encountered during the retrieving or loading of the objects,
// an exception is provided to the {@link CompletionListener}."
// b) The @throws declaration is "@throws CacheException thrown if there is a problem performing the load."
// Proposed change: Change CacheLoaderTest to check for CacheException
// See: https://github.com/jsr107/jsr107tck/issues/99 org.jsr107.tck.integration.CacheWriterTest
// Observation: SImilar to CacheLoaderTest
// CacheWriterExceptionorg.jsr107.tck.integration.CacheLoaderWithoutReadThroughTest
- Observation: shouldLoadWhenAccessingWithEntryProcessor() requires that no load takes place for cache.invoke() in case of no-read-through
-
Issue: While the Spec has a nice table about read-through and is clear about it, the JavaDocs do not mention it:
// "If an {@link Entry} does not exist for the specified key, an attempt is made to load it (if a loader is configured)" (no mention of read-through)
- Proposed change: invoke() has fairly complex behaviour, and the documentation should be as clear as possible. // Old: "an attempt is made to load it (if a loader is configured)" // New: "an attempt is made to load it (if the cache is read-through)"
Unclear spec which kind of Listener should fire on Evictions
// Observation: Spec is unlcear. It talks about "evictions" when doing "expiration", but not about "true" evicitions.
// ehcache sends EVICTED, it seems. I will go for it, but it should be clarified.
// Proposed change: ClarificationSpec: CacheWriter table
// V getAndReplace(K key, V value)
// Observation: Wrong description "Yes, if this method returns true"
// Proposed change: "Yes, if this method returns a non-null value"org.jst107.tck.??? <<< Determine the test(s) that fail
// Observation: javax.cache.Cache.Entry and MutableEntry need to be Serializable. Otherwise TCK fails (TODO: Add the test names)
// Proposed action: Clarify. Possibly add "Serializable" requirement to Specorg.jsr107.tck.RemoveTest
- Observation: shouldWriteThroughRemove_SpecificEntry() mandates that remove(key,value) only(!) writes-through, if we happen to have the (key,value) combination in the local cache.
- Issue: This can lead to non-deterministic behavior if the local cache has evicted that Cache entry. It is also inconsistent with all other methods: Usually the write-through is always done, and the local Cache get mutated for the successfully written-through entries. But here the local Cache is inspected first.
- Proposed change: Unclear. The reason could be an omission in the CacheWriter Interface: It does not have a write(key,value) method. To be discussed.
christian-esken
@christian-esken has some findings for the Spec which look quite relevant: https://github.com/trivago/triava/blob/master/tck/README.md
This is a task for me to look it through and open specific issues for it and give back feedback.