Details
Improvement
Open
Major
Description
Attached is a patch for JBehave.
The main change is the addition of a new parser. Its based on Eclipse JFace, which means we can track line numbers in error reporting. It can also be re-used for syntax highlighting in an Eclipse editor. JFace requires some Eclipse libraries, so these are new dependencies which still work even though we're not running in an OSGi container. The parser is also very fast compared to the current regex-based parser - the longest running test was 25s for the previous parser, and now its just 0.5s.
In ScenarioParserBehaviour we've written some tests for the default configuration of JUnitScenario. They fail with the current parser but pass with the new one which is able to throw a new ParseException for each type of problem.
We've added one test to CandidateStepBehaviour.java that currently fails. Doug tried fixing it by changing the ExamplesTable to be backed by a LinkedHashMap which means that the arguments are automatically in the correct order. But this did not work for other examples and couldn't figure out how to make it work, so the test is currently broken. We'd need help to fix this.
Further proposed changes would improve the detection of problems such as:
- not having enough columns in the examples section to cover all the required variables
- having more columns than required in the examples section
- checking the order of statements e.g. ensure a When comes after a Given
- allowing boolean's for parameters
The first two are quite difficult as it involves a good knowledge of the CandidateStep class.
Activity
Hi Mike, thanks for the contributed patch. It looks very interesting, but I have few comments:
1. There is no mention of which JFace dependencies are required. Are they available on Maven Central repo? If so, it would be great if you could amend the pom.xml to declare it and prove it builds from CLI. If not, that poses a problem and we could not accept the patch until the dependencies are uploaded to Central or somewhere else accessible from network (via mvn, ant or gradle).
2. The patch is for JBehave 2.x (svn) and not for JBehave 3.x (git). Given that it intended for 3.1 it would help if you could provide the patch directly for the 3.x branch.
3. Given that the contribution focuses on the integration with JFace toolkit, it would probably make sense to have a separate module, e.g. jbehave-jface. Makes for much easier dependency management.
4. There are copyright notices to Yell Group in the Java source files. By submitting the patch, you are implicitly transfering to the JBehave project the ownership of the copyright, so the copyright notice should be removed (the @author tags could and should stay). Even better if you explicitly waived any claim of copyright from Yell or personally from the authors.
Mauro Talevi
added a comment - Hi Mike, thanks for the contributed patch. It looks very interesting, but I have few comments:
1. There is no mention of which JFace dependencies are required. Are they available on Maven Central repo? If so, it would be great if you could amend the pom.xml to declare it and prove it builds from CLI. If not, that poses a problem and we could not accept the patch until the dependencies are uploaded to Central or somewhere else accessible from network (via mvn, ant or gradle).
2. The patch is for JBehave 2.x (svn) and not for JBehave 3.x (git). Given that it intended for 3.1 it would help if you could provide the patch directly for the 3.x branch.
3. Given that the contribution focuses on the integration with JFace toolkit, it would probably make sense to have a separate module, e.g. jbehave-jface. Makes for much easier dependency management.
4. There are copyright notices to Yell Group in the Java source files. By submitting the patch, you are implicitly transfering to the JBehave project the ownership of the copyright, so the copyright notice should be removed (the @author tags could and should stay). Even better if you explicitly waived any claim of copyright from Yell or personally from the authors. Thanks. We'll review you're comments and get back to you.
But in the meantime, your website points to svn for the latest source code:
http://jbehave.org/download/
states:
"The very latest code is also available from the source code repository."
which points to:
http://jbehave.org/documentation/source-repository/
which uses SVN:
svn co http://svn.codehaus.org/jbehave/trunk jbehave
Mike Neeve
added a comment - Thanks. We'll review you're comments and get back to you.
But in the meantime, your website points to svn for the latest source code:
http://jbehave.org/download/
states:
"The very latest code is also available from the source code repository."
which points to:
http://jbehave.org/documentation/source-repository/
which uses SVN:
svn co http://svn.codehaus.org/jbehave/trunk jbehave
Hi Mike,
svn is still the repo used for the last stable release 2.5.x but git is the repo for 3.x (still in RC, so not yet a stable release). But not to worry, if it's a burden to use git, do provide svn patch and we'll convert it to 3.x and commit to git repo. We don't want this to be a showstopper for a very interesting contribution.
Thanks again.
Mauro Talevi
added a comment - Hi Mike,
svn is still the repo used for the last stable release 2.5.x but git is the repo for 3.x (still in RC, so not yet a stable release). But not to worry, if it's a burden to use git, do provide svn patch and we'll convert it to 3.x and commit to git repo. We don't want this to be a showstopper for a very interesting contribution.
Thanks again. Updated page to provide links to source repos for both 2.x and 3.x:
Mauro Talevi
added a comment - Updated page to provide links to source repos for both 2.x and 3.x:
http://jbehave.org/documentation/source-repository/ Mauro Talevi
made changes -
| Field | Original Value | New Value |
|---|---|---|
| Fix Version/s | 3.2 [ 16757 ] | |
| Fix Version/s | 3.1 [ 16511 ] |
Mauro Talevi
made changes -
| Fix Version/s | 3.x [ 16979 ] | |
| Fix Version/s | 3.2 [ 16757 ] |
I'm curious about the use cases for a visual component (JFace) and a non-visual function (parsing).
This does more than just parsing? I guess I don't fully understand this.
We should probably create a new module for this rather than jbehave-core for dependency management.