fix: Include consistent index & placeholders for Example names in junit #1682
Conversation
f22bd23 to
f8c8bad
Compare
Previously, when a feature file contained scenarios with `Example` nodes the junit report would append an index to each testcase name to differentiate them (e.g. `<testcase name="Some scenario outline #1"`, `<testcase name="Some scenario outline Behat#2`, etc). These indices were dynamically generated based on the examples that actually ran. As a result, on a `--rerun` the names would start from `#1` in sequence and be inconsistent with the numbering of the first execution. This makes it difficult to compare reports between each test run, for example to link a passing rerun to the previous failure. With this change, examples are numbered consistently based on the order they appear in the feature file. Additionally, we also now support placeholders in the Scenario Outline title. This allows users to include some or all of the values from the Examples: table in each `<testcase` name for additional clarity. This matches the behaviour of the official cucumber runners. This PR builds on work done by @uuf6429 and in particular the implementation of the numbering & placeholder replacement that he contributed to Behat/Gherkin.
f8c8bad to
32074a9
Compare
| }, | ||
| { | ||
| "name": "Passed & Failed #1", | ||
| "name": "Passed & Failed with value=5 #1", |
There was a problem hiding this comment.
There are no changes to the JSON implementation, but I have added placeholders to the fixture which is shared between the JSON and junit formatters.
| <failure message="Then I must have 13: Failed asserting that 14 matches expected '13'."/> | ||
| </testcase> | ||
| <testcase name="Passed & Failed #1" classname="Adding numbers" status="failed" time="-IGNORE-VALUE-" file="features-DIRECTORY-SEPARATOR-single_feature.feature" line="29"> | ||
| <testcase name="Passed & Failed with value=5 #1" classname="Adding numbers" status="failed" time="-IGNORE-VALUE-" file="features-DIRECTORY-SEPARATOR-single_feature.feature" line="29"> |
There was a problem hiding this comment.
I have not explicitly added a test to prove that the #1, #2 etc are in fact consistent between runs.
However, we're proving the placeholders appear in the output, and we know that comes from the implementation in Behat/Gherkin that also appends the index as it parses the gherkin file - and that is tested in that project. I think that provides sufficient confidence that the rerun will now be numbered correclty.
| paths: | ||
| - src/Behat/Behat/Definition/Printer/ConsoleDefinitionPrinter.php | ||
| - src/Behat/Behat/EventDispatcher/Cli/StopOnFailureController.php | ||
| - src/Behat/Behat/Output/Node/Printer/JUnit/JUnitScenarioPrinter.php |
There was a problem hiding this comment.
The constructor now has an unused param that we need to keep for BC
| /** | ||
| * @var array | ||
| * | ||
| * @deprecated This will be removed in the next major as the JUnit formatter no longer uses this information |
There was a problem hiding this comment.
Do we really need to deprecate private properties/methods? I would say we just need to deprecate public/protected ones
There was a problem hiding this comment.
private methods can be deleted directly, as they cannot be used from the outside.
There was a problem hiding this comment.
Good point @carlos-granados I got trigger happy marking things to remind us to delete them. It was partly to record that I had reviewed for any other uses or side-effects of that behaviour and confirmed it will be safe to delete once the public ->getCurrentOutline() is gone. I felt it also marked that we're expecting to remove these, so discourages anyone from adding any new (internal) feature that uses them.
@stof I deprecated rather than deleted the private method because we need to keep it as-is for now - it collects the state that is later used by the (deprecated and now unused by us) ->getCurrentOutline() public method.
If the comments make the diff / codebase too noisy then I can happily remove them and we can just work out what to delete if/when we get to the point of deleting the ->getCurrentOutline() method.
What do you think?
There was a problem hiding this comment.
I still feel we should only deprecate the public interface, I believe that should be enough to see what needs to be removed
There was a problem hiding this comment.
No problem, done now.
carlos-granados
left a comment
There was a problem hiding this comment.
Looks great, good job @acoulton just a question
carlos-granados
left a comment
There was a problem hiding this comment.
Great stuff, thanks @acoulton
Previously, when a feature file contained scenarios with
Examplenodes the junit report would append an index to each testcase name to differentiate them (e.g.<testcase name="Some scenario outline #1",<testcase name="Some scenario outline #2, etc).These indices were dynamically generated based on the examples that actually ran. As a result, on a
--rerunthe names would start from#1in sequence and be inconsistent with the numbering of the first execution. This makes it difficult to compare reports between each test run, for example to link a passing rerun to the previous failure.With this change, examples are numbered consistently based on the order they appear in the feature file. This fixes #1448.
Additionally, we also now support placeholders in the Scenario Outline title. This allows users to include some or all of the values from the Examples: table in each
<testcasename for additional clarity. This matches the behaviour of the official cucumber runners.This PR builds on work done by @uuf6429 in #1459 and in particular the implementation of the numbering & placeholder replacement that he contributed in Behat/Gherkin#270 and Behat/Gherkin#271.