Rewrite the JUnit Formatter to the new formatter Api #855
Conversation
The current implementation of the JUnit Formatter does not handle several failing scenarios form the same Examples table correctly. Nor does it handle undefined or pending scenarios properly in strict mode.
| @builder = Builder::XmlMarkup.new( :indent => 2 ) | ||
| @time = 0 | ||
| def before_test_case(test_case) | ||
| if not same_feature_as_previous_test_case?(test_case.feature) |
There was a problem hiding this comment.
Link to style-guide on not vs !: https://github.com/bbatsov/ruby-style-guide#bang-not-not
Another thing: How to use Ruby's English and/or operators without going nuts It would be great if you could examine all of the other new usages of and and or, to see whether they should not rather be the conventional && and ||.
(By the way: SUPER awesome that you update the JUnit formatter!)
There was a problem hiding this comment.
Shouldn't unless be as good as if !?
There was a problem hiding this comment.
Yes, you're right, in this case there is no else, and we are free to improve one step more.
(By preferring unless over if ! and by not using not you now fixed two things.)
There was a problem hiding this comment.
In order to learn more about code conventions, you can use Rubocop on the files you've changed, and get a detailed report on any code-style deviations. The repo as a whole does not follow the above-mentioned style guide, but lots of Ruby idiom can be gleaned from reading the tool's output. (How much does Cucumber code-base not follow rubocop in all files?: "327 files inspected, 6849 offenses detected")
There was a problem hiding this comment.
I reworked the commit and fixed all conditionals related Rubocop detected issues. I did not fix everything Rubocop remarked on. I usually think "evolution, not revolution", and when the code base generally doesn't seem to follow the "no lines longer than 80 characters", I do not go over the top to only write lines shorter than 80 characters either (for example).
Since Cucumber is Scenario/Test Case oriented it makes more sense to put the intercepted stdout and stderr there, instead in the testsuite element (that represents the Feature). Stdout and stderr elements are already created for the testcase elements, but they have so far always been empty.
brasmusson
force-pushed
the
junit-formatter-new-api
branch
3 times, most recently
from
2d55075
to
085c58f
Compare
brasmusson
force-pushed
the
junit-formatter-new-api
branch
from
c546ed9
to
4f3ebda
Compare
Rewrite the JUnit Formatter to the new formatter Api
|
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. |
The rewrite does also automatically fixed some problems regarding Scenario Outline test cases. Multiple failing test cases from the same Examples table, and pending and undefined test cases from Scenario Outlines in strict mode, have not been handled properly.
In addition the intercepted stdout and stderr data are moved from the testsuite elements (which representing Features) to the testcase element (fd5e38e). Stdout and stderr elements are already created (but empty) for testcase elements. It makes more sense to put the intercepted stdout and stderr in the testcase, and having empty stdout and stderr elements there (which stated "nothing was intercepted") seems to misinformation is anything.
The final change (4f3ebda) requires cucumber/cucumber-ruby-core#92, I think a Result#non_zero_exit_status? query can simplify logic not only in the JUnit Formatter, but also in other parts of the Cucumber code base.
Related to #839.