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

Improve readability of relationships test #1904

Merged

Conversation

bf4
Copy link
Member

@bf4 bf4 commented Sep 1, 2016

#1902 was not enough, it appears

@mention-bot
Copy link

@bf4, thanks for your PR! By analyzing the annotation information on this pull request, we identified @groyoh to be a potential reviewer

@bf4 bf4 force-pushed the fix_intermittent_relationship_test_failures branch 2 times, most recently from 3e6a79d to 0bd17a6 Compare September 1, 2016 14:12
@bf4 bf4 changed the title Really fix intermittent relationship test failures Improve readability of relationships test Sep 1, 2016
@bf4 bf4 force-pushed the fix_intermittent_relationship_test_failures branch 3 times, most recently from 1f9dfaa to dd8e147 Compare September 4, 2016 19:51
end

def test_take2_relationship_simple_link
expected = {
Copy link
Member Author

@bf4 bf4 Sep 4, 2016

Choose a reason for hiding this comment

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

example of tests that test at very different levels

  1. test_relationship_simple_link
  2. test_take2_relationship_simple_link

Copy link
Contributor

Choose a reason for hiding this comment

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

what's the purpose of all the take2 tests? other than seemingly having more setup and logic to assert

Copy link
Member Author

Choose a reason for hiding this comment

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

Merging two test files; need to disambiguate

@NullVoxPopuli
Copy link
Contributor

was there anything left to do on this other than the rebase?

bf4 added 2 commits September 25, 2016 20:51
I accidentally introduced the duplication when
merging
rails-api#1543 (comment)
and they've evolved separately since then.

They both have some value and need to be combined.
@bf4 bf4 force-pushed the fix_intermittent_relationship_test_failures branch from dd8e147 to baa4716 Compare September 26, 2016 01:54
Copy link
Member Author

@bf4 bf4 left a comment

Choose a reason for hiding this comment

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

ready for review

@bf4 bf4 force-pushed the fix_intermittent_relationship_test_failures branch 2 times, most recently from 6ebdaee to 35f38dc Compare September 26, 2016 02:40
@bf4 bf4 force-pushed the fix_intermittent_relationship_test_failures branch 4 times, most recently from 5371c40 to e7e0126 Compare September 26, 2016 03:48
@bf4 bf4 force-pushed the fix_intermittent_relationship_test_failures branch from e7e0126 to 4b72742 Compare September 26, 2016 03:49

def test_relationship_with_nil_serializer
Copy link
Member Author

Choose a reason for hiding this comment

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

a relationship with a nil serializer that isn't using virtual values (i.e. doesn't need a serializer in the firsts place) isn't a supported use case

Copy link
Member Author

Choose a reason for hiding this comment

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

In practice, AMS will try to call as_json on the data object

model_attributes = { blog: Blog.new(id: 1) }
relationship_name = :blog
model = new_model(model_attributes)
actual = build_serializer_and_serialize_relationship(model, relationship_name) do
Copy link
Contributor

Choose a reason for hiding this comment

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

I really like this pattern, of defining the serializer for test in the test itself

Copy link
Contributor

@NullVoxPopuli NullVoxPopuli left a comment

Choose a reason for hiding this comment

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

Overall I really like how consistent the tests are now. :-)

@NullVoxPopuli NullVoxPopuli merged commit c69855b into rails-api:master Sep 26, 2016
@NullVoxPopuli NullVoxPopuli deleted the fix_intermittent_relationship_test_failures branch September 26, 2016 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants