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 regression tests for keys on nested associations #15999

Conversation

eileencodes
Copy link
Member

This adds the regressions tests from PR #15893 to master. It's checking that both strings and symbols are accepted as keys for nested associations.

@@ -412,6 +412,26 @@ def test_join_table_can_be_overridden
assert_equal 'product_categories', reflection.join_table
end

def test_reflection_association_accepts_symbols_as_keys
Copy link
Contributor

Choose a reason for hiding this comment

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

This test name reads strangely to me. Is there a name we can use that better expresses what we're testing? Alternatively, can we test this at a more granular level to make it clearer that what is described in the test name is actually what we're testing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you elaborate what you mean by test at a more granular level?

Copy link
Member Author

Choose a reason for hiding this comment

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

@sgrif not sure if this gives more background or clarify into the test - but it came from this issue (#15671).

I'm definitely not against improving the test further, btw. Just want you to see where I was coming from in making the tests.

What do you think a better test name would be? 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

test_includes_accepts_strings test_includes_accepts_symbols

Copy link
Contributor

Choose a reason for hiding this comment

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

And by test at a more granular level, I meant actually passing a string or symbol to the reflection method in question, if that's what we want to test.

This adds the regressions tests from issue rails#15893 to
master. It's checking that both strings and symbols are
accepted as keys for nested associations.
@eileencodes
Copy link
Member Author

@sgrif I changed the name of the first tests tests and added tests specifically for reflect_on_association. I feel like this makes sense to have the lower and higher level. Open to discussion though. 😸

rafaelfranca added a commit that referenced this pull request Jul 1, 2014
…eys-on-nested-associations

Add regression tests for keys on nested associations
@rafaelfranca rafaelfranca merged commit ad778bc into rails:master Jul 1, 2014
@sgrif
Copy link
Contributor

sgrif commented Jul 1, 2014

@eileencodes I agree completely. Lower level will give the failure I'd want to see if I change this later, higher level helps inform me where it's used so I know where to look if I change the behavior.

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.

None yet

3 participants