Repository navigation
Let a retry policy decide will_be_retried in the runner #345
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
66f3a62
52656e0
9ab0226
f398b79
6944446
ecd3dd9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,11 +14,13 @@ class Runner | |
| attr_reader :event_bus, :running_test_case, :running_test_step, :id_generator | ||
| private :event_bus, :running_test_case, :running_test_step, :id_generator | ||
|
|
||
| def initialize(event_bus, id_generator = Cucumber::Messages::Helpers::IdGenerator::UUID.new, backtrace_filter = nil, max_attempts = 1) | ||
| # @param retry_policy [#will_be_retried?, nil] asked, once a test case has finished, whether it is going to be | ||
| # run again. It receives the test case and its result. When nil, no test case is ever reported as retried. | ||
| def initialize(event_bus, id_generator: Cucumber::Messages::Helpers::IdGenerator::UUID.new, backtrace_filter: nil, retry_policy: nil) | ||
| @event_bus = event_bus | ||
| @id_generator = id_generator | ||
| @backtrace_filter = backtrace_filter | ||
| @max_attempts = max_attempts | ||
| @retry_policy = retry_policy | ||
| @current_test_case = nil | ||
| end | ||
|
|
||
|
|
@@ -34,8 +36,8 @@ def test_case(test_case, &descend) | |
| descend.call(self) | ||
|
|
||
| result = calculate_test_case_result(test_case) | ||
| event_bus.envelope(to_test_case_finished_envelope(test_case, result)) | ||
| event_bus.test_case_finished(test_case, result) | ||
| event_bus.envelope(to_test_case_finished_envelope(result)) | ||
| self | ||
| end | ||
|
|
||
|
|
@@ -82,12 +84,12 @@ def to_test_case_started_envelope(test_case) | |
| ) | ||
| end | ||
|
|
||
| def to_test_case_finished_envelope(result) | ||
| def to_test_case_finished_envelope(test_case, result) | ||
| Cucumber::Messages::Envelope.new( | ||
| test_case_finished: Cucumber::Messages::TestCaseFinished.new( | ||
| test_case_started_id: @current_test_case_started_id, | ||
| timestamp: time_to_timestamp(Time.now), | ||
| will_be_retried: result.failed? && (@attempt < @max_attempts) | ||
| will_be_retried: @retry_policy&.will_be_retried?(test_case, result) || false | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of having this nil safe check and an alternation, can we instead have the retry policy always instantiated and return a value?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in f398b79: the runner now defaults to a |
||
| ) | ||
| ) | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| # Upgrading to cucumber-core 20.0.0 | ||
|
|
||
| ## Retry policy for the Test Runner | ||
|
|
||
| In cucumber-core 19.0.0 the `Cucumber::Core::Test::Runner` started to emit the `TestCaseFinished` envelope itself, and | ||
| decided the value of its `will_be_retried` field from a maximum number of attempts passed to its constructor. The | ||
| runner cannot know that on its own: whether a test case is retried is decided by whoever runs it again (in | ||
| cucumber-ruby, the retry filter, which also honours `--retry-total`), so the two got out of sync. | ||
|
|
||
| The runner now takes a retry policy instead, and its optional constructor arguments (`id_generator`, `backtrace_filter` | ||
| and `retry_policy`) became keyword arguments. The policy is asked, every time a test case finishes, whether that test | ||
| case will be run again. It has to answer without side effects, as it is asked before the | ||
| `test_case_finished` event is broadcast. | ||
|
|
||
| ### Before cucumber-core 20.0.0 | ||
|
|
||
| ```ruby | ||
| runner = Cucumber::Core::Test::Runner.new(event_bus, id_generator, backtrace_filter, max_attempts) | ||
| ``` | ||
|
|
||
| ### With cucumber-core 20.0.0 | ||
|
|
||
| ```ruby | ||
| class RetryPolicy | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lets namespace this just to cover ourselves as it's completely new Maybe with a comment for the file path ref that is the default one
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 6944446: the example is now |
||
| def will_be_retried?(test_case, result) | ||
| result.failed? && attempts_so_far(test_case) < max_attempts | ||
| end | ||
| end | ||
|
|
||
| runner = Cucumber::Core::Test::Runner.new( | ||
| event_bus, | ||
| id_generator: id_generator, | ||
| backtrace_filter: backtrace_filter, | ||
| retry_policy: RetryPolicy.new | ||
| ) | ||
| ``` | ||
|
|
||
| When no retry policy is given, no test case is reported as retried. `Cucumber::Core::Test::Runner.new(event_bus)` keeps | ||
| working as before. | ||
|
|
||
| ## Envelope ordering | ||
|
|
||
| The runner now emits the `TestCaseFinished` envelope before it broadcasts the `test_case_finished` event. Handlers of | ||
| that event that run the test case again therefore see the envelope of the previous attempt already emitted. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather have the retry policy not be something passed in as an extraneous class. But instead something that is forcibly set based on configuration.
So I envisage something like
@configuration.retry_policy.will_be_retried?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Just to make sure I understand you correctly: core has no configuration object,
Configurationlives in cucumber-ruby. Did you mean the policy built there (Configuration#retry_policy, as in your comment on cucumber/cucumber-ruby#1910) and handed to the runner, as it is now? Or should I introduce a configuration in core that the runner reads the policy from?