Hi Ivan, You are right, I have not checked for those scenarios. My intial objective was to write tests for keys listed in Messages.properties and test for atleast one scenario where that key would be used. I focussed more on annotations and less on deployment descriptor related stuff. Hence this check only for annotations. Ideally, the test should involve all scenarios. I have limited time, so it will take a while before I can cover all the scenarios.If you would like to fix it and add coverage for TimedObject and descriptor, please feel free to do that. Creating a JIRA issue would be the first step.
BTW, such feedback is exactly the way we will end up covering more scenarios. Keep em coming and thank you very much. On Wed, Jul 28, 2010 at 3:20 AM, Ivan <[email protected]> wrote: > Hi, Karan > I have a question for this change, the timeout method could be specified > by @Timeout or impmented TimedObject or configured in the deployment > descriptor, so why the annotation checking is required ? > Thanks ! > > 2010/7/21 <[email protected]> > > > Author: kmalhi > > Date: Wed Jul 21 02:51:58 2010 > > New Revision: 966073 > > > > URL: http://svn.apache.org/viewvc?rev=966073&view=rev > > Log: > > Updated CheckCallbacks to test for scenario where a class has two or more > > overloaded methods, but the method with the wrong signature is annotated > > with @Timeout > > Commented out keys from Messages.properties and also updated a detailed > > message for a key > > Added @Timeout tests > > > > Modified: > > > > > > openejb/trunk/openejb3/container/openejb-core/src/main/java/org/apache/openejb/config/rules/CheckCallbacks.java > > > > > > openejb/trunk/openejb3/container/openejb-core/src/main/resources/org/apache/openejb/config/rules/Messages.properties > > > > > > openejb/trunk/openejb3/container/openejb-core/src/test/java/org/apache/openejb/config/rules/CheckInvalidTimeoutTest.java > > > > Modified: > > > openejb/trunk/openejb3/container/openejb-core/src/main/java/org/apache/openejb/config/rules/CheckCallbacks.java > > URL: > > > http://svn.apache.org/viewvc/openejb/trunk/openejb3/container/openejb-core/src/main/java/org/apache/openejb/config/rules/CheckCallbacks.java?rev=966073&r1=966072&r2=966073&view=diff > > > > > ============================================================================== > > --- > > > openejb/trunk/openejb3/container/openejb-core/src/main/java/org/apache/openejb/config/rules/CheckCallbacks.java > > (original) > > +++ > > > openejb/trunk/openejb3/container/openejb-core/src/main/java/org/apache/openejb/config/rules/CheckCallbacks.java > > Wed Jul 21 02:51:58 2010 > > @@ -16,6 +16,7 @@ > > */ > > package org.apache.openejb.config.rules; > > > > +import java.lang.annotation.Annotation; > > import java.lang.reflect.Method; > > import java.lang.reflect.Modifier; > > import java.util.ArrayList; > > @@ -29,6 +30,7 @@ import javax.ejb.PostActivate; > > import javax.ejb.PrePassivate; > > import javax.ejb.Remove; > > import javax.ejb.SessionSynchronization; > > +import javax.ejb.Timeout; > > import javax.interceptor.InvocationContext; > > > > import org.apache.openejb.OpenEJBException; > > @@ -336,12 +338,24 @@ public class CheckCallbacks extends Vali > > if (timeout == null) return; > > try { > > Method method = getMethod(ejbClass, timeout.getMethodName(), > > javax.ejb.Timer.class); > > - > > - Class<?> returnType = method.getReturnType(); > > - > > - if (!returnType.equals(Void.TYPE)) { > > - fail(bean, "timeout.badReturnType", > > timeout.getMethodName(), returnType.getName()); > > + Annotation[] declaredAnnotations = > > method.getDeclaredAnnotations(); > > + boolean isAnnotated = false; > > + for (Annotation annotation : declaredAnnotations) { > > + if(annotation.equals(Timeout.class)){ > > + isAnnotated = true; > > + break; > > + } > > + } > > + > > + if (isAnnotated) { > > + Class<?> returnType = method.getReturnType(); > > + if (!returnType.equals(Void.TYPE)) { > > + fail(bean, "timeout.badReturnType", > > timeout.getMethodName(), returnType.getName()); > > + } > > + }else{ > > + fail(bean, "timeout.missing.possibleTypo", > > timeout.getMethodName(), getMethods(ejbClass, > > timeout.getMethodName()).size()); > > } > > + > > } catch (NoSuchMethodException e) { > > List<Method> possibleMethods = getMethods(ejbClass, > > timeout.getMethodName()); > > > > > > Modified: > > > openejb/trunk/openejb3/container/openejb-core/src/main/resources/org/apache/openejb/config/rules/Messages.properties > > URL: > > > http://svn.apache.org/viewvc/openejb/trunk/openejb3/container/openejb-core/src/main/resources/org/apache/openejb/config/rules/Messages.properties?rev=966073&r1=966072&r2=966073&view=diff > > > > > ============================================================================== > > --- > > > openejb/trunk/openejb3/container/openejb-core/src/main/resources/org/apache/openejb/config/rules/Messages.properties > > (original) > > +++ > > > openejb/trunk/openejb3/container/openejb-core/src/main/resources/org/apache/openejb/config/rules/Messages.properties > > Wed Jul 21 02:51:58 2010 > > @@ -223,10 +223,11 @@ > > 2.timeout.badReturnType = Timeout method must return 'void': method {0} > > returns {1} > > 3.timeout.badReturnType = Timeout method "{0}" illegally returns {1} > > instead of void. Change the method signature to "void > {0}(javax.ejb.Timer)" > > > > +#Don't think this is ever going to be used, commenting it out for now. > If > > there is a case where this key can be used, please uncomment it and write > a > > test for it > > # fail(bean, "timeout.missing", timeout.getMethodName()); > > -1.timeout.missing = Timeout method missing > > -2.timeout.missing = Timeout method missing: "{0}" in class {1} > > -3.timeout.missing = Timeout method "{0}" not found in class {1}. The > > required method signature is "void {0}(javax.ejb.Timer)" > > +#1.timeout.missing = Timeout method missing > > +#2.timeout.missing = Timeout method missing: "{0}" in class {1} > > +#3.timeout.missing = Timeout method "{0}" not found in class {1}. The > > required method signature is "void {0}(javax.ejb.Timer)" > > > > # fail(bean, "timeout.invalidArguments", timeout.getMethodName(), > > getParameters(possibleMethods.get(0))); > > 1.timeout.invalidArguments = Invalid Timeout arguments > > @@ -236,7 +237,7 @@ > > # fail(bean, "timeout.missing.possibleTypo", timeout.getMethodName(), > > possibleMethods.size()); > > 1.timeout.missing.possibleTypo = Timeout method missing or invalid > > 2.timeout.missing.possibleTypo = Timeout method missing or invalid: > looked > > for "void {0}(javax.ejb.Timer)" > > -3.timeout.missing.possibleTypo = Timeout method missing or invalid. > There > > are {1} methods with the name "{0}" visible, none have the required > > signature of "void {0}(javax.ejb.Timer)" > > +3.timeout.missing.possibleTypo = Timeout method missing or invalid. > There > > are {1} methods with the name "{0}" visible, either the wrong one has > been > > annotated with @Timeout or none have the required signature of "void > > {0}(javax.ejb.Timer). A bean should have only one method annotated with > > @Timeout and the method signature must match void {0}(javax.ejb.Timer)" > > > > # > > > fail(componentName,"timeout.tooManyMethods",timeoutMethods.size(),Join.join(",", > > timeoutMethods)); > > 1.timeout.tooManyMethods = More than one method annotated with @Timeout > > > > Modified: > > > openejb/trunk/openejb3/container/openejb-core/src/test/java/org/apache/openejb/config/rules/CheckInvalidTimeoutTest.java > > URL: > > > http://svn.apache.org/viewvc/openejb/trunk/openejb3/container/openejb-core/src/test/java/org/apache/openejb/config/rules/CheckInvalidTimeoutTest.java?rev=966073&r1=966072&r2=966073&view=diff > > > > > ============================================================================== > > --- > > > openejb/trunk/openejb3/container/openejb-core/src/test/java/org/apache/openejb/config/rules/CheckInvalidTimeoutTest.java > > (original) > > +++ > > > openejb/trunk/openejb3/container/openejb-core/src/test/java/org/apache/openejb/config/rules/CheckInvalidTimeoutTest.java > > Wed Jul 21 02:51:58 2010 > > @@ -26,26 +26,40 @@ import org.junit.runner.RunWith; > > > > @RunWith(ValidationRunner.class) > > public class CheckInvalidTimeoutTest extends TestCase { > > - @Keys( { @Key(value = "timeout.badReturnType"), > > @Key("timeout.invalidArguments"), @Key("timeout.tooManyMethods") }) > > + @Keys( { @Key(value = "timeout.badReturnType"), > > @Key("timeout.invalidArguments"), @Key("timeout.tooManyMethods") , > > @Key("timeout.missing.possibleTypo")}) > > public EjbJar test() throws Exception { > > System.setProperty("openejb.validation.output.level", "VERBOSE"); > > EjbJar ejbJar = new EjbJar(); > > ejbJar.addEnterpriseBean(new StatelessBean(TestBean.class)); > > ejbJar.addEnterpriseBean(new StatelessBean(FooBean.class)); > > + ejbJar.addEnterpriseBean(new StatelessBean(BarBean.class)); > > return ejbJar; > > } > > - > > +// A case where the class has the method with wrong signature annotated > > with @Timeout > > + // timeout.badReturnType > > + // timeout.invalidArguments > > public static class TestBean { > > @Timeout > > public Object bar() { > > return null; > > } > > } > > - > > +// A case where the class has more than one method annotated with > @Timeout > > + // timeout.tooManyMethods > > public static class FooBean { > > @Timeout > > public void foo(javax.ejb.Timer timer) {} > > @Timeout > > public void bar(javax.ejb.Timer timer) {} > > } > > -} > > \ No newline at end of file > > +// A case where the class has overloaded methods, but the method with > the > > wrong signature has been annotated with @Timeout > > + // timeout.missing.possibleTypo > > + public static class BarBean { > > + > > + public void foo(javax.ejb.Timer timer) {} > > + > > + @Timeout > > + public void foo() {} > > + } > > + > > +} > > > > > > > > > -- > Ivan > -- Karan Singh Malhi
