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

Make HttpStatusAgent provide redirect info #1590

Merged
merged 3 commits into from
Jul 20, 2016
Merged

Make HttpStatusAgent provide redirect info #1590

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9dad35f
Make HttpStatusAgent provide redirect info
strugee Jul 19, 2016
c5b532d
Mock to_hash for HttpStatusAgent specs
strugee Jul 19, 2016
1a434d8
Refactor HttpStatusAgent spec mock responses
strugee Jul 20, 2016
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
3 changes: 2 additions & 1 deletion app/models/agents/http_status_agent.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,8 @@ def check_this_url(url, local_headers)

# Deal with failures
if measured_result.result
payload.merge!({ 'response_received' => true, 'status' => current_status })
final_url = boolify(interpolated['disable_redirect_follow']) ? url : measured_result.result.to_hash[:url]
Copy link
Contributor

Choose a reason for hiding this comment

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

@strugee Just looking at the code... perhaps measured_result.url would work here?

The measured_result is a wrapper around the result, with a method_missing property to pass any method calls back to the original result. (https://github.com/cantino/huginn/blob/29007c9019e35220964b16fb578c44c6effd0763/lib/time_tracker.rb#L15)

The other difference is that the tests have fake objects that may not have to_hash defined. If to_hash is a method on the object in production, you could append to_hash to the fake and bingo it should be fine.

Sorry, this is a point where faking/mocking can be a pain. And I say this as the one who wrote the original tests. 😄 I thought the tests would be easier, since faking a bunch of different types of responses could be difficult or add more dependencies to Huginn. But now the objects getting faked are more and more complicated. This "given and take" decision is now firmly in "take" mode. My apologies on that. 😭

Copy link
Contributor Author

Choose a reason for hiding this comment

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

measured_result.result.url doesn't work anywhere. measured_result.url works in RSpec, weirdly, but not in actual Rails. ????

In any case, I ended up just mocking to_hash (it's basically an alias to to_h). So specs pass now! \o/

Copy link
Member

Choose a reason for hiding this comment

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

This should have said measured_result.result.env.url.to_s. The url here is a URI object, and url is a string, so url != final_url which is set to redirected always evaluates to true. The specs are not actually testing anything because agent.faraday is mocked to return what they want.

payload.merge!({ 'final_url' => final_url, 'redirected' => (url != final_url), 'response_received' => true, 'status' => current_status })
# Deal with headers
if local_headers.present?
header_results = measured_result.result.headers.select {|header, value| local_headers.include?(header)}
Expand Down
24 changes: 19 additions & 5 deletions spec/controllers/http_status_agent_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
require 'rails_helper'

class MockResponse < Struct.new(:status, :headers, :url)
alias_method :to_hash, :to_h
end

describe 'HttpStatusAgent' do

let(:agent) do
Expand Down Expand Up @@ -109,7 +113,7 @@ def agent.checked_url
let(:header_value) { SecureRandom.uuid }

let(:event_with_a_successful_ping) do
agent.faraday.set(successful_url, Struct.new(:status, :headers).new(status_code, {}))
agent.faraday.set(successful_url, MockResponse.new(status_code, {}, successful_url))
Event.new.tap { |e| e.payload = { url: successful_url, headers_to_save: "" } }
end

Expand Down Expand Up @@ -208,10 +212,20 @@ def agent.checked_url
expect(agent.the_created_events[0][:payload]['url']).to eq(successful_url)
end

it "should return the final url" do
agent.receive events
expect(agent.the_created_events[0][:payload]['final_url']).to eq(successful_url)
end

it "should return whether the url redirected" do
agent.receive events
expect(agent.the_created_events[0][:payload]['redirected']).to eq(false)
end

describe "but the ping returns a status code of 0" do

let(:event_with_a_successful_ping) do
agent.faraday.set(successful_url, Struct.new(:status, :headers).new(0, {}))
agent.faraday.set(successful_url, MockResponse.new(0, {}, successful_url))
Event.new.tap { |e| e.payload = { url: successful_url, headers_to_save: "" } }
end

Expand Down Expand Up @@ -241,7 +255,7 @@ def agent.checked_url
describe "but the ping returns a status code of -1" do

let(:event_with_a_successful_ping) do
agent.faraday.set(successful_url, Struct.new(:status, :headers).new(-1, {}))
agent.faraday.set(successful_url, MockResponse.new(-1, {}, successful_url))
Event.new.tap { |e| e.payload = { url: successful_url, headers_to_save: "" } }
end

Expand Down Expand Up @@ -302,7 +316,7 @@ def agent.checked_url

describe "with a header specified" do
let(:event_with_a_successful_ping) do
agent.faraday.set(successful_url, Struct.new(:status, :headers).new(status_code, {header => header_value}))
agent.faraday.set(successful_url, MockResponse.new(status_code, {header => header_value}, successful_url))
Event.new.tap { |e| e.payload = { url: successful_url, headers_to_save: header } }
end

Expand All @@ -318,7 +332,7 @@ def agent.checked_url
let(:nonexistant_header) { SecureRandom.uuid }

let(:event_with_a_successful_ping) do
agent.faraday.set(successful_url, Struct.new(:status, :headers).new(status_code, {header => header_value}))
agent.faraday.set(successful_url, MockResponse.new(status_code, {header => header_value}, successful_url))
Event.new.tap { |e| e.payload = { url: successful_url, headers_to_save: header + "," + nonexistant_header } }
end

Expand Down