Sending JSON in functional tests does not populate the params hash #13135

Closed
ream88 opened this Issue Dec 2, 2013 · 30 comments

Comments

Projects
None yet
@ream88
Contributor

ream88 commented Dec 2, 2013

test 'sending raw JSON does not populate the params hash' do
  @request.headers['Content-Type'] = 'application/json'
  post :create, { key: "value" }.to_json

  assert_equal "value", params['key'] # => Will fail!
end

will result in following params hash:

{"controller"=>"posts", "action"=>"create"} # Hence the missing key-value pair.

However sending the corresponding request with curl or other apps, it works like intended. I even asked on StackOverflow, however no answer yet. Looks like a bug for me.

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Dec 2, 2013

Member

Hi @haihappen, you specified the Content-Type: application/json header in your curl command but you don't have this in your test, so that's might be why it's not working.

Member

chancancode commented Dec 2, 2013

Hi @haihappen, you specified the Content-Type: application/json header in your curl command but you don't have this in your test, so that's might be why it's not working.

@ream88

This comment has been minimized.

Show comment
Hide comment
@ream88

ream88 Dec 2, 2013

Contributor

I did it with @request.headers['Content-Type'] = 'application/json'and by passing the format: :json option, both didn't work. Btw updated the issue and added the header.

Contributor

ream88 commented Dec 2, 2013

I did it with @request.headers['Content-Type'] = 'application/json'and by passing the format: :json option, both didn't work. Btw updated the issue and added the header.

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Dec 2, 2013

Member

In a functional test there's no middleware stack - everything is mocked and the controller action is called directly. Just pass the parameters hash you want to test without converting it to JSON first.

Member

pixeltrix commented Dec 2, 2013

In a functional test there's no middleware stack - everything is mocked and the controller action is called directly. Just pass the parameters hash you want to test without converting it to JSON first.

@pixeltrix pixeltrix closed this Dec 2, 2013

@ream88

This comment has been minimized.

Show comment
Hide comment
@ream88

ream88 Dec 2, 2013

Contributor

Ok, the only problem is:

I have a before_filter which checks for a Signature based on the raw request body, much like the same technique used by AWS. What is the best way to test this before_filter, just rely on integration tests and skip it in functional tests or populating the params hash on my own (like I'm already doing).

Contributor

ream88 commented Dec 2, 2013

Ok, the only problem is:

I have a before_filter which checks for a Signature based on the raw request body, much like the same technique used by AWS. What is the best way to test this before_filter, just rely on integration tests and skip it in functional tests or populating the params hash on my own (like I'm already doing).

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Dec 3, 2013

Member

This is a bit of a hack but if you pass a string like you were doing then this is set to the raw request, however you still need to pass the parameters, e.g

form = { post: { title: 'A post title', body: 'Some text' } }
post :create, form.to_json, form.merge(format: 'json')

The format: 'json' ensures that the request is set to the correct format and before filters should still run even in functional tests. I need to check whether this broke during the Rails 2.3 -> 3.0 upgrade - we switched to parsing parameters using rack middleware and it may have not been tested properly.

Member

pixeltrix commented Dec 3, 2013

This is a bit of a hack but if you pass a string like you were doing then this is set to the raw request, however you still need to pass the parameters, e.g

form = { post: { title: 'A post title', body: 'Some text' } }
post :create, form.to_json, form.merge(format: 'json')

The format: 'json' ensures that the request is set to the correct format and before filters should still run even in functional tests. I need to check whether this broke during the Rails 2.3 -> 3.0 upgrade - we switched to parsing parameters using rack middleware and it may have not been tested properly.

@pixeltrix pixeltrix reopened this Dec 3, 2013

@ghost ghost assigned pixeltrix Dec 3, 2013

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Dec 3, 2013

Member

So it appears that passing a body as a string was added in 5b97088 but the documentation is poor since the example has no assertions and the tests don't check for the request body being parsed. I'll take a look to see if it can be made to work - it's a little tricky as it needs to merge with anything passed afterwards.

Member

pixeltrix commented Dec 3, 2013

So it appears that passing a body as a string was added in 5b97088 but the documentation is poor since the example has no assertions and the tests don't check for the request body being parsed. I'll take a look to see if it can be made to work - it's a little tricky as it needs to merge with anything passed afterwards.

@apotonick

This comment has been minimized.

Show comment
Hide comment
@apotonick

apotonick Dec 3, 2013

Contributor

The problem here is a fundamental design flaw, @pixeltrix.

If we'd actually invoke the Rails stack from top to bottom, we could make sure the environment is the same as in production. Also, this would save hundreds of LOCs in AC::TestCase where we manually set up an error-prone fake environment for the test.

Currently, this would imply creating a real HTTP request and invoking the stack.

Contributor

apotonick commented Dec 3, 2013

The problem here is a fundamental design flaw, @pixeltrix.

If we'd actually invoke the Rails stack from top to bottom, we could make sure the environment is the same as in production. Also, this would save hundreds of LOCs in AC::TestCase where we manually set up an error-prone fake environment for the test.

Currently, this would imply creating a real HTTP request and invoking the stack.

@apotonick

This comment has been minimized.

Show comment
Hide comment
@apotonick

apotonick Dec 3, 2013

Contributor

By "invoking the stack" I mean running the entire thing including Rack middlewares - which would automatically parse the request body into params.

Also, we've had severe problems in Roar where the API (!) for TestRequest would differ from a "real" Rack::Request instance (e.g. body.string).

Things would be easier and closer to real life by running the stack. What do you think?

Contributor

apotonick commented Dec 3, 2013

By "invoking the stack" I mean running the entire thing including Rack middlewares - which would automatically parse the request body into params.

Also, we've had severe problems in Roar where the API (!) for TestRequest would differ from a "real" Rack::Request instance (e.g. body.string).

Things would be easier and closer to real life by running the stack. What do you think?

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Dec 4, 2013

Member

@apotonick yeah, I know about the full stack thing - I'm always having to explain that to people. The reason for the mock environment is speed - currently running a simple controller test using AC::TestCase is 25% quicker than using AD::IntegrationTest. My not-so secret plan is to make integration tests at least 25% quicker, make AC::TestCase inherit from AD::IntegrationTest, add a legacy DSL module to handle the test API and then nuke the current functionals code. Not going to happen for 4.1 but I'm definitely planning on it for 4.2.

Can you make this work using the currently flawed code? 😄

Member

pixeltrix commented Dec 4, 2013

@apotonick yeah, I know about the full stack thing - I'm always having to explain that to people. The reason for the mock environment is speed - currently running a simple controller test using AC::TestCase is 25% quicker than using AD::IntegrationTest. My not-so secret plan is to make integration tests at least 25% quicker, make AC::TestCase inherit from AD::IntegrationTest, add a legacy DSL module to handle the test API and then nuke the current functionals code. Not going to happen for 4.1 but I'm definitely planning on it for 4.2.

Can you make this work using the currently flawed code? 😄

@apotonick

This comment has been minimized.

Show comment
Hide comment
@apotonick

apotonick Dec 4, 2013

Contributor

Awesome, @pixeltrix, I'm all up for the secret battle plan. In the mean-time I can fix this bug. I look into it tomorrow!

How do you measure speed for functional/integration test?

Contributor

apotonick commented Dec 4, 2013

Awesome, @pixeltrix, I'm all up for the secret battle plan. In the mean-time I can fix this bug. I look into it tomorrow!

How do you measure speed for functional/integration test?

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Dec 4, 2013

Member

How do you measure speed for functional/integration test?

Just implement the equivalent test in a functional test and integration test and then benchmark it in the normal way, e.g:

require 'benchmark/ips'
require "action_controller/railtie"

class BlogApplication < Rails::Application
  config.eager_load = false
  config.root = File.dirname(__FILE__)
  config.session_store :cookie_store, key: 'session'
  config.secret_key_base = 'secret'
end

BlogApplication.initialize!

BlogApplication.routes.draw do
  resources :posts, only: %w[index]
end

require 'rails/test_help'

class PostsController < ActionController::Base
  def index
    render text: 'Hello, World!'
  end
end

class FunctionalTest < ActionController::TestCase
  tests PostsController

  def test_posts_index
    get :index
    assert_equal 'Hello, World!', response.body
  end
end

class IntegrationTest < ActionDispatch::IntegrationTest
  def test_posts_index
    get '/posts'
    assert_equal 'Hello, World!', response.body
  end
end

Benchmark.ips do |x|
  runner = MiniTest::Unit.new
  functional = FunctionalTest.new('test_posts_index')
  integration = IntegrationTest.new('test_posts_index')

  x.report('Functional Test') do
    functional.run(runner)
  end

  x.report('Integration Test') do
    integration.run(runner)
  end
end

Which when I run it on my MacBook Air gives:

$ ruby test_comparison.rb 
Calculating -------------------------------------
     Functional Test        97 i/100ms
    Integration Test        62 i/100ms
-------------------------------------------------
     Functional Test     1000.4 (±6.4%) i/s -       5044 in   5.062338s
    Integration Test      624.5 (±4.2%) i/s -       3162 in   5.072164s
Run options: --seed 56593

# Running tests:

..

Finished tests in 0.017042s, 117.3571 tests/s, 234.7142 assertions/s.

2 tests, 4 assertions, 0 failures, 0 errors, 0 skips

Looking at it again it's more like 33% slower (or functionals are 50% quicker, if you prefer).

Member

pixeltrix commented Dec 4, 2013

How do you measure speed for functional/integration test?

Just implement the equivalent test in a functional test and integration test and then benchmark it in the normal way, e.g:

require 'benchmark/ips'
require "action_controller/railtie"

class BlogApplication < Rails::Application
  config.eager_load = false
  config.root = File.dirname(__FILE__)
  config.session_store :cookie_store, key: 'session'
  config.secret_key_base = 'secret'
end

BlogApplication.initialize!

BlogApplication.routes.draw do
  resources :posts, only: %w[index]
end

require 'rails/test_help'

class PostsController < ActionController::Base
  def index
    render text: 'Hello, World!'
  end
end

class FunctionalTest < ActionController::TestCase
  tests PostsController

  def test_posts_index
    get :index
    assert_equal 'Hello, World!', response.body
  end
end

class IntegrationTest < ActionDispatch::IntegrationTest
  def test_posts_index
    get '/posts'
    assert_equal 'Hello, World!', response.body
  end
end

Benchmark.ips do |x|
  runner = MiniTest::Unit.new
  functional = FunctionalTest.new('test_posts_index')
  integration = IntegrationTest.new('test_posts_index')

  x.report('Functional Test') do
    functional.run(runner)
  end

  x.report('Integration Test') do
    integration.run(runner)
  end
end

Which when I run it on my MacBook Air gives:

$ ruby test_comparison.rb 
Calculating -------------------------------------
     Functional Test        97 i/100ms
    Integration Test        62 i/100ms
-------------------------------------------------
     Functional Test     1000.4 (±6.4%) i/s -       5044 in   5.062338s
    Integration Test      624.5 (±4.2%) i/s -       3162 in   5.072164s
Run options: --seed 56593

# Running tests:

..

Finished tests in 0.017042s, 117.3571 tests/s, 234.7142 assertions/s.

2 tests, 4 assertions, 0 failures, 0 errors, 0 skips

Looking at it again it's more like 33% slower (or functionals are 50% quicker, if you prefer).

apotonick added a commit to apotonick/rails that referenced this issue Dec 9, 2013

In AC::TestCase#process, parse the request body to params as it is do…
…ne in the ParamsParser middleware. fixes #13135.

this is another work-around to make AC::TestCase behave the way a real integration test works. as stated here rails#13135 we are planning to replace AC::TestCase logic with a faster IntegrationTest. instead of mocking all the environment variables, this will invoke the Rails stack and provide an environment that's identical to one in production.
@apotonick

This comment has been minimized.

Show comment
Hide comment
@apotonick

apotonick Dec 9, 2013

Contributor

@pixeltrix I fixed it in apotonick/rails@1c38716 but this only works for format: :json environments (which should be ok for now).

@haihappen Could you test if that works for you?

Next step: speeding up IntegrationTest - this AC::TestCase is just messy.

Contributor

apotonick commented Dec 9, 2013

@pixeltrix I fixed it in apotonick/rails@1c38716 but this only works for format: :json environments (which should be ok for now).

@haihappen Could you test if that works for you?

Next step: speeding up IntegrationTest - this AC::TestCase is just messy.

@apotonick

This comment has been minimized.

Show comment
Hide comment
@apotonick

apotonick Dec 13, 2013

Contributor

@haihappen Hey brother, any news? Is that working for you?

@pixeltrix I can rebase and PR.

Contributor

apotonick commented Dec 13, 2013

@haihappen Hey brother, any news? Is that working for you?

@pixeltrix I can rebase and PR.

@ream88

This comment has been minimized.

Show comment
Hide comment
@ream88

ream88 Dec 18, 2013

Contributor

@apotonick I changed the particular tests to descend from ActionDispatch::IntegrationTest which fixed the problem for me :)

Contributor

ream88 commented Dec 18, 2013

@apotonick I changed the particular tests to descend from ActionDispatch::IntegrationTest which fixed the problem for me :)

@apotonick

This comment has been minimized.

Show comment
Hide comment
@apotonick

apotonick Dec 18, 2013

Contributor

@haihappen And, is it "slower"? Haha

@pixeltrix Should we merge this anyway? I will focus on making the integration tests faster over xmas.

Contributor

apotonick commented Dec 18, 2013

@haihappen And, is it "slower"? Haha

@pixeltrix Should we merge this anyway? I will focus on making the integration tests faster over xmas.

@apotonick apotonick referenced this issue in apotonick/roar-rails Jan 12, 2014

Closed

Sending JSON data in rspec #36

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Jan 25, 2014

Member

I'm going to push this to 4.2 - it's going to be hard enough to make sure we don't break people's existing tests without adding more hacks on top so I think it should be done as part of the refactoring

Member

pixeltrix commented Jan 25, 2014

I'm going to push this to 4.2 - it's going to be hard enough to make sure we don't break people's existing tests without adding more hacks on top so I think it should be done as part of the refactoring

@rafaelfranca rafaelfranca modified the milestones: 4.2.0, 5.0.0 Aug 18, 2014

@carlesjove

This comment has been minimized.

Show comment
Hide comment
@carlesjove

carlesjove Oct 16, 2014

Hey folks. I've spent four hours or so today figuring out what was going on with my controller test, until I reached this page and learnt it was a Rails bug… I don't know how the process works, but maybe it'd be appropiate to add a note at the docs about this? Passing params as JSON is quite common, and unless you dig a lot it's not obvious to learn that in Integration tests you can pass JSON with no problem, but not in Functional tests… Well, this is just an idea to help future folks like me :-)

Hey folks. I've spent four hours or so today figuring out what was going on with my controller test, until I reached this page and learnt it was a Rails bug… I don't know how the process works, but maybe it'd be appropiate to add a note at the docs about this? Passing params as JSON is quite common, and unless you dig a lot it's not obvious to learn that in Integration tests you can pass JSON with no problem, but not in Functional tests… Well, this is just an idea to help future folks like me :-)

@waleedasif322

This comment has been minimized.

Show comment
Hide comment
@waleedasif322

waleedasif322 Nov 1, 2014

We also had this problem. If it can be highlighted somewhere in the docs that would've been great

We also had this problem. If it can be highlighted somewhere in the docs that would've been great

@rails-bot rails-bot added the stale label Mar 20, 2015

@rails-bot

This comment has been minimized.

Show comment
Hide comment
@rails-bot

rails-bot Mar 20, 2015

This issue has been automatically marked as stale because it has not been commented on for at least
three months.

The resources of the Rails team are limited, and so we are asking for your help.

If you can still reproduce this error on the 4-2-stable, 4-1-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

This issue has been automatically marked as stale because it has not been commented on for at least
three months.

The resources of the Rails team are limited, and so we are asking for your help.

If you can still reproduce this error on the 4-2-stable, 4-1-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

@shuhaowu

This comment has been minimized.

Show comment
Hide comment
@shuhaowu

shuhaowu Jun 19, 2015

Can still reproduce on rails 4.2

Can still reproduce on rails 4.2

@rafaelfranca rafaelfranca added pinned and removed stale labels Jun 25, 2015

@jarthod

This comment has been minimized.

Show comment
Hide comment
@jarthod

jarthod Jul 31, 2015

Yep still present, no easy way to work with raw body in 4.2

jarthod commented Jul 31, 2015

Yep still present, no easy way to work with raw body in 4.2

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 9, 2015

Member

It was fixed on the master branch. Unfortunately it is not easy to backport so it will not be fixed on Rails 4

Member

rafaelfranca commented Sep 9, 2015

It was fixed on the master branch. Unfortunately it is not easy to backport so it will not be fixed on Rails 4

@cdcooksey

This comment has been minimized.

Show comment
Hide comment
@cdcooksey

cdcooksey Oct 8, 2015

I've spent about 16 hours trying to figure out why my test wasn't working, and now I'm finally finding this thread and it all makes sense.

In our case, we're using rails-api. Rather than inherit from IntegrationTest, we're just passing a non-JSON hash, and it seems to work.

It's a hideous workaround for an API, but it's faster and we can't wait until Rails 5.

For google purposes so others can find this thread:

strong parameters
rails 4
top level nested key
wrap_parameters
json minitest
empty parameters
empty params

I've spent about 16 hours trying to figure out why my test wasn't working, and now I'm finally finding this thread and it all makes sense.

In our case, we're using rails-api. Rather than inherit from IntegrationTest, we're just passing a non-JSON hash, and it seems to work.

It's a hideous workaround for an API, but it's faster and we can't wait until Rails 5.

For google purposes so others can find this thread:

strong parameters
rails 4
top level nested key
wrap_parameters
json minitest
empty parameters
empty params

@apotonick

This comment has been minimized.

Show comment
Hide comment
@apotonick

apotonick Oct 8, 2015

Contributor

I started to test with Rack-test. It is a million times faster, no magic and does exactly what you would think it does.

Contributor

apotonick commented Oct 8, 2015

I started to test with Rack-test. It is a million times faster, no magic and does exactly what you would think it does.

@cdcooksey

This comment has been minimized.

Show comment
Hide comment
@cdcooksey

cdcooksey Oct 8, 2015

Nice. 👍 Personally, I want to move to RSpec. I miss it. 😄

Nice. 👍 Personally, I want to move to RSpec. I miss it. 😄

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Oct 9, 2015

Member

@apotonick yes, you're right - if only it'd been around when integration tests were implemented in Rail 1.1 😉

Member

pixeltrix commented Oct 9, 2015

@apotonick yes, you're right - if only it'd been around when integration tests were implemented in Rail 1.1 😉

@apotonick

This comment has been minimized.

Show comment
Hide comment
@apotonick

apotonick Oct 10, 2015

Contributor

Haha I hear ya @pixeltrix but I still wonder why it never got cleaned up? It's so much code to do a very simple thing. What's the plan with this?

Contributor

apotonick commented Oct 10, 2015

Haha I hear ya @pixeltrix but I still wonder why it never got cleaned up? It's so much code to do a very simple thing. What's the plan with this?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Oct 10, 2015

Member

It is already being done in master branch

Member

rafaelfranca commented Oct 10, 2015

It is already being done in master branch

@rafaelfranca rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015

@sixty4bit

This comment has been minimized.

Show comment
Hide comment
@sixty4bit

sixty4bit Feb 22, 2017

Has this been fixed in 5.0.0?

Has this been fixed in 5.0.0?

@cdcooksey

This comment has been minimized.

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