Scoping and filtering methods, and more helpful UnexpectedResponse #9

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

ravigadad commented Apr 23, 2012

  1. Added an optional status code as an attribute of an UnexpectedResponse, since in most cases this exception is thrown by the APIDriver, and it's helpful to know why the request failed. Especially useful when the response was actually a 500 error instead of a 404, for example.

  2. Added #where and #mapped_by methods to MentalModel::Matcher, for scoping based on a select block, and modification of collection elements using a map block.

Ravi Gadad added some commits Apr 20, 2012

Ravi Gadad (rg) Add status code to UnexpectedResponse
When an APIDriver request results in an unexpected response, it's extremely helpful to know what the response
was (especially in the case of 500 errors).  Adding this to the exception.
b0ecaf5
Ravi Gadad (rg) Add .where and .mapped_by to Matcher
Allow for filtering using a given block, with MentalModel::Matcher#where, and mapping using a given block, with
MentalModel::Matcher#mapped_by.
109c51a
Ravi Gadad (rg) Assertion helper uses scopers/filters
Now we can use scopers (.where, .only) and filters (.mapped_by) with #assert-compatible testing frameworks as
well as with RSpec.
73bbc13
Ravi Gadad (rg) New Matcher usage to fix timing issues
You can now use the MentalModel::Matcher in a new way - by matching against an object that returns the observed data,
rather than matching against an array of the observed data itself.  This enables us to, within the matcher itself,
retry failures for a period of time, to account for possible rendering delays (such as Javascript-rendered elements)
999bdee

@jwilger jwilger commented on the diff Apr 23, 2012

lib/kookaburra/api_driver.rb
@@ -82,7 +82,7 @@ def check_response_status!(request_type, response, options)
verb, default_status = verb_map[request_type]
expected_status = options[:expected_response_status] || default_status
unless expected_status == response.status
- raise UnexpectedResponse, "#{verb} to #{response.url} responded with " \
+ raise UnexpectedResponse.new(response.status), "#{verb} to #{response.url} responded with " \
@jwilger

jwilger Apr 23, 2012

Owner

I was under the impression that in raise Exception, message the "message" part was simply passed into Exception.new, so not sure what is happening to message on lines where this is raised now. Maybe I'm wrong?

@ravigadad

ravigadad Apr 23, 2012

Contributor

Yeah, this is due to Ruby's wacky implementation of Kernel.raise. If you give it an exception class, the initializer is called with the message as the first argument, but the message is also stored on the exception. If you have an #initialize for your exception, you may not want the message as a param - in which case you explicitly create the instance of the Exception. The message is still stored on the exception somehow (possibly using a separate setter method), though according to what I read here: http://www.ruby-forum.com/topic/207448, the message and backtrace (optional third param to raise) are tucked away in inaccessible instance variables. When you call #message on the exception, the message is returned even if it wasn't part of the initialization, since #message uses #to_s which uses the internal variables. Magic! And yes, this has been raised as an issue, and debated: http://bugs.ruby-lang.org/issues/5898

@ravigadad

ravigadad Apr 23, 2012

Contributor

(which isn't to say I agree with the issue as it was raised - it just seems to be another instance where the confusing dichotomy of Exception.new and Kernel.raise cause problems).

@jwilger

jwilger Apr 23, 2012

Owner

I wonder if the best thing to do would be to just pass in method, URL and status and let the exception class construct its own message from that? (i.e. raise UnexpectedResponse.new(:post, 'http://foo.com/bar', 404)).

Regards,
John Wilger

ph: 971-678-0999
http://johnwilger.com

@ravigadad

ravigadad Apr 23, 2012

Contributor

That could work - in which case, I think we should use a different class of exception for the detect_server_error! exception. Unless we assume that a detected server error should always show up as a 500. I think UnexpectedResponse should always be expected to indicate the actual response.

@ravigadad

ravigadad Apr 23, 2012

Contributor

At any rate, that sounds like a refactoring - the message is received the same way whether the exception constructor creates it or it's set by the raise call.

Owner

jwilger commented Apr 23, 2012

I will give a more detailed review later, just wanted to note the exception issue that caught my eye.

@ravigadad ravigadad commented on the diff Apr 23, 2012

lib/kookaburra/ui_driver/ui_component.rb
@@ -149,7 +149,7 @@ def component_locator
# function returns true
def detect_server_error!
if @server_error_detection.try(:call, browser)
- raise UnexpectedResponse, "Your server error detection function detected a server error. Looks like your applications is busted. :-("
+ raise UnexpectedResponse.new, "Your server error detection function detected a server error. Looks like your application is busted. :-("
@ravigadad

ravigadad Apr 23, 2012

Contributor

Note this change, related to your comment on the call to Kernel.raise in APIDriver. Here I had to pass an instance of the exception, rather than the class - otherwise, the new exception would have the message set as the status_code (as well as the return from #message, even though they're completely unrelated), because the message is sent in as the first argument to initialize, when raise is given a class.

Contributor

ravigadad commented Apr 25, 2012

Reorganized pull requests to get timing fix into projectdx/kookaburra master.

ravigadad closed this Apr 25, 2012

Then why aren't there two objects?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment