Skip to content

Handle the retry_total_tests configuration option in the runner - #344

Open
brasmusson wants to merge 1 commit into
mainfrom
feature/max-retry-fix
Open

Handle the retry_total_tests configuration option in the runner#344
brasmusson wants to merge 1 commit into
mainfrom
feature/max-retry-fix

Conversation

@brasmusson

Copy link
Copy Markdown
Contributor

Description

The retry filter both take into account the retry_attempts and the retry_total_tests configuration options, therefore the runner will have to do the same when calculating the will_be_retried field in the test_case_finished envelopes.

Now that the runner does not take the retry_total_tests configuration option, test_case_finished envelopes can state that the test case will be retried, but the retry filter will not retry the test case. This confuses that HTML formatter which only displays result for test cases which has a test case finished envelope with will_be_retried=false (that is the final try for that test case).

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

Please add an entry to the relevant section of CHANGELOG.md as part of this pull request.

Note to other contributors

If your change may impact future contributors, explain it here, and remember to update README.md and CONTRIBUTING.md accordingly.

Checklist:

Your PR is ready for review once the following checklist is
complete. You can also add some checks if you want to.

  • Tests have been added for any changes to behaviour of the code
  • New and existing tests are passing locally and on CI
  • bundle exec rubocop reports no offenses
  • CHANGELOG.md has been updated

* The retry filter both take into account the retry_attempts and the
  retry_total_tests configuration options, therefore the runner will
  have to do the same when calculating the will_be_retried field in
  the test_case_finished envelopes.

@luke-hill luke-hill left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work finding this out

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)
def initialize(event_bus, id_generator = Cucumber::Messages::Helpers::IdGenerator::UUID.new, backtrace_filter = nil, max_attempts = 1, max_total_retried_tests = Float::INFINITY)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this lean on the config option rather than the verbose value. As this value is a default for the config option (Question btw)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see now, this "will" lean on the config value specified.

@luke-hill

Copy link
Copy Markdown
Contributor

@brasmusson i'm going to be off for a week now.

Can you just ensure that appropriate changelogs are written. But feel free to merge both as/when appropriate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants