Skip to content
New issue

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

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated isolated tests to assert correct behavior. #2010

Merged
merged 10 commits into from
Dec 25, 2016
Merged

Updated isolated tests to assert correct behavior. #2010

Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8d18374
Updated isolated tests to assert correct behavior.
akshah123 Dec 21, 2016
921ffac
Made updates as requested. Added back failure test to check for 500 …
akshah123 Dec 21, 2016
baf58c7
Fixed whitespace issues.
akshah123 Dec 21, 2016
57a4fc2
Removed unused parameter.
akshah123 Dec 22, 2016
2debd58
Added log command so i can figure out why it's failing on some versio…
akshah123 Dec 22, 2016
b712de3
Fixed up the tests so that it will pass with all version of rails.
akshah123 Dec 22, 2016
377ea05
Fixed up the test so it only checks this condition when it is possibl…
akshah123 Dec 22, 2016
c4a27b2
Added check to get unsafe params if rails version is great than 5
akshah123 Dec 23, 2016
822e1b2
Changed the tests to be explicit about exact serialization response.
akshah123 Dec 23, 2016
ffde106
Removed require for pry
akshah123 Dec 23, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 16 additions & 15 deletions test/active_model_serializers/register_jsonapi_renderer_test_isolated.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ class << self
end

def render_with_jsonapi_renderer
author = Author.new(params[:data][:attributes])
unlocked_params = Rails::VERSION::MAJOR >= 5 ? params.to_unsafe_h : params
attributes = unlocked_params[:data].present? ? unlocked_params[:data][:attributes] : {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I gather Author.new(params[:data][:attributes]) is failing for some reason. perhaps we should instead author = Author.new(params.require(:data).require(:attributes))? I'm not sure what the need is to cast the ActionController::Parameters or when data wouldn't be present.

In actual usage I would use the JsonApi deserializer here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right that params[:data] is not set at all and the reason is that app doesn't know how to parse provided data.

.require will not help since it doesn't see data at all. Also, require will cause problem when jsonapi is not registered since it doesn't see the actual value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, so params is {} ? Then that's probably still a good failure :)

author = Author.new(attributes)
render jsonapi: author
end

Expand Down Expand Up @@ -59,18 +61,12 @@ def test_jsonapi_parser_not_registered
end

def test_jsonapi_renderer_not_registered
expected = {
'data' => {
'attributes' => {
'name' => 'Johnny Rico'
},
'type' => 'users'
}
}
payload = '{"data": {"attributes": {"name": "Johnny Rico"}, "type": "authors"}}'
headers = { 'CONTENT_TYPE' => 'application/vnd.api+json' }
post '/render_with_jsonapi_renderer', params: payload, headers: headers
assert expected, response.body
assert_equal 500, response.status
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make an assertion about the nature of the failure by matching on the response.body?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't get response.body to show the exception. It kept returning empty body. I think this probably has something to do with it failing on render.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good.

can you add an assertion for the empty body?

If you know what the exception actually, was, would love for you to paste it and stack trace here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actual exception message:

Missing template json_api_renderer_test/test/render_with_jsonapi_renderer with {:locale=>[:en], :formats=>[:html], :variants=>[], :handlers=>[:raw, :erb, :html, :builder, :ruby]}. Searched in:

backtrace

["test/active_model_serializers/register_jsonapi_renderer_test_isolated.rb:19:in `render_with_jsonapi_renderer'",
 "test/active_model_serializers/register_jsonapi_renderer_test_isolated.rb:68:in `test_jsonapi_renderer_not_registered'"]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome! thanks!

assert_equal '', response.body
assert response.request.env['action_dispatch.exception'].is_a?(ActionView::MissingTemplate) if response.request.present?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when is response.request.present? not true?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I can tell, it changes on a specific rails version. At some 4.x version, we start getting response.request but we dont' have that for 4.1.

end

def test_jsonapi_parser
Expand Down Expand Up @@ -113,16 +109,21 @@ def test_jsonapi_parser_registered
def test_jsonapi_renderer_registered
expected = {
'data' => {
'attributes' => {
'name' => 'Johnny Rico'
},
'type' => 'users'
'id' => 'author',
'type' => 'authors',
'attributes' => { 'name' => 'Johnny Rico' },
'relationships' => {
'posts' => { 'data' => nil },
'roles' => { 'data' => nil },
'bio' => { 'data' => nil }
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#2010 (comment)

}
}

payload = '{"data": {"attributes": {"name": "Johnny Rico"}, "type": "authors"}}'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we should include a uuid here. how about "id": ""36c9c04e-86b1-4636-a5b0-8616672d1765" will make the expectation simpler. JSON API recommends that if a client wants to create an id on post, to use a uuid.

You've been working on this PR a while now, and the main goal of it is done. Thanks! I can do some of the other things I'd like in a followup :)

headers = { 'CONTENT_TYPE' => 'application/vnd.api+json' }
post '/render_with_jsonapi_renderer', params: payload, headers: headers
assert expected, response.body
assert_equal expected.to_json, response.body
end

def test_jsonapi_parser
Expand Down