Change to use worst Test Step result as the Test Case result

[brasmusson]

Description

Change to use worst Test Step result as the Test Case result

When calculating the Test Case result Cucumber-Ruby-Core should conform with the GetWorstTestCaseResult function used in the Messages module.

Type of change

Please delete options that are not relevant.

• New feature (non-breaking change which adds new behaviour)

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

[luke-hill]

Haven't got round to fully reviewing but we have this concept in messages
https://github.com/cucumber/messages/blob/main/ruby/lib/cucumber/messages/helpers/test_step_result_comparator.rb

[brasmusson]

@luke-hill Yes test_step_result_rankings from Messages can be used. It does raises the question about 'Cucumber::Core::Test::Result::TYPES`, its order control the order in the summary. Shouldn't that order conform to the ranking in Messages (which it does after the changes in this PR).

[luke-hill]

@luke-hill Yes test_step_result_rankings from Messages can be used. It does raises the question about 'Cucumber::Core::Test::Result::TYPES`, its order control the order in the summary. Shouldn't that order conform to the ranking in Messages (which it does after the changes in this PR).

I think it does, plus the flaky bit. So I don't mind which way we do things tbh.

There's lots of refactors I want to do once cucumber-ruby v11 is released (Just waiting on fixing up last CCK bits).

[luke-hill]

Can I ask that without this change what is currently happening / going wrong.

Also should this be included as a minimum for cucumber v11? I'm soon about to cut it and want to get everything sewn up at the same time ideally

[brasmusson]

Can I ask that without this change what is currently happening / going wrong.

Also should this be included as a minimum for cucumber v11? I'm soon about to cut it and want to get everything sewn up at the same time ideally

Without this if a scenario has only an undefined step and an after hook fails, the summary report will say

1 scenario (1 undefined)
1 step (1 undefined)

instead of the expected:

1 scenario (1 failed)
1 step (1 undefined)

Without this if a scenario has one undefined step (or pending) followed by an ambiguous step, the summary report will say

1 scenario (1 undefined)
2 steps (1 ambiguous, 1 undefined)

instead of the expected:

1 scenario (1 ambiguous)
2 steps (1 ambiguous, 1 undefined)

[luke-hill]

Ooooh ok. This might actually help me then with some of the work I need to do for messages and @davidjgoss will hopefully be able to help with this.

We have 3 CCK examples currently failing, two of which are regarding hooks failing.

Also are these only specific to the summary report. Or any report for that matter (I'm thinking of messages predominantly)

Also what would happen if it were Before Hooks that failed. Or BeforeAll hooks

[brasmusson]

It is the result field in the internal TestCaseFinished event that gets the wrong value. Any formatter or filter that reads and uses that field of the TestCaseFinished event will be wrong. It is easy to point out the summary, because there the result field of TestCaseFinished events are used for sure. I have not looked how may formatters actually use that field. Formatters using messages are not affected.

[luke-hill]

Reviewed 3 and a bit files out of the 5. Got a few questions, but I get what is being done here.

[luke-hill]

For speed I'm just going to approve these