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

Add tests for meta on resource objects. #1

Merged
merged 1 commit into from
Dec 9, 2015

Conversation

rafael
Copy link

@rafael rafael commented Nov 24, 2015

@beauby what do you think about this as starting point? Are there any other tests that you think we should add? Do you think adding a new file is ok or is it a better place for these tests to live?

TODO:

  • Add serializer-level tests (testing ActiveModel::Serializer.meta and ActiveModel::Serializer#meta)
  • Adapt current test to make use of the object itself (in order to showcase the power of blocks over direct params)
  • Add test to make use of direct parameters

@beauby
Copy link
Owner

beauby commented Nov 24, 2015

@rafael Thanks a lot! I think there should be tests in test/serializers/meta_test.rb (which already exists), that would just make sure the serializer-level logic works with blocks and hashes, and in test/adapter/json_api/resource_meta_test.rb which you created.

class MetaPostSerializer < ActiveModel::Serializer
attributes :id
meta do
{ stuff: "value" }
Copy link
Owner

Choose a reason for hiding this comment

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

Would be great to make a test using the block syntax (like here) to make use of some of object's property, like

meta do
  { comments_count: object.comments.count }
end

and an other test that makes use of the direct syntax (no bloc) as follows:

meta { stuff: 'value' }

Detail: use single quotes for strings without interpolation, otherwise Rubocop will complain when we try to merge it.

@beauby
Copy link
Owner

beauby commented Nov 24, 2015

When those are taken care of, I'll be happy to merge your commit in my branch, and then get the branch merged into master!

@rafael
Copy link
Author

rafael commented Nov 25, 2015

Cool! Thanks for the feedback.

Will make this changes and update the PR.

@rafael rafael force-pushed the resource-level-meta branch from d0c5883 to e4ea512 Compare November 27, 2015 17:56
@rafael
Copy link
Author

rafael commented Nov 27, 2015

@beauby . I was not 100% I got what you meant with the serializer-level tests. I added some. Do they look good to you ?

@gaganawhad
Copy link

@beauby I took a look at this, I am not sure I follow what the serializer-level test is either. Great job here @rafael

@rafael
Copy link
Author

rafael commented Dec 6, 2015

@beauby did you have a chance to review the tests ? Do they look good to merge?

@beauby
Copy link
Owner

beauby commented Dec 9, 2015

@rafael Can't really review it right now but it looks good so I'm merging those into my branch and we'll ask people to review it on rails-api/AMS.

beauby added a commit that referenced this pull request Dec 9, 2015
Add tests for meta on resource objects.
@beauby beauby merged commit 22371aa into beauby:resource-level-meta Dec 9, 2015
@gaganawhad
Copy link

Awesome! Thanks guys!

beauby added a commit that referenced this pull request Dec 10, 2015
Add tests for meta on resource objects.
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

Successfully merging this pull request may close these issues.

3 participants