Skip to content
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

Allowed customization of Spring step definitions context #203

Merged

Conversation

vladimirkl
Copy link
Contributor

Hi,
I've created this pull request because current implementation of SpringFactory makes impossible to customize step definitions context. Actually we have singleton parent context configured in cucumber.xml and child StaticApplicationContext that is created on the fly for every scenario. SpringFactory registers only AutowiredAnnotationBeanPostProcessor in child context and that's all. There is no way to register any additional bean post processor or bean factory post processor. We cannot have PropertyPlaceholderConfigurer in child context, @PersistenceContext annotation support and many other useful things.

So I made the following:

  • Replaced child StaticApplicationContext with child ClassPathXmlApplicationContext
  • Made child ClassPathXmlApplicationContext to take "classpath*:cucumber-steps.xml" location as constructor parameter. So it finds all cucumber-steps.xml files on the classpath.
  • Added basic cucumber-steps.xml file to cucumber-spring. This file has only context:annotation-config declaration - to made autowiring annotations working from scratch.

So any additional cucumber-steps.xml files may add customization to step definitions context. I've added one with PropertyPlaceholderConfigurer to test suite and added a test checking that it is working.

SpringFactory now searches for cucumber-steps.xml and if found it - uses for configuring  step definitions context.
@aslakhellesoy
Copy link
Contributor

@ConradMueller you contributed the last major changes here. Can you look over this and let us know if you agree? I'm not familiar enough with Spring to understand all of this.

autowirer.setBeanFactory(stepContext.getBeanFactory());
stepContext.getBeanFactory().addBeanPostProcessor(autowirer);
stepContext.getBeanFactory().addBeanPostProcessor(new CommonAnnotationBeanPostProcessor());
stepContext = new ClassPathXmlApplicationContext(new String[]{"classpath*:cucumber-steps.xml"},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you rename this to cucumber-glue.xml instead please?

@conradthukral
Copy link
Contributor

Looks like a good idea to me (and it simplifies the code, too...).

@aslakhellesoy aslakhellesoy merged commit feb8d43 into cucumber:master Feb 17, 2012
@TeDDaN
Copy link

TeDDaN commented Feb 22, 2012

@vladimirkl and @ConradMueller this chance have the side effect that a new spring context (for the cucumber-glue.xml) is created for each senario. This gives me a big performance hit.

I am not absolutely sure about the intention of this. But I am guessing that if the cucumber-glue.xml is loaded once the same functionality would achieved and execution a lot faster.

@vladimirkl
Copy link
Contributor Author

New spring child context was always created for new scenario. This is behaviour by design - so that each scenarion is completely isolated. My contribution did not change this - I only allowed customization of child context. And parent context from cucumber.xml is still singleton

@TeDDaN
Copy link

TeDDaN commented Feb 22, 2012

Yes
My theory is that creating a child context for cucumber-glue.xml triggers a annotation scan and that is slow. This is done for each step. Which is a new behavior.

Before this fix the first scenario to about 3 seconds to run and the following about 0.3-0.4 seconds. No every feature takes about 3-4 seconds.

@vladimirkl
Copy link
Contributor Author

Old behaviour registered AutowiredAnnotationBeanPostProcessor - that was doing annotation scan on every bean in child context. I replaced it with context:annotation-config. According to docs it registers PersistenceAnnotationBeanPostProcessor, AutowiredAnnotationBeanPostProcessor, CommonAnnotationBeanPostProcessor and RequiredAnnotationBeanPostProcessor. This of course adds some overhead but not much. I've never experienced such delays like you. Without those PostProcessors spring autowiring in glue code is very limited

@conradthukral
Copy link
Contributor

Could the classpath* search slow it down that much?
(as the added post processors don't seem to be likely culprits?)

@TeDDaN
Copy link

TeDDaN commented Feb 22, 2012

I am not sure what the reason for the slow down is.
But it really slows down. I have tested with this fix as the only difference.
Running the tests have become disk bound on my computer with a ssd hard drive.

@vladimirkl
Copy link
Contributor Author

@TeDDaN, I've made some optimizations - so classpath scanning occurs only ones. Please check this in my repository: vladimirkl/cucumber-jvm, branch spring-optimization. Let me know if this solves your issue

@vladimirkl
Copy link
Contributor Author

Also cucumber core calls SpringFactory.addClass() several times for single glue code class - so same bean definintions were registered multiple times. I've fixed this by storing classes in HashSet - so we have exactly one bean definition for every glue code class

@TeDDaN
Copy link

TeDDaN commented Feb 23, 2012

It didn't solve the performance issue.

@vladimirkl
Copy link
Contributor Author

I've created a feature with scenario outline including 500 examples, so I have spring context created 500 times. This scenario executed in 7 seconds - it's really fast. So I cannot reproduce performance issue. Is it possible that you create a testcase that performs really slow?

@aslakhellesoy
Copy link
Contributor

@TeDDaN I suggest you create a new ticket for the performance degradation. This ticket is closed, and closed tickets tend to be forgotten.

@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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants