Writing tests using webmock
Closed this issue · 6 comments
I tried to write tests for dyn-rb.
For that I used webmock to simulate API calls.
To follow the rest, you may want to check a sample that I pushed to:
https://github.com/spier/dyn-rb/tree/stub_test
webmock disables all remote API calls by default.
Therefore I normally just write a test that uses the client. This then leads to exceptions which indicate which API calls need to be stubbed.
The exceptions look this:
WebMock::NetConnectNotAllowedError:
Real HTTP connections are disabled. Unregistered request: GET https://emailapi.dynect.net/rest/json/reports/bounces?apikey=1&endtime=3&startindex=0&starttime=2 with headers {'Accept'=>'*/*', 'Accept-Encoding'=>'gzip;q=1.0,deflate;q=0.6,identity;q=0.3', 'Content-Type'=>'application/x-www-form-urlencoded', 'User-Agent'=>'dyn-rb 1.0.3'}
dyn-rb currently makes this type of iterative test implementation impossible, as the exception handling in lib/dyn/messaging.rb assumes that the exception object will have a response property, which is not the case of the WebMock::NetConnectNotAllowedError exception that is thrown.
This leads to this error:
Failure/Error: response = dyn.bounces.list(2,3)
NoMethodError:
undefined method `response' for #<WebMock::NetConnectNotAllowedError:0x007ff56e0f1340>
# ./lib/dyn/messaging.rb:143:in `rescue in api_request'
# ./lib/dyn/messaging.rb:136:in `api_request'
# ./lib/dyn/messaging.rb:118:in `get'
# ./lib/dyn/messaging/bounces.rb:31:in `list'
# ./spec/dyn-rb_spec.rb:22:in `block (3 levels) in <top (required)>'
Therefore I was wondering:
Does the api_request method in lib/dyn/messaging.rb have to make assumptions about the Exception that is returned? If it would not do that, then it would be much easier to create tests for this API client with webmock :)
+1 I've not been working with dyn-rb for more than a day but agree very much with you that something to facilitate testing is certainly on the priority list.
@spier been looking at this issue again myself today. IMHO you are completely right that the api_request method should not make assumptions about the exception.
I'd go even further to say that unless absolutely required it should handle the exception at all and allow any errors which may occur to bubble up.
Error responses sent by the API will have a response property, but you're right that the SDK probably shouldn't assume that this will be there, particularly as 5xx errors probably wouldn't have this.
@spier since you already have a fork going, could you try a simple check for response in the rescue? My Ruby's a little rusty but I think something like this should do the trick:
response_body = begin
response = block.call
response.body
rescue Exception => e
if @verbose
puts "I have #{e.inspect} with #{e.http_code}"
end
if e.response.nil?
raise e
else
e.response
end
end
@SirRawlins are you saying you'd prefer the rescue to be dropped completely, and for errors to always raise an exception? If so, personally I'd be okay with this instead, and it would be more in line with how errors are handled by the other language SDKs.
I prefer the 2nd option to just let the exceptions bubble up to the caller, as the caller needs to decide himself anyways what he wants to do if the API request fails (e.g. wait and try again).
So without catching the Exception, the code would be:
# Handles making Dynect API requests and formatting the responses properly.
def api_request(&block)
response_body = begin
response = block.call
response.body
end
response = JSON.parse(response_body || '{}')
if (response["response"] && response["response"]["status"] == 200)
response["response"]["data"]
else
response
end
end One other question:
What is the purpose of the verbose flag? The API clients for both Messaging and Traffic can be initialized with verbose=true/false. @sunnygleason might know?
Please also take a look at my branch https://github.com/spier/dyn-rb/tree/issues/6 to see some sample tests for /bounces, /issues, /complaints and let me know if you thing that this is a good testing approach.