Skip to content

Commit

Permalink
Spike a fix in the retry filter.
Browse files Browse the repository at this point in the history
  • Loading branch information
brasmusson committed Mar 20, 2016
1 parent 8d87340 commit 8f9445f
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 15 deletions.
20 changes: 15 additions & 5 deletions lib/cucumber/filters/retry.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,23 @@ module Cucumber
module Filters
class Retry < Core::Filter.new(:configuration)

def test_case(test_case)
super

def test_case(test_case)
configuration.on_event(:after_test_case) do |event|

This comment has been minimized.

Copy link
@mattwynne

mattwynne Mar 20, 2016

Member

We moved the whole event hook registration to happen before the test case is run, so that it's able to hear about the first failure - if it's registered after the call to super it will miss the first after_test_case event which will have already fired by the time the hook is registered.

test_case.describe_to(receiver) if event.result.failed?
next unless event.test_case == test_case

This comment has been minimized.

Copy link
@mattwynne

mattwynne Mar 20, 2016

Member

We added this guard because we're registering an event handler for every test case, so we don't want to run this block for any other test cases.

next unless event.result.failed?
next if test_case_counts[test_case] >= configuration.retry_attempts

This comment has been minimized.

Copy link
@mattwynne

mattwynne Mar 21, 2016

Member

Warning! @brasmusson realised this won't work in integration, as the test case objects' equality operator doesn't work properly. We need to use the test case's location as the key instead.

This comment has been minimized.

Copy link
@brasmusson

brasmusson Mar 21, 2016

Author Contributor

Yes, test_case.location did the trick in the Retry filter. The next problem when spiking integrating the Retry filter in the pipeline of filters, was a No method error in the Formatter::LegacyApi::Adapter. This is a very complex hack to support the old formatter API (which we will try to get rid of in v3.0.0). It might be the case that that code does not like the same test case being reported twice. Event though the fix might be simple, it could very well be that it takes a long time to find out how and were to do the fix.


test_case_counts[test_case] += 1
event.test_case.describe_to(receiver)
end

super
end

def test_case_counts
@test_case_counts ||= Hash.new { |h,k| h[k] = 0 }
end

end
end
end
end
22 changes: 12 additions & 10 deletions spec/cucumber/filters/retry_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,31 +43,33 @@

context "failing test case" do
it "describes the test case the specified number of times" do
expect(test_case).to receive(:describe_to).with(receiver).exactly(3).times
expect(receiver).to receive(:test_case) { |test_case|

This comment has been minimized.

Copy link
@mattwynne

mattwynne Mar 20, 2016

Member

We switched to expecting on the receiver, which is a stub, rather than stubbing a method on the test case object, which is a real instance of the test case class. I think it's better practice to not stub methods on real objects.

configuration.notify(fail)

This comment has been minimized.

Copy link
@mattwynne

mattwynne Mar 20, 2016

Member

We do the configuration.notify from within the receiver block to make the timing more realistic - when the filter is run in integration, the event will be emitted by the runner as the test case is passed out of the filter.

}.exactly(3).times

This comment has been minimized.

Copy link
@mattwynne

mattwynne Mar 20, 2016

Member

I think this was crucial - we had to make sure we simulate 3 failures from the test case, not just one 👍

filter.test_case(test_case)
configuration.notify(fail)
end
end

context "flaky test cases" do

context "a little flaky" do
it "describes the test case twice" do
expect(test_case).to receive(:describe_to).with(receiver).exactly(2).times
results = [fail, pass]
expect(receiver).to receive(:test_case) { |test_case|
configuration.notify(results.shift)
}.exactly(2).times

This comment has been minimized.

Copy link
@mattwynne

mattwynne Mar 20, 2016

Member

Because we changed to emit the result event from the stub receiver, we needed to use this pattern of popping results off a stack. Make sense?

filter.test_case(test_case)
configuration.notify(fail)
configuration.notify(pass)
end
end

context "really flaky" do
it "describes the test case 3 times" do
expect(test_case).to receive(:describe_to).with(receiver).exactly(3).times
results = [fail, fail, pass]
expect(receiver).to receive(:test_case) { |test_case|
configuration.notify(results.shift)
}.exactly(3).times
filter.test_case(test_case)
configuration.notify(fail)
configuration.notify(fail)
configuration.notify(pass)
end
end
end
end
end

0 comments on commit 8f9445f

Please sign in to comment.