hooks should not require public visibility -- at least should log an error/warning

[elonderin]

Is your feature request related to a problem? Please describe. I was frustrated when i tried to figure out why my tests would fail. Until i notices that my setup was not public. From the old days in junit i know that they used to require that too, but no more and nowadays it's so common to have all that test stuff with package visibility (to avoid cluttering autocompletion in IDEs i guess) I just did it here too w/o even giving it a 2nd though.

So now my code looks like so to make my team mates (we are all new CC) aware of the fact.

   @Before //method must be public!! otherwise it's silently ignored!
    public void setUp() {
        msUniteTestHarness.reset();
    }

Describe the solution you'd like Also allow package visibility but by all means don't just silently drop the execution

Describe alternatives you've considered Throwing an error would be OK too but behavior is not anymore in alignment with junit.

[mpkorstanje]

This is an acceptable suggestion, but:

1. This would be a breaking change. So it would have to go into the next major version. We are currently in the process of releasing v7 RC1 so this would have to be resolved in relatively short order. Otherwise the next opportunity would be v8. Possibly next year but impossible to say as this is an open source project.

2. The motivation behind JUnit 5 not requiring methods to be public was to reduce the amount of ceremony needed to write tests. Specifically test classes and methods are can be package-private, projected or public. Merely allowing methods to not be public but classes would only lead to more confusion. So any pull requests that address this issue would have to be equally motivated to reduce ceremony and address both method and class visibility.

   Fortunately both are covered by the same section of code:

   https://github.com/cucumber/cucumber-jvm/blob/6a4567a7a32d904debbfda2d77feab0a9569e547/java/src/main/java/io/cucumber/java/MethodScanner.java#L23-L45

Would you be interested in creating a pull request?

[elonderin]

I wish i had the time then i would. Not seeing this though...

This would be a breaking change.

May i ask why ? After all, the methods which are now public would still work or is there some impl. detail i dont understand?

[mpkorstanje]

Step definitions and hooks that would previously be ignored would now be discovered. Even though this may not have been intentional, it would change an existing setup.

[elonderin]

is there a timeline on v8 ? I guess it's still long way out/unknown, or ?

maybe have an opt-in property for v7?

[aslakhellesoy]

We don't use timelines or ETAs since contributions are mostly made on a voluntary basis.

[mpkorstanje]

maybe have an opt-in property for v7?

Currently there is no system to provide properties to the BackendSupplier so that would also be a breaking change. Not to mention that cucumber would be carrying the complexity cost.

[rozenshteyn]

I was considering working on this issue. So, I took a look at the code of the MethodScanner mentioned above and it only requires the test class to be public not the methods annotated with the hook annotations in the test class. Maybe JUnit's @Before annotation was accidentally used or the problem with the Cucmber's @Before annotation processing lies elsewhere.

[mpkorstanje]

it only requires the test class to be public not the methods annotated with the hook annotations in the test class.

I don't believe that is correct. https://docs.oracle.com/javase/8/docs/api/java/lang/Class.html#getMethods--

[rozenshteyn]

Oops. You're right. I will test out what happens if getDeclaredMethods() is used instead of getMethods().

[rozenshteyn]

I've made progress in implementing a solution for this request. I do have a question about some functionality though. There is a test in MethodScannerTest and JavaBackendTest that tests the following scenario. When there is a class

package io.cucumber.java.incorrectlysubclassedsteps;

import io.cucumber.java.steps.Steps;

public class SubclassesSteps extends Steps {

}

that extends

package io.cucumber.java.steps;

import io.cucumber.java.en.Given;

public class Steps {

    @Given("test")
    public void test() {

    }

}

an io.cucumber.java.InvalidMethodException is expected to be thrown by the MethodScanner class. Since my implementation of the requested feature involves changing from using Class.getMethods() to Class.getDeclaredMethods(), methods of the parent class are no longer being considered. Is such a test still valid? I think for the child class like the one above my change should be ok (the child class has no annotations of its own so none of its methods would be in play). However, what about the following child classes?:

package io.cucumber.java.incorrectlysubclassedsteps;

import io.cucumber.java.steps.Steps;

public class SubclassesSteps extends Steps {

    @Before
    public void before() {

    }
}

or

package io.cucumber.java.incorrectlysubclassedsteps;

import io.cucumber.java.steps.Steps;

public class SubclassesSteps extends Steps {

    @Given("test")
    public void test() {

    }
}

I assume that Cucumber tries to invoke the collected annotated methods, but in what order? Is it ok if parent and child classes have non-overlapping annotations? Or, should an io.cucumber.java.InvalidMethodException still be thrown if class that extends another class with Cucumber annotations is encountered?

Thanks.

[mpkorstanje]

Cucumber does not allow step definitions on super classes. It results in duplicate step definitions.

class A {
   @Given("^hello .*$")
   public void example(){
   }
}

class B extends A {

}

class C extends A {

}

For example when classes B and C (and A) are scanned for glue this always results in a duplicate step definition because both B and C (and A) have the example method which would register a step definition with the same regex. Because the regex is the same, both step definitions would match the same step and Cucumber would not have a way to disambiguate. For consistency (and implementation convenience) this rule is also applied to other methods such as hooks.

[mpkorstanje]

Though to resolve this, I don't think you have to calculate the actual methods of a class.

abstract class A {
   @Given("^hello .*$")
   public void example(){
   }
}

class B extends A {
   @Override
   public void example(){
   }
}

In pseudo code:

class to scan = B
current class = class to scan
while current class != null {
  scan methods on current class
  current class = parent class of current class
}
reject any methods found not from current class

[rozenshteyn]

In the implementation that I'm trying to test I'm using Class.getDeclaredMethods() which:

public Method[] getDeclaredMethods()
                            throws SecurityException

Returns an array containing Method objects reflecting all the declared methods of the class or interface represented by this Class object, including public, protected, default (package) access, and private methods, but excluding inherited methods.

So, there should not be ambiguity as to which class a method came from - it would only be from the actual class being scanned. If the getDeclaredMethods() method is used, then in the following scenario:

class A {
   @Given("^hello .*$")
   public void example(){
   }
}

class B extends A {
   @Override
   public void example(){
   }
}

the scan result would only contain class A with its own example() method. On the other hand in the following scenario:

class A {
   @Given("^hello .*$")
   public void example(){
   }
}

class B extends A {
   @Override
   @Given("^hello .*$")
   public void example(){
   }
}

the scan result would contain both classes A and B, each with its own example method.

[mpkorstanje]

We would still have to scan the super class. Just to let users know they made a mistake.

JUnit for example does allow inheritance -it makes sense there- and that assumption is carried over to Cucumber where it does not make sense.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in two months if no further activity occurs.