Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce a Duration object #71

Merged
merged 2 commits into from
Oct 12, 2014
Merged

Introduce a Duration object #71

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3e39c8d
Introduce a Duration object in Cucumber::Core::Test::Result
brasmusson Oct 9, 2014
7230654
Let Test::Timer return a Test::Result::Duration object
brasmusson Oct 12, 2014
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
30 changes: 26 additions & 4 deletions lib/cucumber/core/test/result.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ def with_duration(new_duration)
class Raisable < StandardError
attr_reader :message, :duration

def initialize(message = "", duration = :unknown, backtrace = nil)
def initialize(message = "", duration = UnknownDuration.new, backtrace = nil)
@message, @duration = message, duration
super(message)
set_backtrace(backtrace) if backtrace
Expand All @@ -101,7 +101,7 @@ class Undefined < Raisable

def describe_to(visitor, *args)
visitor.undefined(*args)
visitor.duration(duration, *args) unless duration == :unknown
visitor.duration(duration, *args)
self
end

Expand All @@ -116,7 +116,7 @@ class Skipped < Raisable

def describe_to(visitor, *args)
visitor.skipped(*args)
visitor.duration(duration, *args) unless duration == :unknown
visitor.duration(duration, *args)
self
end

Expand All @@ -131,7 +131,7 @@ class Pending < Raisable

def describe_to(visitor, *args)
visitor.pending(self, *args)
visitor.duration(duration, *args) unless duration == :unknown
visitor.duration(duration, *args)
self
end

Expand Down Expand Up @@ -201,6 +201,28 @@ def total
total_passed + total_failed + total_skipped + total_undefined
end
end

class Duration
attr_reader :duration

def initialize(duration)
@duration = duration
end

def exist?
true
end
end

class UnknownDuration
def exist?
false
end

def duration
0
end
end
end
end
end
Expand Down
4 changes: 3 additions & 1 deletion lib/cucumber/core/test/timer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
require 'cucumber/core/test/result'

module Cucumber
module Core
module Test
Expand All @@ -8,7 +10,7 @@ def start
end

def duration
nsec
Result::Duration.new(nsec)
end

def nsec
Expand Down
5 changes: 3 additions & 2 deletions spec/cucumber/core/test/action_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
require 'cucumber/core/test/action'
require 'cucumber/core/test/duration_matcher'

module Cucumber
module Core
Expand Down Expand Up @@ -76,13 +77,13 @@ module Test
it "records the nanoseconds duration of the execution on the result" do
mapping = Action.new { }
duration = mapping.execute(last_result).duration
expect( duration ).to eq 1
expect( duration ).to be_duration 1
end

it "records the duration of a failed execution" do
mapping = Action.new { raise StandardError }
duration = mapping.execute(last_result).duration
expect( duration ).to eq 1
expect( duration ).to be_duration 1
end
end

Expand Down
17 changes: 17 additions & 0 deletions spec/cucumber/core/test/duration_matcher.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# -*- encoding: utf-8 -*-
require 'cucumber/core/test/result'
require 'rspec/expectations'

module Cucumber::Core::Test
RSpec::Matchers.define :be_duration do |expected|
match do |actual|
actual.exist? and actual.duration == expected
end
end

RSpec::Matchers.define :an_unknown_duration do
match do |actual|
not actual.exist? and expect(actual).to respond_to(:duration)
end
end
end
38 changes: 33 additions & 5 deletions spec/cucumber/core/test/result_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# -*- encoding: utf-8 -*-
require 'cucumber/core/test/result'
require 'cucumber/core/test/duration_matcher'

module Cucumber::Core::Test
describe Result do
Expand All @@ -9,7 +10,7 @@ module Cucumber::Core::Test

describe Result::Passed do
subject(:result) { Result::Passed.new(duration) }
let(:duration) { 1 * 1000 * 1000 }
let(:duration) { Result::Duration.new(1 * 1000 * 1000) }

it "describes itself to a visitor" do
expect( visitor ).to receive(:passed).with(args)
Expand Down Expand Up @@ -38,7 +39,7 @@ module Cucumber::Core::Test

describe Result::Failed do
subject(:result) { Result::Failed.new(duration, exception) }
let(:duration) { 1 * 1000 * 1000 }
let(:duration) { Result::Duration.new(1 * 1000 * 1000) }
let(:exception) { StandardError.new("error message") }

it "describes itself to a visitor" do
Expand Down Expand Up @@ -84,6 +85,7 @@ module Cucumber::Core::Test

it "describes itself to a visitor" do
expect( visitor ).to receive(:undefined).with(args)
expect( visitor ).to receive(:duration).with(an_unknown_duration, args)
result.describe_to(visitor, args)
end

Expand All @@ -99,6 +101,7 @@ module Cucumber::Core::Test

it "describes itself to a visitor" do
expect( visitor ).to receive(:skipped).with(args)
expect( visitor ).to receive(:duration).with(an_unknown_duration, args)
result.describe_to(visitor, args)
end

Expand All @@ -111,8 +114,8 @@ module Cucumber::Core::Test

describe Result::Summary do
let(:summary) { Result::Summary.new }
let(:failed) { Result::Failed.new(10, exception) }
let(:passed) { Result::Passed.new(11) }
let(:failed) { Result::Failed.new(Result::Duration.new(10), exception) }
let(:passed) { Result::Passed.new(Result::Duration.new(11)) }
let(:skipped) { Result::Skipped.new }
let(:unknown) { Result::Unknown.new }
let(:undefined) { Result::Undefined.new }
Expand Down Expand Up @@ -158,13 +161,38 @@ module Cucumber::Core::Test

it "records durations" do
[passed, failed].each { |r| r.describe_to summary }
expect( summary.durations ).to eq [11, 10]
expect( summary.durations[0] ).to be_duration 11
expect( summary.durations[1] ).to be_duration 10
end

it "records exceptions" do
[passed, failed].each { |r| r.describe_to summary }
expect( summary.exceptions ).to eq [exception]
end
end

describe Result::Duration do
subject(:duration) { Result::Duration.new(10) }

it "exist? returns true" do
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to use nil? here? (obviously the opposite way around to this exist? method). That way we're not inventing a new protocol for null objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is best to adhere to the standard protocol. In the back of my head I remember seeing the construct isNull() in relation to null objects, but I can tell you that there a lot more search result for having the null object quacking like a real object, than about naming the query to distinguish the null object from a real object.
Fixed in cc5d1cb and cucumber/common@7b1d1d9

Copy link
Member

Choose a reason for hiding this comment

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

Relevant http://devblog.avdi.org/2011/05/30/null-objects-and-falsiness/

Copy link
Member

Choose a reason for hiding this comment

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

What's your take-away from that @tooky

Copy link
Member

Choose a reason for hiding this comment

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

@mattwynne sending .nil? or .exist? is really a type check disguised as a message send.

The example where I've seen it used, something like:

unless duration.exist?
  formatter.add_duration duration.duration * 10 ** 9
end

I think perhaps we could do something like:

duration.with_duration do |milli_secs| # I'm guessing this is what we are storing duration in?
  formatter.add_duration milli_secs * 10 ** 9
end

The standard duration could then yield the value to the block, and the unknown duration could be a no-op.

WDYT? /cc @brasmusson

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine if #tap yields the integer (ms) value, or doesn't yield for the null object. Don't you agree @tooky?

If we wanted to make it crystal-clear, we could call it #tap_value.

Copy link
Member

Choose a reason for hiding this comment

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

@mattwynne @brasmusson I think it would be a bit weird to yield the value, would it violate the POLS not to get the actual object?

#tap_value is ok... but how about we give the value a good name? Is it recorded in milli seconds?

Then you could do:

duration.tap { |duration| formatter.add_duration duration.mill_secs * 10 ** 9 }

(Incidentally, should the duration deal with time conversions?)

@brasmusson I think that UnkownDuration should implement the same interface as a duration, but perhaps rather than return 0, it could return itself?

Copy link
Member

Choose a reason for hiding this comment

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

Agreed @tooky - #tap should yield the object itself. A good name for the method is either #ms or #milliseconds IMO - no need for underscores.

It seems to me that with this API, bug-free code would never call that method on the null object, so it doesn't really need to implement it. Or it could throw an error telling the client to use the #tap trick. I think returning self might be a bit surprising.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The duration is actually recorded in nano seconds so #nanoseconds could be the method name. As a side note, the gherkin_formatter_adapter converts the duration to seconds, and then the gherkin::json_formatter converts it back to nanoseconds.

If we look at the duration object API as #nanoseconds is only allowed to be called inside a #tap block, than throwing an error is reasonable.
If we look at #nanoseconds as a regular method of the API, then also UnkownDuration should return something reasonable for it. 0 is kind of the most neutral integer value (would it have been a float, then Float::NAN could be used). Returning self is possible, if UnkownDuration also use some other tricks from http://devblog.avdi.org/2011/05/30/null-objects-and-falsiness/, so it responds reasonable "nullish" to the methods that could be sent to an integer:

  def method_missing(*args, &block)
    self
  end

  def to_a; []; end
  def to_s; ""; end
  def to_f; 0.0; end
  def to_i; 0; end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With 47ec1e1 and cucumber/common@00e9701 #tap and #nanoseconds are used to pass the duration value to the JSON formatter, unless its an UnkownDuration.

expect( duration.exist? ).to be_truthy
end

it "has a duration" do
expect( duration.duration ).to eq 10
end
end

describe Result::UnknownDuration do
subject(:duration) { Result::UnknownDuration.new }

it "exist? returns false" do
expect( duration.exist? ).to be_falsy
end

it "return duration 0" do
expect( duration.duration ).to eq 0
end
end
end
end
5 changes: 3 additions & 2 deletions spec/cucumber/core/test/runner_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
require 'cucumber/core/test/runner'
require 'cucumber/core/test/case'
require 'cucumber/core/test/step'
require 'cucumber/core/test/duration_matcher'

module Cucumber::Core::Test
describe Runner do
Expand Down Expand Up @@ -33,7 +34,7 @@ module Cucumber::Core::Test

it "records the nanoseconds duration of the execution on the result" do
expect( report ).to receive(:after_test_case) do |reported_test_case, result|
expect( result.duration ).to eq 1
expect( result.duration ).to be_duration 1
end
test_case.describe_to runner
end
Expand All @@ -44,7 +45,7 @@ module Cucumber::Core::Test

it "records the duration" do
expect( report ).to receive(:after_test_case) do |reported_test_case, result|
expect( result.duration ).to eq 1
expect( result.duration ).to be_duration 1
end
test_case.describe_to runner
end
Expand Down
13 changes: 13 additions & 0 deletions spec/cucumber/core/test/timer_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,22 @@
require 'cucumber/core/test/timer'
require 'cucumber/core/test/duration_matcher'

module Cucumber
module Core
module Test
describe Timer do
before do
time = double
allow( Time ).to receive(:now) { time }
allow( time ).to receive(:nsec).and_return(946752000, 946752001)
allow( time ).to receive(:to_i).and_return(1377009235, 1377009235)
end

it "returns a Result::Duration object" do
timer = Timer.new.start
expect( timer.duration ).to be_duration 1
end

it "would be slow to test" do
# so we won't
end
Expand Down