Use deterministic unique ids in Descriptions #1121
Conversation
Rerunning failed tests with Surefire requires a method to identify the tests. Surefire currently uses a tuple of (Class, Method) to identify failed tests. Cucumber-jvm uses test suites which do not consist of Classes or methods. Therefore another method is needed. JUnit provides the option to select tests that match a Description. Descriptions are compared using a unique Id. To identify tests between different runs we should provide Descriptions with a predictable unique id. The current implementation did not suffice. While the provided cucumber elements are unique, they are not predictable between runs making it impossible to use their descriptions to rerun a failed test. After this change it will be possible to update Surefire to use descriptions to rerun failed tests. Related Issues: - https://issues.apache.org/jira/browse/SUREFIRE-1372 - #1120 - temyers/cucumber-jvm-parallel-plugin#31
|
Build failure was present since 8864972. Does not appear to be related to my changes. |
| } | ||
|
|
||
| Serializable next() { | ||
| return new Id(base, id++); |
There was a problem hiding this comment.
Using a sequence counter as a base for the id-object makes them unpredictable. As I understand it using
cucumber/examples/java/calculator/basic_arithmetic.feature:7:12
and
cucumber/examples/java/calculator/basic_arithmetic.feature:12
would result in different ids for the test case from the scenario on line 12 (feature file from the java-calculator example in the repo).
The id should instead be base on the uri(/path) of the feature file and the line number of the feature/scenario/etc.
There was a problem hiding this comment.
~~~For example:~~~
```feature
Feature: Basic Arithmetic
Background: A Calculator
Given a calculator I just turned on
Scenario: Addition
When I add 4 and 5
Then the result is 9
Scenario: Another Addition
When I add 4 and 7
Then the result is 11
```
~~~Results in a tree of executors that looks like this:~~~
```
FeatureRunner => Feature: Basic Arithmetic
|- ScenarioRunner => Scenario: Addition
| |- ExecutionUnitRunner =>
| |- Background step: Given a calculator I just turned on
| |- When I add 4 and 5
| |- Then the result is 9
|- ScenarioRunner => Scenario: Another Addition
|- ExecutionUnitRunner =>
|- Background step: Given a calculator I just turned on
|- When I add 4 and 7
|- Then the result is 11
```
~~~This tree can then be numbered in depth first order:~~~
```
0. FeatureRunner => Feature: Basic Arithmetic
1. |- ScenarioRunner => Scenario: Addition
2. | |- ExecutionUnitRunner =>
2. | |- Background step: Given a calculator I just turned on
4. | |- When I add 4 and 5
5. | |- Then the result is 9
6. |- ScenarioRunner => Scenario: Another Addition
7. |- ExecutionUnitRunner =>
8. |- Background step: Given a calculator I just turned on
9. |- When I add 4 and 7
10. |- Then the result is 11
```
~~~Which means that for a given feature file creating the numbering of descriptions is entirely predictable. So I guess I don't quite understand your comment.~~~
~~~> The id should instead be base on the uri(/path) of the feature file and the line number of the feature/scenario/etc.~~~
~~~Using the feature uri and the line number alone is not feasible as background steps are repeated. A failing background step in the execution of Scenario `Another Addition` would result in both `Another Addition` and `Addition` being rerun. Their descriptions both contain a child Description that uses line 3 .~~~
~~~It would be possible to use the uri and line number provided we also used some other other identifier to disambiguate between background steps in different scenarios. So as far as I can see numbering the tree by depth first order is sufficient and simple.~~~
There was a problem hiding this comment.
After reviewing the changes in Gherkin v4 I've changed my opinion. It is indeed much better to use a line based identification.
- FeatureRunner: feature-uri
- ExecutionRunner: feature-uri:scenario-line
- Step: feature-uri:scenario-line:step-line
|
Maybe it is an idea to add a |
|
That would certainly fix surefire reporting the steps of a scenario as tests and some other reporting oddities. Doesn't really impact the rerunning failing tests though. Rerunning is done by creating a tree of suites with the tests as leaves. Any tests that don't need to be rerun (e.g. that don't match the description of the failed tests) are removed. Any suites that have become empty are also removed. Then the whole tree is executed again. Because the ExecutionUnitRunner overrides the execute method the whole scenario is executed and things work as expected. Adding |
|
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. |
Summary
Use deterministic unique ids in Descriptions.
Motivation and Context
Rerunning failed tests with Surefire requires a method to identify the tests.
Surefire currently uses a tuple of (Class, Method) to identify failed tests.
Cucumber-jvm uses test suites which do not consist of Classes or methods.
Therefore another method is needed.
JUnit provides the option to select tests that match a Description.
Descriptions are compared using a unique Id. To identify tests between
different runs we should provide Descriptions with a predictable unique
id.
The current implementation did not suffice. While the provided cucumber
elements are unique, they are not predictable between runs making it
impossible to use their descriptions to rerun a failed test.
After this change it will be possible to update Surefire to use
descriptions to rerun failed tests.
Related Issues:
How Has This Been Tested?
Replaced
ExecutionUnitRunnerTest#shouldPopulateRunnerStepsWithStepsUsedInStepDescriptionswithFeatureRunnerTest#shouldPopulateDescriptionsWithStableUniqueIdsthat creates the same feature twice and tests the id's are unique and predictable.The reproducer for Surefire can be found in my fork of Surefire. Running with cucumber 1.2.5 results in failed tests, with the result of this PR as 1.2.6-SNAPSHOT results in 2 flakes.
Types of changes
Checklist: