Skip to content

Transaction commence and Rollback #46

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

Closed
patrickmcmichael opened this issue Sep 20, 2011 · 33 comments
Closed

Transaction commence and Rollback #46

patrickmcmichael opened this issue Sep 20, 2011 · 33 comments
Assignees
Labels
📖 documentation Improvements or additions to documentation

Comments

@patrickmcmichael
Copy link
Contributor

What is the current status and intended direction regarding transation/auto-rollback support for cucumber-jvm (in general, with a specific IoC factory in play)?

  • manual callouts within @before or @after?
  • counterpart for Java for Cucumber::Rails.use_transactional_fixtures?
  • flag property on @Before/@after annotation?

Thanks,

PWM

@aslakhellesoy
Copy link
Contributor

I'm not sure we'd want to build that in, as there are so many different Database APIs around. Have you tried just doing it yourself in a @before and @after hook? Shouldn't be more than a few LOC.

@patrickmcmichael
Copy link
Contributor Author

That was option #1 above, but I wanted to get a feel for the direction intended before going there.

In my case, we'll be using Spring as our IoC container. It'd be cool if the Spring Factory looked for some flag annotation that on a scenario-by-scenario basis could kick off an auto-rollback tx before scenario start, similar to how @transactional and the Spring JUnit runner work together.

@aslakhellesoy
Copy link
Contributor

I'm not sure a Java annotation would make sense here. Where would you put it? What if you could do:

public class MyTxnHooks {
    @Autowired
    private SpringTxn txn; // Or whatever it's called

    @Before
    public void startTxn() { txn.start(); }

    @After
    public void rollbackTxn() { txn.rollback(); }
}

I don't know much Spring, so let me know if that makes sense.

@patrickmcmichael
Copy link
Contributor Author

@Before/@after are scenario boundary hooks vs step boundary, correct? Working on that assumption, we can definitely do simple tx start/rollback api calls.

Was just trying to think if, when SpringFactory was in play, if the runner (@RunWith(Cucumber.class)) or a TX-aware extension of it could function more like the spring junit runner that detects @transactional and does all the tx magic and auto-rollback for you.

thx, pwm

@aslakhellesoy
Copy link
Contributor

Yes - @before runs before a Scenario, and @after runs after. There are no step hooks.

In Cucumber-Rails, transaction start/rollback is also implemented using After and Before hooks.

Are you going to contribute some code for this?

@patrickmcmichael
Copy link
Contributor Author

I'm thinking of experimenting with some annotation-based hook points...if it proves to be something that could be done declaratively vs programatically inside @Before/@after, I'll fork and contribute the result.

@patrickmcmichael
Copy link
Contributor Author

Think I have a reusable solution, though it is likely still something we'd do case-by-case vs bury in the global code, or perhaps we can add it as a support class alongside Spring for when it's useful.

Before going there, is it safe to assume that in the cuke lifecycle that any mapped stepdef or hook classes are instantiated per-scenario-run, and not reused? (in other words, if they hold state which is needed across step/hook defs (e.g. then needs state from when, or after needs state from before) is this thread safe by each scenario execution having its own runtime instance of the stepdef or hookdef class?

@aslakhellesoy
Copy link
Contributor

All classes that contain stepdefs or hooks are instantiated anew for each scenario. If you are using cucumber-spring, application beans will carry over between scenarios: https://github.com/cucumber/cucumber-jvm/blob/master/spring/src/test/java/cucumber/runtime/java/spring/SpringFactoryTest.java

Regarding thread safety - I haven't tried to run Cucumber in a multi-threaded environment. Are you talking about multiple JUnit threads, or multiple threads in the code being tested?

@mmalmeida
Copy link
Contributor

I'm also thinking about a way to tackle the problem. My main motivation is: ease of use.

Spring has a AbstractTransactionalJUnit4SpringContextTests class which you can extend in your tests (see http://www.jarvana.com/jarvana/view/org/springframework/spring-test/3.0.6.RELEASE/spring-test-3.0.6.RELEASE-sources.jar!/org/springframework/test/context/junit4/AbstractTransactionalJUnit4SpringContextTests.java?format=ok )

This makes transaction management in your test automatic: the abstract class is marked with @transactional, which means "each method is run in a transaction". As one method = one test, your tests are run inside a transaction, with the added benefit that you can set them to rollback automatically after the transaction.

Main advantages are:

  • your transaction management will be the same for cucumber and for your integration tests
  • if you let spring manage your transactions in the real application as well, you have a single point of transaction management.

The problem so far is one: those tests are run with @RunWith(SpringJUnit4ClassRunner.class), and Cucumber runs with Cucumber.class

The only way I see it is to have a Runner that has both runner's functionality. How difficult that is I have no idea.

@patrickmcmichael
Copy link
Contributor Author

@mmalmeida Good reminder to post back here...here's what I've done here...working great so far. I considered trying to generalize this enough to issue a pull request for @aslakhellesoy, but feel like some of the specifics in here, while reusable across tests, may still make assumptions that are too environment specific to try for reuse at the cucumber-jvm lib level...?

Anyhow, here's the gist:

  1. In my test class I have something like this:
@RunWith(Cucumber.class)
@Feature(value="features/foo_bar.feature", packages={"x.y.z.txhooks"})
  • Recall that @feature checks for step and hook (e.g. before/after) defs in the same package as the test file itself lives, BUT ALSO in any packages specified.
  • I leverage this, combined with a distinct package containing ONLY my TX Hook Defs, in this example x.y.z.txhooks.
  1. I then create a simple hookdef file in that distinct package, something like this...
package x.y.z.txhooks;

import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.transaction.PlatformTransactionManager;
import org.springframework.transaction.TransactionStatus;
import org.springframework.transaction.support.DefaultTransactionDefinition;

import cucumber.annotation.After;
import cucumber.annotation.Before;

public class cukespike_TxHooks {

    private TransactionStatus txStatus;

    @Autowired
    private PlatformTransactionManager txMgr;

    @Before
    public void rollBackBeforeHook() {
        txStatus = txMgr.getTransaction(new DefaultTransactionDefinition());
    }

    @After
    public void rollBackAfterHook() {
        txMgr.rollback(txStatus);
    }

}
  1. This gives me auto-rollback capability similar to the spring junit4 runner, w/o forcing us to mess with Cucumber.class as our runner at all.

  2. In the form above, all scenarios in the given @feature would get the tx behavior. If (questionable need here, IMO) flexibility is needed from one scenario to the next within a single feature file, some w/ and some w/o tx support, the @before and @after tags could leverage cucumber-jvm's tag support for these annotations to wrap ONLY those scenarios with a "txRollback" tag, for example.

a) I'm not sure how likely it is that scenarios with enough in common to be grouped under a given feature would really need to vary on their tx behavior.
b) I prefer to keep tags in a .feature file more biz-centric than tech related, though admittedly the implementation is obviously decoupled, so this ain't an unpardonable sin.

Anyhow...this approach is meeting our needs quite well so far.

@patrickmcmichael
Copy link
Contributor Author

P.S. Note that I'm using txStatus as an instance variable in the hookdef file above, and using a singleton @Autowired txMgr. Hence my prior confirmation questions for @aslakhellesoy that any stepdef or hook file is instantiated per scenario run. (see discussion above in this thread)

@aslakhellesoy
Copy link
Contributor

@patrickmcmichael that looks great! It's pretty similar to how Cucumber-Rails uses DatabaseCleaner:

https://github.com/cucumber/cucumber-rails/blob/master/lib/cucumber/rails/hooks/database_cleaner.rb

I'd be happy to add this to Cucumber-JVM. P.S. I edited your comment to get proper syntax highlighting - see the Markdown guide linked at the bottom.

@ghost ghost assigned aslakhellesoy Oct 17, 2011
@patrickmcmichael
Copy link
Contributor Author

Cool...see the rails example did go the tag route. Which levels in a .feature file can support tags...just scenario, or feature too?

@aslakhellesoy
Copy link
Contributor

Feature, Scenario, Scenario Outline and Examples can have tags.

I'd like to use the tag @txn if you don't mind.

@patrickmcmichael
Copy link
Contributor Author

Sounds good

@patrickmcmichael
Copy link
Contributor Author

Interestingly, the ruby counterpart used a negative tag (~no_database_cleaner), thus forcing inclusion ONLY when tx rollback was not the desired behavior. Not sure if that't the right way to go in the Java space or not. Also, we have the possibility of alternate IoC containers (Spring, Pico, Guice). Theoretically there'd be a SpringTxHookDef.java, a Pico one, etc, perhaps in distinct packages. But the @TxN tag itself would be common to each impl'ls @Before/@after defs.

@aslakhellesoy
Copy link
Contributor

Using DatabaseCleaner in Rails is so common that we made it opt-out instead of opt-in. I suppose it might be somewhat less common in Spring, so let's make it opt-in for now.

Nobody has expressed the need for transactions with the other DI containers yet, so we can wait with those.

@patrickmcmichael
Copy link
Contributor Author

@aslakhellesoy Are you using approach 1 or 2 here http://help.github.com/send-pull-requests/ for your branches? (e.g. topic branch off this project, or off of a fork?) I'm newer to github, so bear with me.

@aslakhellesoy
Copy link
Contributor

Option 1 - you don't have access to the main repo

@patrickmcmichael
Copy link
Contributor Author

k. Also, I was leaning towards package cucumber.runtime.java.spring.hooks -- sound good? I'm assuming no auto-plug of this hook, but that each client desiring it would reference it in the respective @feature packages property...?

@aslakhellesoy
Copy link
Contributor

Sounds good

@mmalmeida
Copy link
Contributor

@patrickmcmichael I've tried out your code and it looks very nice. It worked perfectly on a simple hibernate+spring testcase I already had.

Would your idea be to replace the packages={"x.y.z.txhooks"} notation with a class-level @TxN annotation, ie only classes with @TxN would inherit that code's behaviour?

@patrickmcmichael
Copy link
Contributor Author

The @TxN annotation will actually be referenced from the @Before and @After annotations in the hookdef class, then it may be placed in .feature files, typically at the Feature level, though other levels as @aslakhellesoy noted above, also support tagging.

I forked and branched last night but had to take off my coding hat and put my single dad hat on again last night. I hope to issue a pull request to get these changes onto the master soon.

@patrickmcmichael
Copy link
Contributor Author

It appears that you may've already done a preemptive merge of this code earlier today, but the code was still in flux. Please repull from the current state of my topic branch on my fork.

@patrickmcmichael
Copy link
Contributor Author

Aha...I see now how my prior post made it sound like all that was pending was the pull request, hence (I'm guessing) your preemptive merge. ;)

Anyhow...you'll definitely want to pull fresh...lots of mods and cleanup today, not to mention it could've been in an unstable state at that point. This code is unit tested via Mockito, but I have also tested it functionally by actually using it within my original project in place of the inline spike that spawned my work here. All seems to be in order.

@patrickmcmichael
Copy link
Contributor Author

Another big mod since your earlier merge was the conversion from setter injection to using BeanFactoryAware:

  • @autowire caused issues with the cuke tests within the cucumber-jvm/spring project
  • But setter injection (e.g. state of code when you did your earlier merge) relies on the client cuke test project getting all the pieces "just so" - brittle, smelly.
  • Going w/ BeanFactoryAware cleans this up substantially on both fronts.

@aslakhellesoy
Copy link
Contributor

@patrickmcmichael apologies for doing the preemptive merge. It looked good to me, and I incorrectly assumed you hadn't sent a pull request because you said you were new to github.

Everything looks great. Thanks a lot and keep those good patches coming!

Aslak

@mmalmeida
Copy link
Contributor

@patrickmcmichael I was testing out your code and I I've found an oddity: on a very basic test* ran 5 times from Eclipse, it works some times and not other times. A breakpoint at the @before annotation is triggered on the correct runs but not in the runs that fail.

Has this ever happened to you? By inspecting the log of the run I see they're almost identical, except in the last part, where I see some spring differences (@aslakhellesoy it might be related with Spring):
In http://pastebin.com/y5jKMfFt notice the difference in lines 6 and 20: there's a spring destroy and bean creation, but in the 1st case the cucumber stepdef beans are created and not in the 2nd case.

Do you have any idea what might be happening (before I run along creating a complex test case for it) ?

@aslakhellesoy
Copy link
Contributor

If you believe there is a bug in the existing code, please open a new ticket.

P.S. Please use gist.github.com for output - it doesn't have all that advertising crap

@patrickmcmichael
Copy link
Contributor Author

I haven't experienced that behavior myself across multiple, successive, Eclipse-triggered runs. I can say that the code (see classics of tx hooks class) does rely on the fact that hookdef classes will be instantiated per scenario run. (@aslakhellesoy this is true even when the matching annotation (@TxN) for the hookdef is at the feature level, correct? still instantiated per scenario within the annotated feature?)

I believe in my early spikes of this, I had actually wired in a dummy constructor just to make sure I got a fresh instance per scenario run, and did a poor-man's confirmation that way.

@mmalmeida
Copy link
Contributor

I have found the origin of the issue:

  • While my hibernate_Test runs HibernateStepDefs, I had AnotherStepDefs with a
    @Before
    public void setUp(){
        super.setUp(); //goes to the database
    }

what happens is that when the transaction @before is run before setUp, the test works; when setUp runs before the txn, it fails.

I was treating cucumber's @before as JUnit before - I did not realise it ran on all features regardless of the place it was declared. So maybe I need to refactor to put my setUp in another place.

Regardless of this, there is the matter that there doesn't seem to be a fixed order for the @before methods (were you aware of this Patrick and Aslak?), so I leave this comment as a reminder for it, in case you weren't aware.

@patrickmcmichael
Copy link
Contributor Author

I know from the docs that the packages scanned for stepdefs include the package the *_Test class with @RunWith(Cucumber.class) and @Feature reside in. Then, any packages specified in the packages attribute of @Feature are scanned. As to order, it would be nice to be able to govern order somehow. That was a concern I had when I wrote the common hookdef for tx support.

@lock
Copy link

lock bot commented Oct 25, 2018

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.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
📖 documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

3 participants