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 2 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
23 changes: 18 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
Expand Up @@ -36,6 +36,9 @@ def f.get url

def f.set url, response, time = nil
sleep(time/1000) if time
def response.to_hash
Copy link
Member

@cantino cantino Jul 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Oy, this is getting ugly. Not your addition, particularly, but this whole mocking code.

The reason to_hash wasn't working is because the mocked responses are built with Struct.new(:status, :headers), which apparently won't accept to_hash.

It'd be nice if we could at least make a mock response class near the top, something like:

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

then replace instances of Struct.new(:status, :headers).new( with MockResponse.new(.

If you don't want to dive into refactoring this spec, I'm happy to merge your change and then work on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cantino implemented MockResponse.

In principle I'm happy to try my hand at refactoring this spec, but I suspect I'm not experienced enough at Ruby to do a good job. So you're probably better off either giving me some pointers, or just doing it yourself :)

Either way is OK by me.

Copy link
Contributor

Choose a reason for hiding this comment

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

@cantino @strugee

I don't think going with the new MockResponse is the best way to change this mock. The structu can be changed itself to:

Struct.new(:status, :headers, :to_hash)
      .new(200, {}, { url: url })

Then you can test the url coming out. That's all to_hash is, a method returning a hash.

Given what I've learned about how tests are normally written on huginn agents, I can throw my hat into possible devs to refactor. I think the better approach at this point would be to use webmock to fake the web response instead of faking the result with a fake object. Then the tests would have everything usually available when making a web call. It's more complicated, but it leads to simpler tests in this situation.

Copy link
Member

Choose a reason for hiding this comment

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

@darrencauthon that's fine too, although I might define a method to return the struct instead of repeating Struct.new(:status, :headers, :to_hash) a lot. If you'd like to give this a go, I'll merge @strugee's PR, then you can have at it. No rush!

Copy link
Contributor

Choose a reason for hiding this comment

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

That's cool. I won't rush but it'll be on my list.

to_h
end
programmed_responses[url] = response
end
end
Expand Down Expand Up @@ -109,7 +112,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, Struct.new(:status, :headers, :url).new(status_code, {}, successful_url))
Event.new.tap { |e| e.payload = { url: successful_url, headers_to_save: "" } }
end

Expand Down Expand Up @@ -208,10 +211,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, Struct.new(:status, :headers, :url).new(0, {}, successful_url))
Event.new.tap { |e| e.payload = { url: successful_url, headers_to_save: "" } }
end

Expand Down Expand Up @@ -241,7 +254,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, Struct.new(:status, :headers, :url).new(-1, {}, successful_url))
Event.new.tap { |e| e.payload = { url: successful_url, headers_to_save: "" } }
end

Expand Down Expand Up @@ -302,7 +315,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, Struct.new(:status, :headers, :url).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 +331,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, Struct.new(:status, :headers, :url).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