search

waterlink / rack-reverse-proxy

A Reverse Proxy for Rack
MIT License
196 stars 52 forks source link

fix Rack response for https_redirect #43

Closed jjb closed 7 years ago

jjb commented 7 years ago

see also https://github.com/waterlink/rack-reverse-proxy/issues/36

waterlink commented 7 years ago

Oh, that is a quite interesting bug here. How do you think we should test it?

jjb commented 7 years ago

I don't have ideas for how to test. Looks like all your tests are at least "one level" away from a "real" http response test. we would maybe need to set up a new type of test where a little rack webserver were run.

The fix clearly brings the code back into the rack spec though. The third element in the array needs to be able to respond to each. ruby strings responded to each (mixed in Enumerable) until 1.9, so maybe that's the history of a simple string being used as the third element.

see:

waterlink commented 7 years ago

I'm also wondering why no tests failed when this change has been introduced. This effectively means that the line under the question is not tested.

It would be amazing to set up some sort of integration test, where we run 2 servers: proxy and the one being proxied.

jjb commented 7 years ago

sounds cool, but I don't have time to do that. if this were my engineering organization i would insist that we set that up before moving forward, but since this is a library with a bug maybe we can merge this and then make another TODO ticket for setting up that layer of testing? 🙏🏼🙃

waterlink commented 7 years ago

@jjb Sounds reasonable!