Fix duplicated step definitions from the same step locations

[ffbit]

Relates to #608, #622, #632 issues.

[ffbit]

Tests to be written.

[ffbit]

@brasmusson, @martykube
Could you please review the code, guys?

[martykube]

@ffbit I've reviewed the code. It's good!

[brasmusson]

It is a matter of taste, but I would probably have used:

if (previous == null) {
    stepDefinitionsByPattern.put(stepDefinition.getPattern(), stepDefinition);
} else if (!haveSameLocation(previous, stepDefinition)) {
    throw new DuplicateStepDefinitionException(previous, stepDefinition);
}

In a method named addStepDefinition, the if (previous == null || haveSameLocation(previous, stepDefinition)) triggered a "no, it is not right to add the stepDefinition in both these cases" in me, but it is actually ok, since put overwrites the previous step definition with the new one.

[ffbit]

changed.

[brasmusson]

@ffbit For a test, see https://github.com/brasmusson/cucumber-jvm/tree/ffbit-fix-duplicated-step-definitions-from-the-same-location

[ffbit]

@brasmusson
I've just pulled your test.
Thanks.

[ffbit]

@aslakhellesoy
WDYT?

[aslakhellesoy]

Thanks for all the work on this everyone. I'll have a look this weekend and let you know.

[ffbit]

@aslakhellesoy
Any comments?

[aslakhellesoy]

It seems to me that this change is a workaround for the situation when the glue path has duplicate entries, and defensively tries to cope with that situation.

Why should we allow duplicate entries in the glue path? Isn't it better to throw an exception early in that case? It sounds like this is a configuration error that the user should try to fix.

[ffbit]

@aslakhellesoy
There is a point described in the #608 issue.

Another one occurs if we've got a super class for the one annotated with the @CucumberOptions.

package a.b.c;

@CucumberOptions
public class RunCukesTest extends RunCukesTestBase {
}

package a.b.c;

public class RunCukesTestBase {
    // Some smart stuff goes here
}

The test part structure of the project might look like

a/b/c/RunCukesTestBase.java
a/b/c/RunCukesTest.java
a/b/c/SomeStepdefs.java

a/b/c/some.feature

In this case we have 2 same paths in the glue paths.

classpath:a/b/c
classpath:a/b/c

And a failure can be seen in the test log

 Tests run: 2, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 1.276 sec <<< FAILURE! - in TestSuite
runCukes(a.b.c.RunCukesTest)  Time elapsed: 0.36 sec  <<< FAILURE!
cucumber.runtime.DuplicateStepDefinitionException: Duplicate step definitions in
a.b.c.SomeStepdefs.a_step(Date) in file:/path_to_project/target/test-classes/
and
a.b.c.SomeStepdefs.a_step(Date) in file:/path_to_project/target/test-classes/
  at cucumber.runtime.RuntimeGlue.addStepDefinition(RuntimeGlue.java:47)
  at cucumber.runtime.java.JavaBackend.addStepDefinition(JavaBackend.java:117)
  at cucumber.runtime.java.MethodScanner.scan(MethodScanner.java:68)
  at cucumber.runtime.java.MethodScanner.scan(MethodScanner.java:41)
  at cucumber.runtime.java.JavaBackend.loadGlue(JavaBackend.java:78)
  ...
  at a.b.c.RunCukesTest.runCukes(RunCukesTest.java:16)


Results :

Failed tests: 
  RunCukesTest.runCukes:16 » DuplicateStepDefinition Duplicate step...

Tests run: 2, Failures: 1, Errors: 0, Skipped: 0

This problem is a sort of solved though a work around here and in the proposal #632.
The both use the same idea "Let's use distinct glue paths".

Let's continue playing with our example.
How about moving the RunCukesTestBase base class one package up, so that the test part structure of the project looks like

a/b/RunCukesTestBase.java
a/b/c/RunCukesTest.java
a/b/c/SomeStepdefs.java

a/b/c/some.feature

In this case we have 2 different paths in the glue paths.

classpath:a/b
classpath:a/b/c

And neither that mentioned work around nor the #632 works and we can see the same annoying failure message.
So the glue paths merging, if it's wanted, should be much smarter.

This pull request tries to solve the problem from the other side - by handling step definitions which have exactly the same location.

[ffbit]

@aslakhellesoy
I've middled up the #632 with the #622 in my previous message.
The right one is the #632 and I've edited the message.

[martykube]

I guess I don't understand why #632 doesn't work in your second example. The change in #632 is to keep the glue paths in a Set so it is not possible to have duplicates.

[brasmusson]

@aslakhellesoy For the case that the duplicate glue paths are coming from the user specifies multiple glue paths options that end up as duplicates, I think that you are right, an error notifying the user to fix the settings appropriate.

The complication here comes from #568 which introduces that list options (formatter, glue, tags, features) are added up from the inheritance tree. #568 intentionally implemented "adding to" behavior, to that for instance default formatter can be specified in a @CucumberOptions in a base class, and additional formatter in @CucumberOptions in subclasses. The side effect is that if base- and subclasses are in the same package (and the don't specify glue path in their @CucumberOptions), that package will be added twice to the glue paths.

So the problem needs to be solved, either in RuntimeOptionsFactory (where the problem is introduced), or in RuntimeOptions (as suggested in #632, which currently does not handle the example with base classes in parent packages), or in RuntimeGlue (as suggested in this PR).

The change in this RP has the down side (sort of) that if a user explicitly specifies --glue thepackage --glue thepackage, the user is not notified that this basically is an configuration error.

[brasmusson]

@martykube I think that #632 does not work with the second example, because a parent package and a sub package are not the identical path, so the Set will allow them both, but the lookup from glue paths include sub packages. So a step definition in the sub package will be found through both parent package path and the sub package path.

[brasmusson]

@aslakhellesoy @ffbit I did some more thinking on this issue. Before #568, the package of the class with @CucumberOptions was only added to the glue paths if the glue was not explicitly defined in @CucumberOptions, that is if otherwise no glue path at all would have been defined. The purpose of #568, as I understand it, is to get the union of all explicitly defined options from @CucumberOptions in the inheritance hierarchy (formatter, glue paths, and so on). The side effect of #568 is that the package of each class in the inheritance tree, that does not explicitly specify glue in their @CucumberOptions, are added to the glue paths (even the packages of base classes without any @CucumberOptions tag at all).

Maybe the right solution for this sort of "package defined multiple times in the glue paths" problem is to, in a sense go back to the original behaviour, only add the package of the runner class with @CucumberOptions, if not glue path has explicitly been defined in any of the @CucumberOptions in the inheritance tree. That means base class packages are never added, and the package of the runner class with the @CucumberOptions is only added if otherwise no glue path at all would have been defined.

Then we would be back to the state that if the same step definition is found through different glue paths item, then it is an configuration error which should generate an exception. WDYT?

[ffbit]

@brasmusson Good point.
And how will it solve the #608 issue?

[brasmusson]

@ffbit On the surface it might appear that the Duplicate Step Definition exception which comes from duplicate glue paths entries, is related to the issue of merging or using data from several @CucumberOptions annotations. But actually, the duplicate glue paths in the TestNG example do not come from @CucumberOptions parameters, since the only @CucumberOptions annotation in the (run by composition) example does not explicitly define the glue path. So the issue is about the handling of default glue when no glue path is explicitly defined. Digging a bit deeper reveals that the same issue exist for the handling of default feature path, and a similar issue exist for the handling of default formatter see my new PR #636.

The question is what should the following runner structure result in?

package thepackage

@CucumberOption(glue={"classpath:thepackage"})
class BaseClass {}

@RunWith(Cucumber.class)
@CucumberOption(glue={"classpath:thepackage"})
public class RunTest extends BaseClass {}

I would say that this should result in a Duplicate Step Definition exception since the user should be notified not to explicitly define duplicate (or parent package, sub package) glue paths.

[brasmusson]

Closed by 3b41a05 (the problem is fixed by #636)

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.