Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

fixes a failing test in actionpack #3641

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants

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
Contributor

josevalim commented Nov 16, 2011

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

Member

indirect commented Nov 16, 2011

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)

Member

jonleighton commented Nov 16, 2011

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

@likethesky likethesky referenced this pull request in jruby/jruby May 30, 2013

Closed

IPv4 stack should be preferred to IPv6 #775

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment