Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

For issue #186. Cucumber tries to instantiate parent classes via Spring #188

Closed
wants to merge 5 commits into from

2 participants

@vasiliygagin

As requested I've created failing test:
java/src/test/java/cucumber/runtime/java/ClasspathMethodScannerTest.java
java/src/test/java/cucumber/runtime/java/test2/Stepdefs2.java

To fix the bug, I've passed additions parameter "Class<?> candidateClass" to JavaBackend.addHook and JavaBackend.addStepDefinition methods. Fixed affected test cases.

While running build I've noticed that cucumber-spring test cases broke. That was because some inner class with hooks was discovered. Not sure if those should be ignored or instantiated by creating parent class first. To be safe I've just "replaced" such classes with their super classes. That is what happening in current implementation.
See following ClasspathMethodScanner chunk of code for details:

            while (candidateClass.getEnclosingClass() != null && !Modifier.isStatic(candidateClass.getModifiers())
                    && candidateClass != Object.class) {
                // those can't be instantiated without container class present.
                candidateClass = candidateClass.getSuperclass();
            }
@aslakhellesoy aslakhellesoy commented on the diff
...cucumber/runtime/java/ClasspathMethodScannerTest.java
@@ -0,0 +1,61 @@
+/*
@aslakhellesoy Owner

When you remove this I'll merge this in. I don't want my name associated with removal of someone else's copyright. In fact, it would be better if the copyright wasn't in any of the commits (you can force push or create a new pull request).

I don't mean to be pedantic here, but this sort of thing has been abused in the past and I want my back clear.

No need to explain. I'm on board with what you said. Thank you for catching this. Having copyright there is probably more problem for me then for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@vasiliygagin

Will reissue request without copyright comments. See pull request #193.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
15 java/src/main/java/cucumber/runtime/java/ClasspathMethodScanner.java
@@ -4,9 +4,11 @@
import cucumber.annotation.Before;
import cucumber.annotation.Order;
import cucumber.io.ClasspathResourceLoader;
+import cucumber.runtime.Utils;
import java.lang.annotation.Annotation;
import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
import java.util.Collection;
import java.util.List;
@@ -23,8 +25,12 @@ public void scan(JavaBackend javaBackend, List<String> gluePaths) {
for (String gluePath : gluePaths) {
String packageName = gluePath.replace('/', '.').replace('\\', '.'); // Sometimes the gluePath will be a path, not a package
for (Class<?> candidateClass : resourceLoader.getDescendants(Object.class, packageName)) {
+ while (candidateClass != Object.class && !Utils.isInstantiable(candidateClass)) {
+ // those can't be instantiated without container class present.
+ candidateClass = candidateClass.getSuperclass();
+ }
for (Method method : candidateClass.getMethods()) {
- scan(method, cucumberAnnotationClasses, javaBackend);
+ scan(candidateClass, method, cucumberAnnotationClasses, javaBackend);
}
}
}
@@ -34,14 +40,15 @@ public void scan(JavaBackend javaBackend, List<String> gluePaths) {
return resourceLoader.getAnnotations("cucumber.annotation");
}
- private void scan(Method method, Collection<Class<? extends Annotation>> cucumberAnnotationClasses, JavaBackend javaBackend) {
+ private void scan(Class<?> candidateClass, Method method, Collection<Class<? extends Annotation>> cucumberAnnotationClasses,
+ JavaBackend javaBackend) {
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, method);
+ javaBackend.addHook(annotation, candidateClass, method);
} else if (isStepdefAnnotation(annotation)) {
- javaBackend.addStepDefinition(annotation, method);
+ javaBackend.addStepDefinition(annotation, candidateClass, method);
}
}
}
View
10 java/src/main/java/cucumber/runtime/java/JavaBackend.java
@@ -82,14 +82,13 @@ public String getSnippet(Step step) {
return snippetGenerator.getSnippet(step);
}
- void addStepDefinition(Annotation annotation, Method method) {
+ void addStepDefinition(Annotation annotation, Class<?> candidateClass, Method method) {
try {
Method regexpMethod = annotation.getClass().getMethod("value");
String regexpString = (String) regexpMethod.invoke(annotation);
if (regexpString != null) {
Pattern pattern = Pattern.compile(regexpString);
- Class<?> clazz = method.getDeclaringClass();
- registerClassInObjectFactory(clazz);
+ registerClassInObjectFactory(candidateClass);
glue.addStepDefinition(new JavaStepDefinition(method, pattern, objectFactory));
}
} catch (NoSuchMethodException e) {
@@ -101,9 +100,8 @@ void addStepDefinition(Annotation annotation, Method method) {
}
}
- void addHook(Annotation annotation, Method method) {
- Class<?> clazz = method.getDeclaringClass();
- registerClassInObjectFactory(clazz);
+ void addHook(Annotation annotation, Class<?> candidateClass, Method method) {
+ registerClassInObjectFactory(candidateClass);
Order order = method.getAnnotation(Order.class);
int hookOrder = (order == null) ? Integer.MAX_VALUE : order.value();
View
51 java/src/test/java/cucumber/runtime/java/ClasspathMethodScannerTest.java
@@ -0,0 +1,51 @@
+package cucumber.runtime.java;
+
+import static java.util.Arrays.*;
+
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Set;
+
+import org.junit.Assert;
+import org.junit.Test;
+import org.mockito.Mockito;
+import org.mockito.internal.util.reflection.Whitebox;
+
+import cucumber.annotation.Before;
+import cucumber.io.ClasspathResourceLoader;
+import cucumber.runtime.Glue;
+import cucumber.runtime.java.test2.Stepdefs2;
+
+/**
@aslakhellesoy Owner

When you remove this I'll merge this in. I don't want my name associated with removal of someone else's copyright. In fact, it would be better if the copyright wasn't in any of the commits (you can force push or create a new pull request).

I don't mean to be pedantic here, but this sort of thing has been abused in the past and I want my back clear.

No need to explain. I'm on board with what you said. Thank you for catching this. Having copyright there is probably more problem for me then for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ * {description}
+ *
+ * @version $Revision$
+ */
+@SuppressWarnings("unchecked")
+public class ClasspathMethodScannerTest {
+
+ @Test
+ public void loadGlue_should_not_try_to_instantiate_super_classes() {
+
+ ClasspathMethodScanner classpathMethodScanner = new ClasspathMethodScanner(new ClasspathResourceLoader(Thread.currentThread()
+ .getContextClassLoader()));
+
+ ObjectFactory factory = Mockito.mock(ObjectFactory.class);
+ Glue world = Mockito.mock(Glue.class);
+ JavaBackend backend = new JavaBackend(factory);
+ Whitebox.setInternalState(backend, "glue", world);
+
+ // this delegates to classpathMethodScanner.scan which we test
+ classpathMethodScanner.scan(backend, asList("cucumber/runtime/java/test2"));
+
+ Set<Class<?>> stepdefClasses = (Set<Class<?>>) Whitebox.getInternalState(backend, "stepDefinitionClasses");
+ Assert.assertEquals(new HashSet<Class<?>>(Arrays.asList(Stepdefs2.class)), stepdefClasses);
+ }
+
+ public static class BaseStepDefs {
+
+ @Before
+ public void m() {
+ }
+ }
+}
View
12 java/src/test/java/cucumber/runtime/java/JavaHookTest.java
@@ -44,7 +44,7 @@ public void loadNoGlue() {
@Test
public void before_hooks_get_registered() throws Exception {
backend.buildWorld();
- backend.addHook(BEFORE.getAnnotation(Before.class), BEFORE);
+ backend.addHook(BEFORE.getAnnotation(Before.class), HasHooks.class, BEFORE);
JavaHookDefinition hookDef = (JavaHookDefinition) glue.getBeforeHooks().get(0);
assertEquals(0, glue.getAfterHooks().size());
assertEquals(BEFORE, hookDef.getMethod());
@@ -53,7 +53,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), AFTER);
+ backend.addHook(AFTER.getAnnotation(After.class), HasHooks.class, AFTER);
JavaHookDefinition hookDef = (JavaHookDefinition) glue.getAfterHooks().get(0);
assertEquals(0, glue.getBeforeHooks().size());
assertEquals(AFTER, hookDef.getMethod());
@@ -62,7 +62,7 @@ public void after_hooks_get_registered() throws Exception {
@Test
public void hook_order_gets_registered() {
backend.buildWorld();
- backend.addHook(AFTER.getAnnotation(After.class), AFTER);
+ backend.addHook(AFTER.getAnnotation(After.class), HasHooks.class, AFTER);
HookDefinition hookDef = glue.getAfterHooks().get(0);
assertEquals(1, hookDef.getOrder());
}
@@ -70,7 +70,7 @@ public void hook_order_gets_registered() {
@Test
public void hook_with_no_order_is_last() {
backend.buildWorld();
- backend.addHook(BEFORE.getAnnotation(Before.class), BEFORE);
+ backend.addHook(BEFORE.getAnnotation(Before.class), HasHooks.class, BEFORE);
HookDefinition hookDef = glue.getBeforeHooks().get(0);
assertEquals(Integer.MAX_VALUE, hookDef.getOrder());
}
@@ -78,7 +78,7 @@ public void hook_with_no_order_is_last() {
@Test
public void matches_matching_tags() {
backend.buildWorld();
- backend.addHook(BEFORE.getAnnotation(Before.class), BEFORE);
+ backend.addHook(BEFORE.getAnnotation(Before.class), HasHooks.class, BEFORE);
HookDefinition before = glue.getBeforeHooks().get(0);
assertTrue(before.matches(asList("@bar", "@zap")));
}
@@ -86,7 +86,7 @@ public void matches_matching_tags() {
@Test
public void does_not_match_non_matching_tags() {
backend.buildWorld();
- backend.addHook(BEFORE.getAnnotation(Before.class), BEFORE);
+ backend.addHook(BEFORE.getAnnotation(Before.class), HasHooks.class, BEFORE);
HookDefinition before = glue.getBeforeHooks().get(0);
assertFalse(before.matches(asList("@bar")));
}
View
4 java/src/test/java/cucumber/runtime/java/JavaStepDefinitionDependencyInjectionTest.java
@@ -41,7 +41,7 @@ public void loadNoGlue() {
@Test
public void constructor_arguments_get_registered() {
backend.buildWorld();
- backend.addStepDefinition(GIVEN.getAnnotation(Given.class), GIVEN);
+ backend.addStepDefinition(GIVEN.getAnnotation(Given.class), Steps.class, GIVEN);
verify(mockObjectFactory).addClass(Steps.class);
verify(mockObjectFactory).addClass(StepContext1.class);
verify(mockObjectFactory).addClass(StepContext2.class);
@@ -50,7 +50,7 @@ public void constructor_arguments_get_registered() {
@Test
public void constructor_arguments_get_registered_exactly_once() {
backend.buildWorld();
- backend.addStepDefinition(OTHER_GIVEN.getAnnotation(Given.class), OTHER_GIVEN);
+ backend.addStepDefinition(OTHER_GIVEN.getAnnotation(Given.class), OtherSteps.class, OTHER_GIVEN);
verify(mockObjectFactory, times(1)).addClass(OtherSteps.class);
verify(mockObjectFactory, times(1)).addClass(StepContext3.class);
verify(mockObjectFactory, times(1)).addClass(StepContext4.class);
View
6 java/src/test/java/cucumber/runtime/java/JavaStepDefinitionTest.java
@@ -51,8 +51,8 @@ public void loadNoGlue() {
@Test(expected = AmbiguousStepDefinitionsException.class)
public void throws_ambiguous_when_two_matches_are_found() throws Throwable {
- backend.addStepDefinition(FOO.getAnnotation(Given.class), FOO);
- backend.addStepDefinition(BAR.getAnnotation(Given.class), BAR);
+ backend.addStepDefinition(FOO.getAnnotation(Given.class), Defs.class, FOO);
+ backend.addStepDefinition(BAR.getAnnotation(Given.class), Defs.class, BAR);
Reporter reporter = mock(Reporter.class);
runtime.buildBackendWorlds();
@@ -62,7 +62,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(FOO.getAnnotation(Given.class), FOO);
+ backend.addStepDefinition(FOO.getAnnotation(Given.class), Defs.class, FOO);
Reporter reporter = mock(Reporter.class);
runtime.buildBackendWorlds();
View
6 java/src/test/java/cucumber/runtime/java/test2/Stepdefs2.java
@@ -0,0 +1,6 @@
+package cucumber.runtime.java.test2;
+
+import cucumber.runtime.java.ClasspathMethodScannerTest;
+
+public class Stepdefs2 extends ClasspathMethodScannerTest.BaseStepDefs {
+}
Something went wrong with that request. Please try again.