Repository navigation
Let a retry policy decide will_be_retried in the runner - #345
Conversation
19900a2 to
d98745a
Compare
The runner emits the TestCaseFinished envelope after broadcasting the test_case_finished event. The retry filter in cucumber-ruby re-runs the test case from inside that event, so the envelope of the first attempt is emitted after the whole retry and carries the test_case_started id of the retry instead of its own. The runner also derives will_be_retried from a maximum number of attempts, which it cannot know: whether a test case runs again is decided by whoever runs it again, and the retry filter stops retrying once --retry-total is exhausted. The runner has to ask a retry policy. These specs fail until the runner emits the envelope before the event and asks a retry policy. Related to cucumber/cucumber-ruby#1905.
d98745a to
3419704
Compare
The runner computed the will_be_retried field of the TestCaseFinished envelope from a maximum number of attempts. It cannot know that on its own: whether a test case runs again is decided by whoever runs it again, which in cucumber-ruby is the retry filter honouring both --retry and --retry-total. The runner now asks a retry policy instead. The envelope is also emitted before the test_case_finished event. The retry filter re-runs the test case from inside that event, so the envelope of the first attempt used to be emitted after the retry, with the test_case_started id of the retry. Related to cucumber/cucumber-ruby#1905.
3419704 to
52656e0
Compare
|
Heya. Just posting this once @Enceradeira as I know you've done work across both repos. I've been away for a bit and will likely not be involved much with this in the coming week or so. Thankyou for your contribution. We have a call on thursdays (Although I'm not there this thursday), so I will try to get to this ASAP (But it could be a while). Rest assured it's not forgotten |
The retry policy was the fourth positional argument of the runner, so passing one meant spelling out the id generator and backtrace filter defaults as well. id_generator, backtrace_filter and retry_policy are now keyword arguments; Runner.new(event_bus) keeps working as before. Related to cucumber/cucumber-ruby#1910 (comment)
2d7df3f to
9ab0226
Compare
luke-hill
left a comment
There was a problem hiding this comment.
Added some thoughts whilst I'm around for now. Will be away for another week or so.
| 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 |
There was a problem hiding this comment.
Instead of having this nil safe check and an alternation, can we instead have the retry policy always instantiated and return a value?
There was a problem hiding this comment.
Done in f398b79: the runner now defaults to a NoRetries policy, so the nil check is gone.
| ### With cucumber-core 20.0.0 | ||
|
|
||
| ```ruby | ||
| class RetryPolicy |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Done in 6944446: the example is now MyProject::RetryPolicy, with a comment pointing to the cucumber-ruby implementation.
| 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) |
There was a problem hiding this comment.
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.
Thanks! Just to make sure I understand you correctly: core has no configuration object, Configuration lives 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?
The runner guarded the retry policy against nil. It now defaults to NoRetries, so it always has a policy to ask. Related to #345 (comment)
luke-hill
left a comment
There was a problem hiding this comment.
Reviewed 3/5 (mini bits), will get to the rest soon.
Again thankyou for your work, sorry for delays
luke-hill
left a comment
There was a problem hiding this comment.
One remaining request for an additional test. Then this is good and RtM
|
Thankyou for all your hard work @Enceradeira This is RtM. I'll work on cutting a release for core soon. Probably in a couple of days |
Description
Alternative to #344, fixing cucumber/cucumber-ruby#1905 together with cucumber/cucumber-ruby#1910.
Since 19.0.0 the
Runneremits theTestCaseFinishedenvelope and decides itswill_be_retriedfield from a maximum number of attempts. The runner cannot know that on its own: whether a test case runs again is decided by whoever runs it again, which in cucumber-ruby is the retry filter honouring both--retryand--retry-total. #344 copies the--retry-totalcounter into the runner, which makes the reported symptom go away but keeps the retry decision in two places, and leaves two further defects in the message stream:--retry 1the flag is nevertrue, so a retried attempt is reported as a final result and the HTML report shows the scenario twice. With--retry 2the second attempt is reported as final although a third one follows.test_case_finishedevent before emitting theTestCaseFinishedenvelope. The retry filter re-runs the test case from inside that event, so the envelope of the first attempt is emitted after the retry and carries thetest_case_started_idof the retry. Onmain, a retried scenario produces twoTestCaseFinishedenvelopes for attempt 2 and none for attempt 1.The compatibility kit does not catch any of this because it only compares message keys, not values.
This PR:
max_attemptsconstructor argument with aretry_policy, an object answeringwill_be_retried?(test_case, result). With no policy, no test case is reported as retried.TestCaseFinishedenvelope before broadcasting thetest_case_finishedevent, so a retry started from that event no longer reorders the stream.Type of change
Cucumber::Core::Test::Runner), seeupgrading_notes/20.0.0.mdChecklist:
bundle exec rubocopreports no offenses