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

Jump to bottom

Extend failure messages (issue 97) #114

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Extend failure messages (issue 97) #114

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
45 changes: 39 additions & 6 deletions lib/resque_spec/matchers.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,16 @@ def queue(actual)
end
end

def actual_queue_message
if (!@times || @times == 1) && !(actual_queue = queue(actual)).empty?
actual_queue_args_str = actual_queue.last[:args].join(', ')

", but got #{actual} with [#{actual_queue_args_str}]#{@times_info} instead"
end
end

failure_message do |actual|
"expected that #{actual} would be queued with [#{expected_args.join(', ')}]#{@times_info}"
"expected that #{actual} would be queued with [#{expected_args.join(', ')}]#{@times_info}#{actual_queue_message}"
end

failure_message_when_negated do |actual|
Expand Down Expand Up @@ -102,8 +110,16 @@ def queue(actual)
end
end

def actual_queue_message
if (!@times || @times == 1) && !(actual_queue = queue(actual)).empty?
actual_queue_args_str = actual_queue.last[:args].join(', ')

", but got #{actual} with [#{actual_queue_args_str}]#{@times_info} instead"
end
end

failure_message do |actual|
"expected that #{actual} would have [#{expected_args.join(', ')}] queued#{@times_info}"
"expected that #{actual} would have [#{expected_args.join(', ')}] queued#{@times_info}#{actual_queue_message}"
end

failure_message_when_negated do |actual|
Expand Down Expand Up @@ -185,13 +201,13 @@ def schedule_queue_for(actual)
chain :at do |timestamp|
@interval = nil
@time = timestamp
@time_info = "at #{@time}"
@time_info = " at #{@time}"
end

chain :in do |interval|
@time = nil
@interval = interval
@time_info = "in #{@interval} seconds"
@time_info = " in #{@interval} seconds"
Copy link
Owner

Choose a reason for hiding this comment

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

Not happy with the spacing here, leave it up to the caller.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, I'll attend this today.

Copy link
Author

Choose a reason for hiding this comment

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

The issues I see if I don't add the blank space are that:

1 - There would be some inconsistency in the failure_message method for the have_scheduled matcher, given that only this method uses the format of ["expected that #{actual} would not have [#{expected_args.join(', ')}] scheduled", @time_info].join(' '), all the others follow the "expected that #{actual} would be queued with [#{expected_args.join(', ')}]#{@times_info}" format.
2 - The last line of the failure_message will have to be updated to ["expected that #{actual} would not have [#{expected_args.join(', ')}] scheduled", "#{@time_info},", actual_queue_message].join(' ') so that there is a comma before showing the existent enqueued job in the failure message, and given that there may be times when the @time_info is empty then an extra blank space would appear in those cases.

What do you think?
(Please let me know if I didn't communicate well any of those points.)

Copy link
Owner

Choose a reason for hiding this comment

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

As an example, addressing point 2:

Line 117 could be:

"but got #{actual} with [#{actual_queue_args_str}]#{@times_info} instead"

Line 248 could be:

`["expected that #{actual} would not have [#{expected_args.join(', ')}] scheduled", "#{@time_info}", actual_queue_message].join(', ')

The ", " is inserted by the caller as needed. Each part of the message is just a building block and not specifying how it is joined to the rest of the message.

Does that make sense?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks. I think I see what you say, however, since a comma is added by each element in the array then the resulting failure text goes like the following:

expected that Person would have [a, b] scheduled, at 2015-06-08 17:41:44 -0500, but got Person with [Les, Hill] scheduled at 2015-06-08 17:41:44 -0500 instead

(Please check that there is an extra comma after "scheduled").
I don't really like having an extra comma there and I think point 1 is valid. but you tell me, I can leave it with the extra comma if that works for you.

Copy link
Owner

Choose a reason for hiding this comment

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

LOL, yea I did not completely rewrite it, so that will need to be addressed too.

Go ahead with my latest suggestion (extra comma) and I will fix that when I merge the PR. Thanks for the doing the PR.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks.

I just realized that when there is no @time_info two commas will appear as in:

expected that Person would have [Les, asdf] scheduled, , but got Person with [Les, Hill] scheduled instead

This goes for scenario in https://github.com/leshill/resque_spec/blob/master/spec/resque_spec/matchers_spec.rb#L357 for instance. I think you can 1) merge this as it is (since it's working and is consistent, in my opinion) or 2) I leave it is and you do the small modifications you see are needed when merging (having an extra blank space works better for me than having one extra comma).

What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

Let me know in case you need anything else from my side : )

end

match do |actual|
Expand All @@ -211,12 +227,29 @@ def schedule_queue_for(actual)
end
end

def actual_queue_time_info(actual_queue)
if @time
" at #{actual_queue[:time]}"
elsif @interval
seconds = (actual_queue[:time] - actual_queue[:stored_at]).round
" in #{seconds} seconds"
end
end

def actual_queue_message
if !(actual_queue = schedule_queue_for(actual)).empty?
actual_queue_args_str = actual_queue.last[:args].join(', ')

", but got #{actual} with [#{actual_queue_args_str}] scheduled#{actual_queue_time_info(actual_queue.last)} instead"
end
end

failure_message do |actual|
["expected that #{actual} would have [#{expected_args.join(', ')}] scheduled", @time_info].join(' ')
"expected that #{actual} would have [#{expected_args.join(', ')}] scheduled#{@time_info}#{actual_queue_message}"
end

failure_message_when_negated do |actual|
["expected that #{actual} would not have [#{expected_args.join(', ')}] scheduled", @time_info].join(' ')
"expected that #{actual} would not have [#{expected_args.join(', ')}] scheduled#{@time_info}"
end

description do
Expand Down