Provide a configurable timeout value for each story running time

Has this been resolved with JBEHAVE-1037 ?

No, there is currently only one configured timeout value.

How about if I implement this using a new Ant task such as: specificStoryTimeoutInSecs="*/Class1.java-500,/Class2.java-700,*/Class3.java-10000".

Format: specificStoryTimeoutInSecs=CSV of "className-timeInSec"

Thoughts about this design? I cannot subscribe to the mailing lists so had to discuss here. I also thought about adding this somehow to the Config/Runner class for a specific story, but it would be easier to configure via a build.xml or pom.

This features requires two main elements:

• configuration: add a configuration element similar to storyTimeoutInSecs, which we may call storyTimeoutInSecsByPath, which will override the default value in storyTimeoutInSecs, and can be expressed as a CSV of regex expressions matching story paths. E.g. "*/long/.story:5000,*/short/.story:200". Note that there are several places where the storyTimeoutInSecs is configured and an equivalent element needs to be added for storyTimeoutInSecsByPath (using the type String in place of long).
• implementation of logic: in StoryManager the story timeouts by paths need to be parsed and matched according to the story path being executed

To subscribe to the mailing lists you need to create a Codehaus account and then subscribe via the project site:

http://xircles.codehaus.org/projects/jbehave

FYI: I have been working on this for some time and am close to completion/submitting a pull request.

Pull Request Sent for this!:
https://github.com/jbehave/jbehave-core/pull/74

Mauro , just want to ensure you are aware of this Pull Request. No rush though - our team will eventually use this but it won't be right away.

Hi Christopher,

yes, I've seen it thanks. I'll get to it by the end of this week.

Cheers

no problem - no rush. thanks for confirming.

Hi Christopher,

finally got around to the patch. Can you please explain why you need to introduce the method

public String getSearchDirectory() 

in the EmbedderMonitor?

If you need to configure anything, it should not be in the monitor class.

After parsing the new Ant Task property "useStoryTimeoutInSecsByPath", I want to ensure that each story path specified is a valid path.

For example, suppose useStoryTimeoutInSecsByPath="*/test1.story:15,*/test2.story:77"
and in this case "test2.story" is not a valid path (that story doesn't exist).

To do this check, I used StoryFinder.findPaths(String searchIn, String include, String exclude) on line 325 of StoryManager.java.

In order to use this method, I had to pass in the correct path to "searchIn", which was already being conveniently determined in AbstractEmbedderTask.searchDirectory(), line 180.

So in summary, I introduced this method in order to take advantage of AbstractEmbedderTask.searchDirectory(), which is accessible via the "embedderMonitor" field in StoryManager.java.

You're mixing two very different concerns here, configuration and monitoring.

What matters is whether the path of the story we're running matches the regex patterns provided in the CSV of story timeouts.

I'm looking into simplify your solution.

Hi Chris,

I've pushed a refactored version. It passes all the tests that you had written (well done, by the way, for the excellent testing coverage of your patch!).

The new version simply checks the story path matches the regex pattern provided (well, almost ... replacing ** -> .*).

Also gone is the timeValuesUsed map which was a rather ugly hack, only there for testing purposes. Again, the monitor pattern has been used to verify behaviour in an IOC-friendly way.

Could you please pull latest and test it works for you?

Cheers

Thank you very much for the additions and feedback. I am in the process of testing and will revert back soon.

Hi Mauro,

Its working very well - I tested thoroughly and don't see any additional issues. Given your simplified regex pattern solution, I think it would be best to update all places in code where I give examples of how to use this new "storyTimeoutInSecsByPath".

In my first commit, all of my examples/comments/unit tests used this format: E.g. "*/long/*.story:5000,*/short/*.story:200"

At the time, I was thinking of a scenario where someone might want to assign a specific timeout to a whole suite of tests under a package such as "long" or "short".

But after reviewing your changes, I think it would be clearer to use this format for the examples: E.g. ".*Long.*.story:5000,.*Short.*.story:200"

This new example would show that its fairly easy to assign a timeout to any tests with "Long" or "Short" in their names.

Essentially keeping the example as simple as possible and showing users that they can simply use ".*" for matching with story path.

Thoughts?

Hi Christopher,

happy to hear that it works well for you. Not sure I understand your comment, though.

The current path format allows use of asterisks [*], which will be converted to regex characters [.*]

Can you clarify what you mean?

Cheers

Post Implementation Discussion Between Mauro and Chris Aguirre

Mauro,

I have uploaded a Word Doc named "JBEHAVE-833_Explanation.docx" to this issue in order to avoid formatting mistakes here in JIRA. Please let me know if you have issues opening the doc and I can send in another format or via another means.

I think it'd be much easier to modifiy the examples' CoreStories configuration to reproduce the behaviour in the doc.

Can you provide such pull request please? I'd be interested in understanding what works and what does not for you.

Thanks

Mauro,

As I work on this pull request, I want to confirm that I am making the changes in the correct place.

I am adding the example usage for "storyTimeoutInSecsByPath" in jbehave-examples/core/pom.xml - is this what you are expecting?

I want to confirm because I tried updating CoreStories.java but its inherently using UnmodifiableEmbedderControls which doesn't allow me to set my own timeout values.

Hi Christopher,

you can set it either in CoreStories or in the pom.xml but be aware that the pom.xml overrides what is set in the embedded Java configuration.

Mauro,

I've updated the pom.xml and added the changes to this pull request: https://github.com/jbehave/jbehave-core/pull/74

It actually works pretty well in the examples' CoreStories context - as I originally expected and described in # 1 of my Word Doc. I did have to use "dot stars" though which you can see in the pom.xml.

The example in the pom.xml is sufficient to give a user an idea of how to use "storyTimeoutInSecsByPath". I have also included a screen shot (storyTimeoutInSecsByPath_inAction.JPG) of the console statements which show what timeouts each story run were using based on the pom.xml.

Hi Christopher,

I've committed a fix to support both ant-style patterns (a la StoryFinder) and regex patterns.

I've also updated the core examples pom.xml with story timeouts.

Can you please give it a whirl and let me know if we can resolve this issue?

Mauro,

Overall its working very well when directories are not involved in "storyTimeoutInSecsByPath". When directories are involved, running from the pom.xml works well, but running from ant like we do in our company does not. I've attached a new Word Doc (JBEHAVE-833_TestFindings.docx) with specific examples of what worked/didn't work for me.

In my opinion, we should leave this as is now. When I originally started this patch, I didn't realize the "storyTimeoutInSecsByPath" would be matched against the story path name only i.e. "and_step.story". Personally, I would be fine with not putting directories in "storyTimeoutInSecsByPath" as a user. We would just need to update all the examples/documentation to not include directory names so users aren't confused as to what works/what doesn't. On the plus side, the users still have the flexibility to use "stars only" or "dot stars" which is nice.

Let me know what you think. If you do agree to stick with what we currently have, then I can submit a patch to update the examples/comments/unit tests with ones that reflect what works well.

Hi Christopher,

thanks for the update. I'm quite puzzled by the difference between Maven and Ant. The logic of the timeout matching is entirely external to either, being in the StoryManager. Can you offer more information or a reproduceable example?

As for the behaviour in presence of slashes, could you please see if you can reproduce at unit level in the StoryManagerBehaviour?