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.

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

There should be documentation on how best to verify controller responses #1437

Closed
NullVoxPopuli opened this issue Jan 16, 2016 · 21 comments
Closed

Comments

@NullVoxPopuli
Copy link
Contributor

For example, lets say I want to test my index action, it returns a list of items.
But instead of having to do:

get :index
json = JSON.parse(response.body)
data = json['data']
expect(data.count).to eq 2
expect(data.first['attributes']['id']).to eq "23"

it would be nice to have an easily navigable JSON API document object, maybe resulting in this kind of test assertion

get :index
doc = AMS::Deserialize(response.body)
expect(doc.count).to eq 2
expect(doc.first.id).to eq "23"

So, I guess this issue may need to be broken in to two issues, but I'd like:

  • documentation on recommended ways to test serialized responses from controllers
  • test helpers to make parsing the raw response.body

I've been kind of away from things for a while, but If this idea is supported, and wanted, I could work on it.

@natematykiewicz
Copy link

I use RSpec and like to add this in my test environment:

class ActionDispatch::TestResponse
  def parsed_body
    @parsed_body ||= case self.content_type
    when Mime::JSON ; self.body.presence && JSON.parse(self.body, :symbolize_names => true)
    else ; raise(NotImplementedError, "Parsing instructions for #{self.content_type} not defined.")
    end
  end
end

Then I use response.parsed_body in my tests. I know it's not packed into AMS, but it definitely helps me.

I like to use request specs for the majority of my API testing, and then add some model tests for complex logic.

Ex:

subject { get '/docs'; response }

it { expect(subject.status).to eq(200) }
it { expect(subject.content_type).to eq(Mime::JSON) }
it { expect(subject.parsed_body).to eq([{:id => '33'},{:id => '44'}]) }

(I have some shared_examples that help me cut down on some of that boilerplate.)

Writing the tests to test the response as a whole and also test the API through the paths instead of a specific controller/action, tests the full app. Then, regardless of what you use to generate your JSON (AMS, JBuilder, etc), your tests still work. You can even completely refactor everything and know that your new code works. I got burned a few too many times by writing too many unit tests and not enough full stack tests. Request specs make it really easy to refactor your code.

I also can be a bit cynical in my testing. I like to test the entire response because, especially when params are involved, I don't want to assume that the rest of the JSON is the way I'd expect just because a different test with different params had the attributes I expected.

I know this doesn't directly answer what you were asking about in your post, but I hope it was helpful in some way.

Now that I get to the end of writing this, I'm reminded of the Hashie gem. It sounds a bit like what you want. https://github.com/intridea/hashie

If you return Hashie::Mash.new(@parsed_body) in that parsed_body method, your response would have the method calls you'd like. Just know that Hashie is slow compared to a standard hash. Doing something like this will slow down your test suite.

@NullVoxPopuli
Copy link
Contributor Author

I think I like the idea behind what you're doing there.
In addition,
maybe I could also make rspec matchers that would behave something like this:

expect(parsed_response).to have_attribute(:name, "the name value")

or

expect(parsed_response).to have_error_on_field(:field_name)

that way, there is less hackery which in itself could be prone to error.

@beauby
Copy link
Contributor

beauby commented Jan 18, 2016

@NullVoxPopuli: IMO, the serializers should be tested separately (using AMS::SerializableResource.new(post).serializable_hash and testing the returned hash's equality against an expected hash), and integration tests should be done kinda the same way (i.e. testing equaity of the returned JSON document with a pre-established expected JSON document).

@NullVoxPopuli
Copy link
Contributor Author

So, @beauby are you saying the returned value from the controller should be tested against the value of the serialized result?

maybe something like:

expect(response.body).to be_the_serialization_of(whatever_model)

and that matcher, would be the SerializableResource.new(whatever_model).serializable_hash

something like that?

how would you write a test for the create action?

@beauby
Copy link
Contributor

beauby commented Jan 18, 2016

I wouldn't rely on AMS to generate the expected output. I'd have a manually typed hash, which I'd compare against the parsed response document.

@NullVoxPopuli
Copy link
Contributor Author

why is that? just if there is a bug in AMS it would go unnoticed by the tests?

any other way, I feel is kinda cumbersome

@beauby
Copy link
Contributor

beauby commented Jan 18, 2016

Because otherwise you're testing AMS's output against itself, which has very little value.

@NullVoxPopuli
Copy link
Contributor Author

Well, it ensures that all the logic performed for the request has returned as expected. Like, there could be permissions or scopes involved, ya know?

@bf4
Copy link
Member

bf4 commented Jan 18, 2016

Depends if it's a unit test or integration

@bf4
Copy link
Member

bf4 commented Jan 18, 2016

I have a pr to verify serialization that I'll make someday... Right now testing agajnst serializableresource should be good enough... I like to also write out serialization results to a fike and check for regressions

@bf4
Copy link
Member

bf4 commented Jan 18, 2016

Great issue! 💯

@NullVoxPopuli
Copy link
Contributor Author

For my specific case, It's integration. I have all of my controllers as empty as possible, so I can unit test smaller chunks of logic, via my skinny_controllers gem.

:-)

I was thinking about adding an rspec matcher addition, if that would be of interest -- though, @bf4, I'd like to know what you have planned for verifying serialization.

This is something that would make my life easier on my project, so I'm willing to help you on it, if it is what I think it might be.

@natematykiewicz
Copy link

Another good reason to test hash vs hash instead of by using the AMS code is, my tests won't have to change when AMS completely changes its syntax. AMS has gone through some hefty changes over the past few versions, so hash vs hash can remain constant when I go to update a "major" (0.x) version.

Another reason I like to avoid testing against the output of something like serializable resource is that I would then have to make sure my tests have the correct includes / excludes based on the user's permissions and params provided in the request. Testing my code with the code that uses it just doesn't feel good to me.

Otherwise, my spec would need to be something like:
SerializableResource.new(whatever_model, :include => [some_stuff], :fields => [other_things], :root => 'non_default_root]).serializable_hash

But, you guys can do what you want. :)

@bf4
Copy link
Member

bf4 commented Jan 18, 2016

I've left links around what rspec stuff I wrote back around rc1. It should be adaptable to presenr patterns... Injust haven't done that yet

Re

Otherwise, my spec would need to be something like:
SerializableResource.new(whatever_model, :include => [some_stuff], :fields => [other_things], :root => 'non_default_root]).serializable_hash
Aren't those the conditions you are serializing with?

B mobile phone

On Jan 18, 2016, at 2:10 PM, Nate Matykiewicz [email protected] wrote:

Otherwise, my spec would need to be something like:
SerializableResource.new(whatever_model, :include => [some_stuff], :fields => [other_things], :root => 'non_default_root]).serializable_hash

@natematykiewicz
Copy link

Aren't those the conditions you are serializing with?

That depends on some complex logic. ;)

@flynfish
Copy link

You can test serialization like so: https://robots.thoughtbot.com/validating-json-schemas-with-an-rspec-matcher

@remear
Copy link
Member

remear commented Mar 17, 2016

See #1470, #1270, #1248

@remear remear closed this as completed Mar 17, 2016
@remear
Copy link
Member

remear commented Mar 17, 2016

@adaam2
Copy link

adaam2 commented May 17, 2018

@remear The link above is broken.

@bf4
Copy link
Member

bf4 commented May 17, 2018

@adaam2 Yeah, it's now on the 0-10 branch instead of master. Or you can find a commit from 2016 and stick it in there instead of master

@adaam2
Copy link

adaam2 commented May 21, 2018

@bf4 Perfect, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants