Make HttpStatusAgent provide redirect info #1590
Conversation
| @@ -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] | |||
There was a problem hiding this comment.
@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. 😭
There was a problem hiding this comment.
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/
There was a problem hiding this comment.
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.
|
This is ready for review. /cc @cantino |
|
In particular I'd note that I'm not sure if the included specs are enough. |
| @@ -36,6 +36,9 @@ def f.get url | |||
|
|
|||
| def f.set url, response, time = nil | |||
| sleep(time/1000) if time | |||
| def response.to_hash | |||
There was a problem hiding this comment.
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
endthen 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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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!
There was a problem hiding this comment.
That's cool. I won't rush but it'll be on my list.
|
👍 thanks everyone! |
This key was introduced in #1590 but it is likely that it has never worked as expected.
This key was introduced in #1590 but it is likely that it has never worked as expected.
This is just a small change to make HttpStatusAgent provide info on what the final, post-redirections URL was for a given request, and a boolean representing whether or not it redirected or not.
Specs are failing. Here's the problem: for some reason,
measured_result.result.to_hash[:url]throws an exception in the RSpec environment, saying thatto_hashisn't a method, and did I meanto_h. Changing the code tomeasured_result.result.to_h[:url]completely fixes the specs, but breaks in the actual Rails environment with a similar exception (to_hisn't a method, did you meanto_hash). So I'm not sure how to solve that one.