Fix the glue class autowiring, transaction and cucumber-glue scope issues of the spring module #711
Conversation
|
If you can, please join here Mon, May 12, 3:00 PM - 4:00 PM to discuss: https://plus.google.com/u/0/events/cstdgkbvp61g7gld8lejsap4j3o |
|
I'm getting an error: |
|
It compiles successfully on my mac Java version |
|
Then the updated version have been build successfully on Linux, Windows and MacOS (head at c5a7e38 - GitHub does not list the commits in parent-child order, it must be confused by some date stamps on the commits when I fixed older commits by amending to them) |
|
It seems to me that the underlying persistence context does not flush, when we put the @TxN tag in the see_messages.feature file. So @manytoone relationship does not work between on user messages. |
Use the first class found with spring annotation, to create the TestContextManagers for all glue classes.
Also remove the @TxN annotation from see_message.feature, since it fails if transactions are used.
Test that if a step def class autowires another step def class that has not been used yet. Then it is the instance that was autowired that are returned to the backend, when asked for an instance.
|
Just when I thought everything was working fine, I came up with a new test case for step def injection - and it fails. |
|
@brasmusson could that explain why the scenarios are failing on some machines and not on others? Do you think there is state leaking between scenarios and the execution order matters? |
|
If changing: to: was the solution to make the first step def injection test case to pass, then it is weird that accidentally almost reverting it to: instead of: could work on any platform (and not only on my machine - on Travis to, but both are Linux). But now that it did work on Linux, it gave a clue that the order things happened was important. So it led me to see that there was a scenario that was not tested. Now that I fixed the code above to the intended one, I think the first scenario is working consistently on all platforms, but that is of no use, if the the latest scenario does not work. |
Let the SpringFactory register the bean definitions of the glue classes in the bean factory of TestContextManager's context. Use this bean factoryto create the instances of the glue classes. Alse register the GlueCodeScope in bean factory of the TestContextManager's context.
Make sure that glue classes with different @ContextConfiguration/ @ContextHierarchy annotations are defined in different packages (since the SpringFactory will throw an exception if glue classes are added with @ContextConfiguration/@ContextHierarchy annotations that are not equal).
| if (stepClassWithSpringContext == null) { | ||
| stepClassWithSpringContext = stepClass; | ||
| } else { | ||
| checkAnnotationsEqual(stepClassWithSpringContext, stepClass); |
There was a problem hiding this comment.
Why do we want to fail if glue classes with different Spring annotations are found?
In real life we often see Spring-y tests that define slightly different contexts and are more or less independent from each other. AFAIU when similar independent step definitions will be loaded by the same runtime (e.g. one JUnit runner class), we'll throw an error in this PR. I'm afraid real users might find that counter-intuitive and inconvenient.
There was a problem hiding this comment.
Only one Spring test is loaded during each run, but more than one Cucumber step definition is loaded for each scenario. I agree with @brasmusson we should allow only one Spring context. @mgurov while I see advantages in injecting a specific Spring context in JUnit tests, I'd like to see an example of where this is needed in Cucumber. Supporting multiple context is controversial enough to deserve a separate entry in the issue tracker and a deeper discussion.
There was a problem hiding this comment.
Also in Cucumber-JvM v1.1.3 only one spring context was allowed, then it was hardcoded that the file cucumber.xml on the classpath defined the spring context.
From an implementation point of view, probable the biggest problem with allowing multiple context is to find out in which the SpringTransactionHooks need to be in to find the bean implementing the PlatformTransactionManager it needs to do its job.
There was a problem hiding this comment.
@paoloambrosio in general I agree that less contexts is better but IMHO it is not the mission of cucumber-jvm to force one or another spring practice.
I'm a bit concerned about the proliferation of the RunCukesTest.javas that we see even in this change, but if you guys do not see an issue with that then fine with me.
There was a problem hiding this comment.
@mgurov I just do not see how it should work in detail. In which of the several context should the SpringTransactionHooks class be put (so it finds the PlatformTransactionManager it needs)? Or should it be forbidden to have more than one context if you use transactions?
And what about glue classes without context annotations, in which context should they be put? Or should we require context annotations on all glue classes (that use spring)? I have got the impression that the requirement that every glue class (that use spring) must have an context annotation is one of the problems the v1.1.6.
There was a problem hiding this comment.
@brasmusson yes, that can be tricky with the TestContextManager. One of the options could be to (partially) reimplement it for our needs and build the context (or hierarchy of contexts) ourself. But maybe the current solution would be good enough as long as heavy spring module users do not complain.
brasmusson
changed the title
In the case that @ContextConfiguration annotation is not used, first try and use the configuartion file classpath:cucumber.xml, if it does not exist create en plain spring context with only the glue classes in it.
|
I'd like to merge this PR to master and release 1.1.8 unless someone has any big issues. You have done some fantastic work here @brasmusson. Are there any other open Spring-related tickets that we need to deal with or does this cover them all? |
|
Well, the "missing docs" part of #569 is still a bit weak. On the plus side, with the latest changes on this PR, it is possible to upgrade from v1.1.3 without changing anything with respect to spring. Apart from that I'm not aware of any. As for a release 1.1.8, I would suggest to also include #713 (Allow empty doc string and data table entries after token replacement from scenario outlines) och #673 (Expose Scenario id to step definitions). Small changes, but still valuable to those that raised underlying issues. |
|
On Wed, 21 May 2014, at 01:37 AM, Björn Rasmusson wrote: Well, the "missing docs" part of [1]#569 is still a bit weak. On the Missing docs do need work, but a stable release that is backwards As for a release 1.1.8, I would suggest to also include [2]#713 — Reply to this email directly or [4]view it on GitHub. References
|
|
Hi, Cheers! |
|
Well, I've just found out that this pull request makes cucumber-spring dependent on Spring-4.0.2 and is not backwards compatible. We use Spring 3.1 so This is unfortunately of no use to us. |
|
@pberlowski It is not this pull request in itself that makes cucumber-spring dependent on Spring-4.0.2. Its that fact that it is created after Cucumber-JVM v1.1.6 was released, in which cucumber-spring is dependent on Spring-4.0.2. This pull request can be applied on Cucumber-JVM v1.1.5 also (for instance), in which cucumber-spring is dependent on Spring-3.2.4. But you are right in the sense that the version in which this pull request is merged and released in, that version will have cucumber-spring dependent on Spring-4.0.2 (if applying this pull request to Cucumber-JVM v1.1.5, then ) |
|
@brasmusson Yeah, I did some digging and came to the same conclusion. Right now we forced our integration test project to use spring 4.0.2. No major vents yet, so we're happy to fly. As I said, I've rebased the current master underneath this pull request and popped that into our nexus. It seems to work fine and does what we initially expected cucumber to do. Many thanks for the solution, I was really surprised when I initially debugged into the SpringFactory... |
|
Can someone please tell me that the current Cucumber 1.1.7 release which version of Spring is compatible with? |
|
Please see CONTRIBUTING.md and the topmost pom.xml for the spring version. Use GitHub to see the pom.xml for the v1.7.7 tag. |
|
Sorry, didn't mean to close this. |
Fix the glue class autowiring, transaction and cucumber-glue scope issues of the spring module
|
At the Cucumber OSS hack night... we thing it works. Crossing our fingers... |
|
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. |
Autowiring one glue class to a field of another glue class does not work. It has been noted, in a comment on another spring issue and on the mailing list, but no issue has been created for it.
This PR fixes the issue with autowiring glue classes . It make the
cucumber-gluescope available in the configuration xml files (fixes #600). It also make the transaction support work again (fixes #637). It also loosens the requirement that each glue class needs an@ContextConfiguration/@ContextHierarchyannotation.The final state is that:
@ContextConfiguration/@ContextHierarchyannotation defining the location of the context configuration file ("cucumber.xml").@ContextConfiguration/@ContextHierarchyannotation, all annotations on those classes must be equal.@ContextConfiguration/@ContextHierarchy, other annotations are read (like@DirtiesContextor@WebAppConfiguration), they will be applied to the glue as a whole.@ContextConfiguration/@ContextHierarchyannotation is found, it will fallback to use the "cucumber.xml" file on the classpath. If "cucumber.xml" does not exists on the classpath, the final fallback is to create a generic context with only the glue classes in.