-
-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
hooks should not require public visibility -- at least should log an error/warning #2370
Comments
This is an acceptable suggestion, but:
Would you be interested in creating a pull request? |
I wish i had the time then i would. Not seeing this though...
May i ask why ? After all, the methods which are now public would still work or is there some impl. detail i dont understand? |
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. |
is there a timeline on v8 ? I guess it's still long way out/unknown, or ? maybe have an opt-in property for v7? |
We don't use timelines or ETAs since contributions are mostly made on a voluntary basis. |
Currently there is no system to provide properties to the |
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 |
I don't believe that is correct. |
Oops. You're right. I will test out what happens if getDeclaredMethods() is used instead of getMethods(). |
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. 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. |
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 |
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:
|
In the implementation that I'm trying to test I'm using Class.getDeclaredMethods() which:
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. |
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. |
… in non-public methods. Fixes issue cucumber#2370
…tations in non-public methods (cucumber#2370)
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. |
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.
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.
The text was updated successfully, but these errors were encountered: