Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ Please visit [cucumber/CONTRIBUTING.md](https://github.com/cucumber/cucumber/blo

## [Unreleased]

### Changed
- Change to use worst Test Step result as the Test Case result
([#317](https://github.com/cucumber/cucumber-ruby-core/pull/317))

## [16.2.0] - 2026-02-06
### Changed
- Added the test result type 'ambiguous'
Expand Down
2 changes: 1 addition & 1 deletion lib/cucumber/core/test/result.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ module Cucumber
module Core
module Test
module Result
TYPES = %i[failed ambiguous flaky skipped undefined pending passed unknown].freeze
TYPES = %i[failed ambiguous flaky undefined pending skipped passed unknown].freeze
Comment thread
luke-hill marked this conversation as resolved.
STRICT_AFFECTED_TYPES = %i[flaky undefined pending].freeze

def self.ok?(type, strict: StrictConfiguration.new)
Expand Down
44 changes: 23 additions & 21 deletions lib/cucumber/core/test/runner.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# frozen_string_literal: true

require 'cucumber/core/test/timer'
require 'cucumber/messages/helpers/test_step_result_comparator'

module Cucumber
module Core
Expand Down Expand Up @@ -45,6 +46,8 @@ def done
end

class RunningTestCase
include Cucumber::Messages::Helpers::TestStepResultComparator

def initialize
@timer = Timer.new.start
@status = Status::Unknown.new(Result::Unknown.new)
Expand All @@ -59,27 +62,27 @@ def result
end

def failed(step_result)
@status = Status::Failing.new(step_result)
not_passing(step_result)
self
end

def ambiguous(step_result)
@status = Status::Ambiguous.new(step_result)
failed(step_result)
self
end

def passed(step_result)
@status = Status::Passing.new(step_result)
@status = Status::Passing.new(step_result) if test_step_result_rankings[step_result.to_message.status] > test_step_result_rankings[status.step_result_message.status]
self
end

def pending(_message, step_result)
@status = Status::Pending.new(step_result)
failed(step_result)
self
end

def skipped(step_result)
@status = Status::Skipping.new(step_result)
failed(step_result)
self
end

Expand All @@ -96,6 +99,13 @@ def duration(_step_duration, _step_result)
self
end

private

def not_passing(step_result)
@status = Status::NotPassing.new(step_result) if test_step_result_rankings[step_result.to_message.status] > test_step_result_rankings[status.step_result_message.status]
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.

I can see we have status defined as an iVar and I assume it's defined somewhere in here, but I can't see it.

self
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.

I'm not sure this self call is needed here, as the not_passing invocation returns self afterwards

end

attr_reader :status
private :status

Expand All @@ -118,6 +128,10 @@ def execute(test_step, monitor, &)
def result
raise NoMethodError, 'Override me'
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.

I cannot see where result (Which is defined in the two child classes - Passing and NotPassing will use the result defined there.

end

def step_result_message
step_result.to_message
end
end

class Unknown < Base
Expand All @@ -132,26 +146,14 @@ def result(duration)
end
end

class Failing < Base
class NotPassing < Base
def execute(test_step, monitor)
result = test_step.skip(monitor.result)
if result.undefined?
result = result.with_message(%(Undefined step: "#{test_step.text}"))
result = result.with_appended_backtrace(test_step)
end
result
end

def result(duration)
step_result.with_duration(duration)
result = result.with_message(%(Undefined step: "#{test_step.text}")) if result.undefined?
Comment thread
luke-hill marked this conversation as resolved.
result = result.with_appended_backtrace(test_step) unless test_step.hook?
result.describe_to(monitor, result)
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.

Is this the bit that does the magic stuff? I know these methods are a bit obscure for me

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The important here is that the only difference between NotPassing.execute and Base.execute is whether skip or execute is called on the test_step. Previously this method was written under the assumption that when the runner has entered "skip mode" the only possible results are skipped and undefined. This is not true. Hooks can result in passing or failing results. Ambiguous steps result in ambiguous results. pending results is very unlikely be who knows, someone might write a after hook the raises a Pending exception.

After calling skip on the test_step, NotPassing.execute shall be the same as Base.execute, that is:

  • Add a message with the step text to undefined results
  • Add any backtrace from the test_step to the result unless it is a hook
  • Pass is (let it describe itself to the RunningTestCase so it can update the result for the test case as a whole if applicable. The line result.describe_to(monitor, result) will in the end either run RunningTestCase.passed or RunningTestCase.not_passing (depending what type of result that have occurred so far).

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.

I assume this "repeated" rewriting / over assignment is possible because a lot of the describe and other methods are returning self

end
end

Pending = Class.new(Failing)

Ambiguous = Class.new(Failing)

class Skipping < Failing
def result(duration)
step_result.with_duration(duration)
end
Expand Down
99 changes: 99 additions & 0 deletions spec/cucumber/core/test/runner_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,105 @@
test_case.describe_to(runner)
end
end

context 'with an initial undefined step' do
let(:test_steps) { [undefined_step, passing_step] }

it 'emits a test_step_finished event with an undefined result' do
expect(event_bus).to receive(:test_step_finished).with(undefined_step, anything) do |_reported_test_case, result|
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.

Where is anything defined.

Or are we simply saying here it's irrelevant what it receives. Wouldn't it be a passing step due to line 255?

expect(result).to be_undefined
end
test_case.describe_to(runner)
end

it 'emits a test_step_finished event with a skipped result' do
expect(event_bus).to receive(:test_step_finished).with(passing_step, anything) do |_reported_test_case, result|
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.

Shouldn't all these shadowing wrapper methods be allow just to permit it through. Won't / Shouldn't we be doing diff assertions based on this not appearing?

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 think this actually should be an ordered set of results. Maybe like here? https://github.com/site-prism/site_prism/blob/v2.15.1/spec/sections_spec.rb#L50-L55

expect(result).to be_skipped
end
test_case.describe_to(runner)
end

it 'emits a test_case_finished event with an undefined result' do
allow(event_bus).to receive(:test_case_finished) do |_reported_test_case, result|
expect(result).to be_undefined
expect(result.exception).to be_a StandardError
end
test_case.describe_to(runner)
end

it 'skips, rather than executing the second step' do
expect(passing_step).not_to receive(:execute)

allow(passing_step).to receive(:skip).and_return(Cucumber::Core::Test::Result::Skipped.new)
test_case.describe_to(runner)
end

context 'with a following ambiguous step' do
let(:test_steps) { [undefined_step, ambiguous_step] }

it 'emits a test_step_finished event with an undefined result' do
expect(event_bus).to receive(:test_step_finished).with(undefined_step, anything) do |_reported_test_case, result|
expect(result).to be_undefined
end
test_case.describe_to(runner)
end

it 'emits a test_step_finished event with a ambiguous result' do
expect(event_bus).to receive(:test_step_finished).with(ambiguous_step, anything) do |_reported_test_case, result|
expect(result).to be_ambiguous
end
test_case.describe_to(runner)
end

it 'emits a test_case_finished event with an ambiguous result' do
allow(event_bus).to receive(:test_case_finished) do |_reported_test_case, result|
expect(result).to be_ambiguous
expect(result.exception).to be_a StandardError
end
test_case.describe_to(runner)
end

it 'skips, rather than executing the second step' do
expect(ambiguous_step).not_to receive(:execute)

allow(ambiguous_step).to receive(:skip).and_return(Cucumber::Core::Test::Result::Ambiguous.new)
test_case.describe_to(runner)
end
end

context 'with a failing after hook' do
let(:test_steps) { [undefined_step, failing_hook] }

it 'emits a test_step_finished event with an undefined result' do
expect(event_bus).to receive(:test_step_finished).with(undefined_step, anything) do |_reported_test_case, result|
expect(result).to be_undefined
end
test_case.describe_to(runner)
end

it 'emits a test_step_finished event with a failing result' do
expect(event_bus).to receive(:test_step_finished).with(failing_hook, anything) do |_reported_test_case, result|
expect(result).to be_failed
end
test_case.describe_to(runner)
end

it 'emits a test_case_finished event with an failing result' do
allow(event_bus).to receive(:test_case_finished) do |_reported_test_case, result|
expect(result).to be_failed
expect(result.exception).to be_a StandardError
end
test_case.describe_to(runner)
end

it 'call skip, rather than execute on test step of the hook' do
expect(failing_hook).not_to receive(:execute)

allow(failing_hook).to receive(:skip).and_return(Cucumber::Core::Test::Result::Failed.new(Cucumber::Core::Test::Result::UnknownDuration.new, instance_double(StandardError, backtrace: [])))
test_case.describe_to(runner)
end
end
end
end

context 'with multiple test cases' do
Expand Down
2 changes: 2 additions & 0 deletions spec/support/shared_context/test_step_types.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,6 @@
let(:pending_step) { Cucumber::Core::Test::Step.new(3, 'Pending Step', double).with_action { raise Cucumber::Core::Test::Result::Pending, 'TODO' } }
let(:skipping_step) { Cucumber::Core::Test::Step.new(4, 'Skipped Step', double).with_action { raise Cucumber::Core::Test::Result::Skipped } }
let(:undefined_step) { Cucumber::Core::Test::Step.new(5, 'Undefined Step', double) }
let(:ambiguous_step) { Cucumber::Core::Test::Step.new(6, 'Ambiguous Step', double).with_unskippable_action { raise Cucumber::Core::Test::Result::Ambiguous } }
let(:failing_hook) { Cucumber::Core::Test::Step.new(7, 'Failing Step', double).with_unskippable_action { raise StandardError, 'Error' } }
end
Loading