[
https://issues.apache.org/jira/browse/IGNITE-4211?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15938171#comment-15938171
]
Anton Vinogradov edited comment on IGNITE-4211 at 3/23/17 12:11 PM:
--------------------------------------------------------------------
> If you don't agree with my approach, and if the test of
> @Cachenable(sync=true) is enough for you:
> Just write it, I will change the test and then it's done.
> Such simple task was delayed for a long time.
We have a the highest standards across the industry and everything should be
fixed/implemented in proper way.
Also each contribution shoud follow
https://cwiki.apache.org/confluence/display/IGNITE/Abbreviation+Rules
and
https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines
> it is very strange test
Next rule is: each contributor should figure it out how current solution work
before proposing replacement.
> I think we have to cover the all new code with unit tests
Correct, but we should not have 2 or more test approach it case one is ehough.
> And I think we have to tests the following cases:
> valueLoader parameter was called only once per same key; [1]
> if a cache contains nothing for a key, a result of valueLoader parameter will
> added to the cache correctly; [2]
> if valueLoader parameter throws some exceptions, the new get-method rethrows
> the excepted type of exception; [3]
Sure, we have to.
All these cases can_be/already covered by GridSpringCacheManagerSelfTest
1) this checks by a double call
{noformat}
assertEquals("value" + i, dynamicSvc.cacheable(i));
assertEquals("value" + i, dynamicSvc.cacheable(i));
{noformat}
and follow-up counter check
{noformat}
assertEquals(1, dynamicSvc.called());
{noformat}
2) this checks by a call
{noformat}
assertEquals("value" + i, dynamicSvc.cacheable(i));
{noformat}
and follow-up cache check
{noformat}
assertEquals(1, c.size());
assertEquals("value" + 1, c.get(1));
{noformat}
3) value loader is an aop'ed @Cacheable method and it can throw only runtime
exception.
And it's will be covered in proper way.
{noformat}
java.lang.RuntimeException
at
org.apache.ignite.cache.spring.GridSpringDynamicCacheTestService.cacheable(GridSpringDynamicCacheTestService.java:43)
at
org.apache.ignite.cache.spring.GridSpringDynamicCacheTestService$$FastClassBySpringCGLIB$$63a1306.invoke(<generated>)
at
org.springframework.cglib.proxy.MethodProxy.invoke(MethodProxy.java:204)
at
org.springframework.aop.framework.CglibAopProxy$CglibMethodInvocation.invokeJoinpoint(CglibAopProxy.java:717)
at
org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:157)
at
org.springframework.cache.interceptor.CacheInterceptor$1.invoke(CacheInterceptor.java:52)
at
org.springframework.cache.interceptor.CacheAspectSupport.invokeOperation(CacheAspectSupport.java:299)
at
org.springframework.cache.interceptor.CacheAspectSupport.execute(CacheAspectSupport.java:332)
at
org.springframework.cache.interceptor.CacheAspectSupport.execute(CacheAspectSupport.java:281)
at
org.springframework.cache.interceptor.CacheInterceptor.invoke(CacheInterceptor.java:61)
at
org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:179)
at
org.springframework.aop.framework.CglibAopProxy$DynamicAdvisedInterceptor.intercept(CglibAopProxy.java:653)
at
org.apache.ignite.cache.spring.GridSpringDynamicCacheTestService$$EnhancerBySpringCGLIB$$7bd6b246.cacheable(<generated>)
at
org.apache.ignite.cache.spring.GridSpringCacheManagerSelfTest.testDynamicCache(GridSpringCacheManagerSelfTest.java:350)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at junit.framework.TestCase.runTest(TestCase.java:176)
{noformat}
> Just make that the any get-methods returned "null".
> What happens? - Nothing. Green light in the GridSpringCacheManagerSelfTest.
Please write an example showing this.
> I found the bug IGNITE-4823, when SpringCache was covered with unit-tests
Thanks for finding this!
Unfortunatelly @CachePut annotation not cover this, so we have to check this
method directly.
BTW, any ideas why it's not covered by @CachePut?
was (Author: avinogradov):
> If you don't agree with my approach, and if the test of
> @Cachenable(sync=true) is enough for you:
> Just write it, I will change the test and then it's done.
> Such simple task was delayed for a long time.
We have a the highest standards across the industry and everything should be
fixed/implemented in proper way.
Also each contribution shoud follow
https://cwiki.apache.org/confluence/display/IGNITE/Abbreviation+Rules
and
https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines
> it is very strange test
Next rule is: each contributor should figure it out how current solution work
before proposing replacement.
> I think we have to cover the all new code with unit tests
Correct, but we should not have 2 or more test approach it case one is ehough.
> And I think we have to tests the following cases:
> valueLoader parameter was called only once per same key; [1]
> if a cache contains nothing for a key, a result of valueLoader parameter will
> added to the cache correctly; [2]
> if valueLoader parameter throws some exceptions, the new get-method rethrows
> the excepted type of exception; [3]
Sure, we have to.
All these cases can_be/already covered by GridSpringCacheManagerSelfTest
1) this checks by a double call
{noformat}
assertEquals("value" + i, dynamicSvc.cacheable(i));
assertEquals("value" + i, dynamicSvc.cacheable(i));
{noformat}
and follow-up counter check
{noformat}
assertEquals(1, dynamicSvc.called());
{noformat}
2) this checks by a call
{noformat}
assertEquals("value" + i, dynamicSvc.cacheable(i));
{noformat}
and follow-up counter check
{noformat}
assertEquals(1, c.size());
assertEquals("value" + 1, c.get(1));
{noformat}
3) value loader is an aop'ed @Cacheable method and it can throw only runtime
exception.
And it's will be covered in proper way.
{noformat}
java.lang.RuntimeException
at
org.apache.ignite.cache.spring.GridSpringDynamicCacheTestService.cacheable(GridSpringDynamicCacheTestService.java:43)
at
org.apache.ignite.cache.spring.GridSpringDynamicCacheTestService$$FastClassBySpringCGLIB$$63a1306.invoke(<generated>)
at
org.springframework.cglib.proxy.MethodProxy.invoke(MethodProxy.java:204)
at
org.springframework.aop.framework.CglibAopProxy$CglibMethodInvocation.invokeJoinpoint(CglibAopProxy.java:717)
at
org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:157)
at
org.springframework.cache.interceptor.CacheInterceptor$1.invoke(CacheInterceptor.java:52)
at
org.springframework.cache.interceptor.CacheAspectSupport.invokeOperation(CacheAspectSupport.java:299)
at
org.springframework.cache.interceptor.CacheAspectSupport.execute(CacheAspectSupport.java:332)
at
org.springframework.cache.interceptor.CacheAspectSupport.execute(CacheAspectSupport.java:281)
at
org.springframework.cache.interceptor.CacheInterceptor.invoke(CacheInterceptor.java:61)
at
org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:179)
at
org.springframework.aop.framework.CglibAopProxy$DynamicAdvisedInterceptor.intercept(CglibAopProxy.java:653)
at
org.apache.ignite.cache.spring.GridSpringDynamicCacheTestService$$EnhancerBySpringCGLIB$$7bd6b246.cacheable(<generated>)
at
org.apache.ignite.cache.spring.GridSpringCacheManagerSelfTest.testDynamicCache(GridSpringCacheManagerSelfTest.java:350)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at junit.framework.TestCase.runTest(TestCase.java:176)
{noformat}
> Just make that the any get-methods returned "null".
> What happens? - Nothing. Green light in the GridSpringCacheManagerSelfTest.
Please write an example showing this.
> I found the bug IGNITE-4823, when SpringCache was covered with unit-tests
Thanks for finding this!
Unfortunatelly @CachePut annotation not cover this, so we have to check this
method directly.
BTW, any ideas why it's not covered by @CachePut?
> Update Spring dependency to latest stable version
> -------------------------------------------------
>
> Key: IGNITE-4211
> URL: https://issues.apache.org/jira/browse/IGNITE-4211
> Project: Ignite
> Issue Type: Improvement
> Components: build
> Affects Versions: 1.7
> Reporter: Sergey Kozlov
> Assignee: Vyacheslav Daradur
> Fix For: 2.0
>
>
> It seems the Spring dependency looks outdated for now. Apache Ignite still
> uses 4.1.0 released two years ago. Could we to update to latest stable
> version (4.3.4 at the moment)?
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
