Don't allow inheritance in glue code (stepdefs and hooks). Also fixes cucumber#257 in a more encapsulated way.

Author: aslakhellesoy

History.md

@@ -1,5 +1,7 @@
 ## [Git master](https://github.com/cucumber/cucumber-jvm/compare/v1.0.0.RC20...master)
 
+* [Java] Open up
+* [Java] Inheritance in glue classes (stepdefs and hooks) is no longer supported - it causes too many problems. (Aslak Hellesøy).
 * [JUnit] `@Cucumber.Options` annotation replaces `@Feature` annotation ([#160](https://github.com/cucumber/cucumber-jvm/issues/160) Aslak Hellesøy)
 * [Spring] Slow Spring context performance ([#241](https://github.com/cucumber/cucumber-jvm/issues/241), [#242](https://github.com/cucumber/cucumber-jvm/pull/242) Vladimir Klyushnikov)
 * [Core] Support for java.util.Calendar arguments in stepdefs. (Aslak Hellesøy)

java/src/main/java/cucumber/runtime/java/ClasspathMethodScanner.java

@@ -11,16 +11,23 @@
 import java.util.Collection;
 import java.util.List;
 
-public class ClasspathMethodScanner {
+class ClasspathMethodScanner {
 
     private final ClasspathResourceLoader resourceLoader;
+    private final Collection<Class<? extends Annotation>> cucumberAnnotationClasses;
 
     public ClasspathMethodScanner(ClasspathResourceLoader resourceLoader) {
         this.resourceLoader = resourceLoader;
+        cucumberAnnotationClasses = findCucumberAnnotationClasses();
     }
 
+    /**
+     * Registers step definitions and hooks.
+     *
+     * @param javaBackend the backend where stepdefs and hooks will be registered
+     * @param gluePaths   where to look
+     */
     public void scan(JavaBackend javaBackend, List<String> gluePaths) {
-        Collection<Class<? extends Annotation>> cucumberAnnotationClasses = findCucumberAnnotationClasses();
         for (String gluePath : gluePaths) {
             String packageName = gluePath.replace('/', '.').replace('\\', '.'); // Sometimes the gluePath will be a path, not a package
             for (Class<?> glueCodeClass : resourceLoader.getDescendants(Object.class, packageName)) {
@@ -30,30 +37,36 @@ public void scan(JavaBackend javaBackend, List<String> gluePaths) {
                 }
                 if (glueCodeClass != null) {
                     for (Method method : glueCodeClass.getMethods()) {
-                        scan(glueCodeClass, method, cucumberAnnotationClasses, javaBackend);
+                        scan(javaBackend, method);
                     }
                 }
             }
         }
     }
 
-    private Collection<Class<? extends Annotation>> findCucumberAnnotationClasses() {
-        return resourceLoader.getAnnotations("cucumber.annotation");
-    }
-
-    private void scan(Class<?> glueCodeClass, Method method, Collection<Class<? extends Annotation>> cucumberAnnotationClasses, JavaBackend javaBackend) {
+    /**
+     * Registers step definitions and hooks.
+     *
+     * @param javaBackend the backend where stepdefs and hooks will be registered
+     * @param method      a candidate for being a stepdef or hook
+     */
+    public void scan(JavaBackend javaBackend, Method method) {
         for (Class<? extends Annotation> cucumberAnnotationClass : cucumberAnnotationClasses) {
             Annotation annotation = method.getAnnotation(cucumberAnnotationClass);
             if (annotation != null && !annotation.annotationType().equals(Order.class)) {
                 if (isHookAnnotation(annotation)) {
-                    javaBackend.addHook(annotation, glueCodeClass, method);
+                    javaBackend.addHook(annotation, method);
                 } else if (isStepdefAnnotation(annotation)) {
-                    javaBackend.addStepDefinition(annotation, glueCodeClass, method);
+                    javaBackend.addStepDefinition(annotation, method);
                 }
             }
         }
     }
 
+    private Collection<Class<? extends Annotation>> findCucumberAnnotationClasses() {
+        return resourceLoader.getAnnotations("cucumber.annotation");
+    }
+
     private boolean isHookAnnotation(Annotation annotation) {
         Class<? extends Annotation> annotationClass = annotation.annotationType();
         return annotationClass.equals(Before.class) || annotationClass.equals(After.class);

java/src/main/java/cucumber/runtime/java/JavaBackend.java

@@ -58,6 +58,18 @@ public void loadGlue(Glue glue, List<String> gluePaths) {
         classpathMethodScanner.scan(this, gluePaths);
     }
 
+    /**
+     * Convenience method for frameworks that wish to load glue from methods explicitly (possibly
+     * found with a different mechanism than Cucumber's built-in classpath scanning).
+     *
+     * @param glue   where stepdefs and hooks will be added.
+     * @param method a candidate method.
+     */
+    public void loadGlue(Glue glue, Method method) {
+        this.glue = glue;
+        classpathMethodScanner.scan(this, method);
+    }
+
     @Override
     public void setUnreportedStepExecutor(UnreportedStepExecutor executor) {
         //Not used here yet
@@ -78,13 +90,13 @@ public String getSnippet(Step step) {
         return snippetGenerator.getSnippet(step);
     }
 
-    void addStepDefinition(Annotation annotation, Class<?> glueCodeClass, Method method) {
+    void addStepDefinition(Annotation annotation, Method method) {
         try {
             Method regexpMethod = annotation.getClass().getMethod("value");
             String regexpString = (String) regexpMethod.invoke(annotation);
             if (regexpString != null) {
                 Pattern pattern = Pattern.compile(regexpString);
-                objectFactory.addClass(glueCodeClass);
+                objectFactory.addClass(method.getDeclaringClass());
                 glue.addStepDefinition(new JavaStepDefinition(method, pattern, objectFactory));
             }
         } catch (NoSuchMethodException e) {
@@ -96,8 +108,8 @@ void addStepDefinition(Annotation annotation, Class<?> glueCodeClass, Method met
         }
     }
 
-    void addHook(Annotation annotation, Class<?> glueCodeClass, Method method) {
-        objectFactory.addClass(glueCodeClass);
+    void addHook(Annotation annotation, Method method) {
+        objectFactory.addClass(method.getDeclaringClass());
 
         Order order = method.getAnnotation(Order.class);
         int hookOrder = (order == null) ? Integer.MAX_VALUE : order.value();

java/src/test/java/cucumber/runtime/java/ClasspathMethodScannerTest.java

@@ -3,20 +3,18 @@
 import cucumber.annotation.Before;
 import cucumber.io.ClasspathResourceLoader;
 import cucumber.runtime.Glue;
-import cucumber.runtime.java.test2.Stepdefs2;
 import org.junit.Test;
 import org.mockito.Mockito;
 import org.mockito.internal.util.reflection.Whitebox;
 
-import static java.util.Arrays.asList;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyNoMoreInteractions;
 
 public class ClasspathMethodScannerTest {
 
     @Test
-    public void loadGlue_should_not_try_to_instantiate_super_classes() {
+    public void loadGlue_registers_the_methods_declaring_class_in_the_object_factory() throws NoSuchMethodException {
 
         ClasspathResourceLoader resourceLoader = new ClasspathResourceLoader(Thread.currentThread().getContextClassLoader());
         ClasspathMethodScanner classpathMethodScanner = new ClasspathMethodScanner(resourceLoader);
@@ -27,14 +25,18 @@ public void loadGlue_should_not_try_to_instantiate_super_classes() {
         Whitebox.setInternalState(backend, "glue", world);
 
         // this delegates to classpathMethodScanner.scan which we test
-        classpathMethodScanner.scan(backend, asList("cucumber/runtime/java/test2"));
+        classpathMethodScanner.scan(backend, BaseStepDefs.class.getMethod("m"));
 
-        verify(factory, times(1)).addClass(Stepdefs2.class);
+        verify(factory, times(1)).addClass(BaseStepDefs.class);
         verifyNoMoreInteractions(factory);
     }
 
-    public static class BaseStepDefs {
+    public static class Stepdefs2 extends BaseStepDefs {
+        public interface Interface1 {
+        }
+    }
 
+    public static class BaseStepDefs {
         @Before
         public void m() {
         }

java/src/test/java/cucumber/runtime/java/JavaHookTest.java

@@ -45,7 +45,7 @@ public void loadNoGlue() {
     @Test
     public void before_hooks_get_registered() throws Exception {
         backend.buildWorld();
-        backend.addHook(BEFORE.getAnnotation(Before.class), HasHooks.class, BEFORE);
+        backend.addHook(BEFORE.getAnnotation(Before.class), BEFORE);
         JavaHookDefinition hookDef = (JavaHookDefinition) glue.getBeforeHooks().get(0);
         assertEquals(0, glue.getAfterHooks().size());
         assertEquals(BEFORE, hookDef.getMethod());
@@ -54,7 +54,7 @@ public void before_hooks_get_registered() throws Exception {
     @Test
     public void after_hooks_get_registered() throws Exception {
         backend.buildWorld();
-        backend.addHook(AFTER.getAnnotation(After.class), HasHooks.class, AFTER);
+        backend.addHook(AFTER.getAnnotation(After.class), AFTER);
         JavaHookDefinition hookDef = (JavaHookDefinition) glue.getAfterHooks().get(0);
         assertEquals(0, glue.getBeforeHooks().size());
         assertEquals(AFTER, hookDef.getMethod());
@@ -63,31 +63,31 @@ public void after_hooks_get_registered() throws Exception {
     @Test
     public void hook_order_gets_registered() {
         backend.buildWorld();
-        backend.addHook(AFTER.getAnnotation(After.class), HasHooks.class, AFTER);
+        backend.addHook(AFTER.getAnnotation(After.class), AFTER);
         HookDefinition hookDef = glue.getAfterHooks().get(0);
         assertEquals(1, hookDef.getOrder());
     }
 
     @Test
     public void hook_with_no_order_is_last() {
         backend.buildWorld();
-        backend.addHook(BEFORE.getAnnotation(Before.class), HasHooks.class, BEFORE);
+        backend.addHook(BEFORE.getAnnotation(Before.class), BEFORE);
         HookDefinition hookDef = glue.getBeforeHooks().get(0);
         assertEquals(Integer.MAX_VALUE, hookDef.getOrder());
     }
 
     @Test
     public void matches_matching_tags() {
         backend.buildWorld();
-        backend.addHook(BEFORE.getAnnotation(Before.class), HasHooks.class, BEFORE);
+        backend.addHook(BEFORE.getAnnotation(Before.class), BEFORE);
         HookDefinition before = glue.getBeforeHooks().get(0);
         assertTrue(before.matches(asList(new Tag("@bar", 0), new Tag("@zap", 0))));
     }
 
     @Test
     public void does_not_match_non_matching_tags() {
         backend.buildWorld();
-        backend.addHook(BEFORE.getAnnotation(Before.class), HasHooks.class, BEFORE);
+        backend.addHook(BEFORE.getAnnotation(Before.class), BEFORE);
         HookDefinition before = glue.getBeforeHooks().get(0);
         assertFalse(before.matches(asList(new Tag("@bar", 0))));
     }

java/src/test/java/cucumber/runtime/java/JavaStepDefinitionTest.java

@@ -56,8 +56,8 @@ public void loadNoGlue() {
 
     @Test
     public void throws_ambiguous_when_two_matches_are_found() throws Throwable {
-        backend.addStepDefinition(THREE_DISABLED_MICE.getAnnotation(Given.class), Defs.class, THREE_DISABLED_MICE);
-        backend.addStepDefinition(THREE_BLIND_ANIMALS.getAnnotation(Given.class), Defs.class, THREE_BLIND_ANIMALS);
+        backend.addStepDefinition(THREE_DISABLED_MICE.getAnnotation(Given.class), THREE_DISABLED_MICE);
+        backend.addStepDefinition(THREE_BLIND_ANIMALS.getAnnotation(Given.class), THREE_BLIND_ANIMALS);
 
         Reporter reporter = mock(Reporter.class);
         runtime.buildBackendWorlds(reporter);
@@ -72,7 +72,7 @@ public void throws_ambiguous_when_two_matches_are_found() throws Throwable {
 
     @Test
     public void does_not_throw_ambiguous_when_nothing_is_ambiguous() throws Throwable {
-        backend.addStepDefinition(THREE_DISABLED_MICE.getAnnotation(Given.class), Defs.class, THREE_DISABLED_MICE);
+        backend.addStepDefinition(THREE_DISABLED_MICE.getAnnotation(Given.class), THREE_DISABLED_MICE);
 
         Reporter reporter = mock(Reporter.class);
         runtime.buildBackendWorlds(reporter);