Skip to content

Updated isolated tests to assert correct behavior. #2010

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

Merged
merged 10 commits into from
Dec 25, 2016
Merged

Updated isolated tests to assert correct behavior. #2010

Changes from 3 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
19 changes: 7 additions & 12 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,8 @@ class << self
end

def render_with_jsonapi_renderer
author = Author.new(params[:data][:attributes])
attributes = params[:data].present? ? 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.

why? how could this not be set and why would we want that fallback if not set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is because when adapter is not set as jsonapi, params are not set. This is there so that it fails on render and not on trying to set Author object. Since we use the same controller in case where it should succeed and fail, I wanted to make sure this part doesn't cause a failure.

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

Expand Down Expand Up @@ -64,13 +65,13 @@ def test_jsonapi_renderer_not_registered
'attributes' => {
'name' => 'Johnny Rico'
},
'type' => 'users'
'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.

why? type looks correct to me

Copy link
Contributor Author

@akshah123 akshah123 Dec 22, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I just wanted it to match what was being returned from controller. I don't have an issue with changing it back to "users" just thought that this way it would match.

}
}
payload = '{"data": {"attributes": {"name": "Johnny Rico"}, "type": "authors"}}'
headers = { 'CONTENT_TYPE' => 'application/vnd.api+json' }
post '/render_with_jsonapi_renderer', params: payload, headers: headers
Copy link
Member

Choose a reason for hiding this comment

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

so, this should have raised an exception when hitting render_with_jsonapi_renderer which calls render jsonapi: json but there's no jsonapi renderer registered. good test to have

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the problem is that it raises an exception while trying to create a new author since params are not parsed correctly without this. So, I can setup another api method.

I think i have a way to change controller to test this case. Let me take a shot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bf4 i can't get the test controller to return the actual exception. I tried using rescue_from but i think since the error is with render it just returns 500 with blank body.

I think for now, I am just going to assert that response.status = 500

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!

end

def test_jsonapi_parser
Expand Down Expand Up @@ -111,18 +112,12 @@ def test_jsonapi_parser_registered
end

def test_jsonapi_renderer_registered
expected = {
'data' => {
'attributes' => {
'name' => 'Johnny Rico'
},
'type' => 'users'
}
}
expected = { 'name' => 'Johnny Rico' }

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, JSON.parse(response.body)['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.

Why only checking part of response. did this fail when changing assert to assert_equal? perhaps should have be expected.to_json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it did fail. The problem is that response.body contains a lot of stuff then just data.attributes and type. It includes relationships, id and other things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here is the actual response.body

{
    "data": {
        "id": "author",
        "type": "authors",
        "attributes": {
            "name": "Johnny Rico"
        },
        "relationships": {
            "posts": {
                "data": null
            },
            "roles": {
                "data": null
            },
            "bio": {
                "data": null
            }
        }
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

Oh, this is because of the PORO Author having relationships that are explicit now, whereas before they were sort of squishy...

Fix is we should be explicit what our model and serializer are:

class JsonApiRendererTest < ActionDispatch::IntegrationTest
  include ActiveSupport::Testing::Isolation
  class Author < ::Model
    attributes :name
  end
  class AuthorSerializer < ActiveModel::Serializer
    type: 'users'
    attribute :id
    attribute :name
  end

I think that's a better expectation, no?

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 think so. But I can't get your code to work. The dependency required is getting a bit too much. For now, i have updated expected object to include all relationships so that we check for exact output.

end

def test_jsonapi_parser
Expand Down