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.

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

Use more display precision for Time and DateTime #416

Merged
merged 1 commit into from
Jan 11, 2014
Merged
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
33 changes: 31 additions & 2 deletions lib/rspec/matchers/built_in/eq.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,47 @@ def match(expected, actual)
end

def failure_message
"\nexpected: #{expected.inspect}\n got: #{actual.inspect}\n\n(compared using ==)\n"
"\nexpected: #{format_object(expected)}\n got: #{format_object(actual)}\n\n(compared using ==)\n"
end

def failure_message_when_negated
"\nexpected: value != #{expected.inspect}\n got: #{actual.inspect}\n\n(compared using ==)\n"
"\nexpected: value != #{format_object(expected)}\n got: #{format_object(actual)}\n\n(compared using ==)\n"
end

def description
"#{name_to_sentence} #{@expected.inspect}"
end

def diffable?; true; end

private

def format_object(object)
if Time === object
format_time(object)
elsif defined?(DateTime) && DateTime === object
format_date_time(object)
else
object.inspect
end
end

TIME_FORMAT = "%Y-%m-%d %H:%M:%S"
# Append microseconds to the default format string
def format_time(time)
time.strftime("#{TIME_FORMAT}.#{"%06d" % time.usec} %z")
end

DATE_TIME_FORMAT = "%a, %d %b %Y %H:%M:%S.%6N %z"
Copy link
Member Author

Choose a reason for hiding this comment

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

This looks like the following:

 Failure/Error: expect(date1).to eq(date2)

   expected: Sat, 01 Jan 2000 01:01:00.200000 +0000
        got: Sat, 01 Jan 2000 01:01:00.100000 +0000

   (compared using ==)

Copy link
Member

Choose a reason for hiding this comment

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

👍 That looks great.

Seeing your time format string makes me think that you might be able to simplify the TIME_FORMAT string even further so that there's not the separate % time.usec bit. Would %6N work in that as well?

# ActiveSupport sometimes overrides inspect. If `ActiveSupport` is
# defined use a custom format string that includes more time precision.
def format_date_time(date_time)
if defined?(ActiveSupport)
date_time.strftime(DATE_TIME_FORMAT)
else
date_time.inspect
end
end
Copy link
Member

Choose a reason for hiding this comment

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

Given the activesupport bug you have a rails PR for, what is the current behavior?

Copy link
Member

Choose a reason for hiding this comment

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

Also, do you think this should check defined?(ActiveSupport)? In another context default_inspect could be defined but it may not be what we want.

Copy link
Member Author

Choose a reason for hiding this comment

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

The current behavior will not solve the problem, both lines will show something like "Tuesday, November 22 12:34:00". I figured we might want to actually hand craft a time format for DateTime but I didn't do that yet, as I wanted to present this as an option. If I researched it correctly the default inspect for DateTime does not appear to be built with a format string but instead has some logic and is implemented in C.

I also considered checking for ActiveSupport. Checking date.defined(ActiveSupport) && date.respond.to?(:default_inspect) would probably give us more precise targeting of the issue. I can't just check for ActiveSupport alone though, due to its optional nature.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you think I should just work up another format string? I feel reasonably comfortable that anything named default_inspect will be what we want but I think we may still want to use a different string so that we're not relying on an unsure ActiveSupport bug fix.

end
end
end
Expand Down
65 changes: 64 additions & 1 deletion spec/rspec/matchers/built_in/eq_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,70 @@ module Matchers
end
end
end

context "Time Equality" do
RSpec::Matchers.define :a_string_with_differing_output do
match do |string|
time_strings = /expected: (.+)\n.*got: (.+)$/.match(string).captures
time_strings.uniq.count == 2
end
end

RSpec::Matchers.define :a_string_with_identical_output do
match do |string|
time_strings = /expected: value != (.+)\n.*got: (.+)$/.match(string).captures
time_strings.uniq.count == 1
end
end

context 'with Time objects' do
let(:time1) { Time.utc(1969, 12, 31, 19, 01, 40, 101) }
let(:time2) { Time.utc(1969, 12, 31, 19, 01, 40, 102) }

it 'Should have a different output string when differing by milliseconds' do
expect {
expect(time1).to eq(time2)
}.to fail_with(a_string_with_differing_output)
end
end

context 'with DateTime objects' do
require 'date'
let(:date1) { DateTime.new(2000, 1, 1, 1, 1, Rational(1, 10)) }
let(:date2) { DateTime.new(2000, 1, 1, 1, 1, Rational(2, 10)) }

it 'Should have diffferent output string when differing by milliseconds' do
expect {
expect(date1).to eq(date2)
}.to fail_with(a_string_with_differing_output)
end

it 'should not raise an exception if DateTime is not defined' do
hide_const('DateTime')
expect {
expect(5).to eq(4)
}.to raise_error(RSpec::Expectations::ExpectationNotMetError)
end
Copy link
Member

Choose a reason for hiding this comment

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

Again, it would be nice if this used the expect { }.fail_with form. You could do this:

RSpec::Matchers.define :a_string_with_differing_datetime_output do
  match do |string|
    time_strings = /expected: (.+)\n.*got: (.+)$/.match(string).captures
    time_strings.uniq.count == 2
  end
end

it 'differentiates the date time objects when their milliseconds differ' do
  expect {
    expect(date2).to eq(date1)
  }.to fail_with(a_string_with_differing_datetime_output)
end


it 'fails with identical messages' do
expect {
expect(date1).to_not eq(date1)
}.to fail_with(a_string_with_identical_output)
end

describe 'ActiveSuppot is defined' do
it 'fails with a differing message' do
stub_const("ActiveSupport", Module.new)
allow(date1).to receive(:inspect).and_return("Timestamp")
allow(date2).to receive(:inspect).and_return("Timestamp")

expect {
expect(date1).to eq(date2)
}.to fail_with(a_string_with_differing_output)
end
end
end
end
Copy link
Member

Choose a reason for hiding this comment

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

There are no specs for the negated case (e.g. expect(x).not_to eq(y)) -- can you add those as well?

end
end
end