fixes a failing test in actionpack

[quackingduck]

The test on line 120 of actionpack/test/controller/new_base/render_template_test.rb:

test "rendering a template with error properly excerts the code" do
  get :with_error
  assert_status 500
  assert_match "undefined local variable or method `idontexist'", response.body
end

is currently failing on master. The test expects to see the message attached to the exception object (and a stack trace) but instead it gets the contents of the fixture 500.html.

It looks like the RoutedRackApp instance created for ActionDispatch::IntegrationTest in actionpack/test/abstract_unit.rb initializes the ActionDispatch::ShowExceptions middleware but doesn't explicitly tell it to consider all requests as local.

One way to way to make the test pass is to change line 167 of actionpack/test/abstract_unit.rb from:

middleware.use "ActionDispatch::ShowExceptions"

to

middleware.use "ActionDispatch::ShowExceptions", true

then all the requests in the test cases that subclass from ActionDispatch::IntegrationTest would be considered local.

Alternatively we could alter the logic in actionpack/lib/action_dispatch/http/request.rb that decides whether or not a request is local. Right now both Request#remote_addr and Request#remote_ip have to match one of [/^127\.0\.0\.\d{1,3}$/, "::1", /^0:0:0:0:0:0:0:1(%.*)?$/]. In the test cases Request#remote_addr is 127.0.0.1 but Request#remote_ip is an empty string. Changing the logic from:

LOCALHOST.any? { |local_ip| local_ip === remote_addr && local_ip === remote_ip }

to:

LOCALHOST.any? { |local_ip| local_ip === remote_addr || local_ip === remote_ip }

would make the test pass and all other test requests would be considered local.

In this patch I just updated the test to send the loopback IP with the request as it seemed the least intrusive way to get it to pass and can have no effect on any of the other tests:

test "rendering a template with error properly excerpts the code" do
  get :with_error, {}, { 'action_dispatch.remote_ip' => "127.0.0.1" }
  assert_status 500
  assert_match "undefined local variable or method `idontexist'", response.body
end

[josevalim]

Maybe another solution to this problem is to call remote_ip.to_s? I am feeling a bit uncomfortable with all these changes... One error could mean having the local exceptions page showing in production to everyone. /cc @indirect

[indirect]

Please don't change the tests! I just need to fix GetIp in the middleware.

On Nov 15, 2011, at 9:25 PM, José Valimreply@reply.github.com wrote:

Maybe another solution to this problem is to call remote_ip.to_s? I am feeling a bit uncomfortable with all these changes... One error could mean having the local exceptions page showing in production to everyone. /cc @indirect

---

Reply to this email directly or view it on GitHub:
#3641 (comment)

[jonleighton]

Closing this as @indirect is working on a fix that doesn't change the tests.