Mailing-List: contact issues-help@ignite.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@ignite.apache.org
Date: Thu, 23 Mar 2017 13:02:42 +0000 (UTC)
From: "Anton Vinogradov (JIRA)" <jira@apache.org>
To: issues@ignite.apache.org
Message-ID: <JIRA.13019868.1478792002000.100869.1490274162086@Atlassian.JIRA>
In-Reply-To: <JIRA.13019868.1478792002000@Atlassian.JIRA>
References: <JIRA.13019868.1478792002000@Atlassian.JIRA> <JIRA.13019868.1478792002904@jira-lw-us.apache.org>
Subject: [jira] [Comment Edited] (IGNITE-4211) Update Spring dependency to
 latest stable version
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
archived-at: Thu, 23 Mar 2017 13:02:53 -0000


    [ 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 1:02 PM:
-------------------------------------------------------------------

[~daradurvs]

> 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 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 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?

> 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)