Fix InheritanceTest#test_scope_inherited_properly implementation bugs

[mtodd]

I've updated the InheritanceTest#test_scope_inherited_properly test to trigger a reflection on the associations (which ActiveRecord::Relation does lazily). This causes the following error to be raised, indicating the test is not properly testing scope inheritance:

ActiveRecord::ConfigurationError: Association named 'account' was not found on Company; perhaps you misspelled it?

This PR fixes #21993 by fixing the pluralization of the joins clause on the Company.of_first_firm scope.

Inheritance is indeed properly working, just improperly tested. 😄

[rails-bot]

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @carlosantoniodasilva (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[mtodd]

Hmm, would like to see mtodd@b780f95 built and fail as expected.

[mtodd]

Not sure if this was the original intent and a simple typo, or if the association for the has_one :account is failing to be inherited.

[mtodd]

#21994 (comment) still applies (RE: JoinDependency)

[arthurnn]

thanks , merged via 60c9701

[rafaelfranca]

That broke the build so I reverted it.

[mtodd]

For the record: the failing build referenced includes https://travis-ci.org/rails/rails/jobs/87585808#L1309-L1309

@rafaelfranca @arthurnn looks like Company's accounts association was removed in @rafaelsales' commit rafaelsales@4f21d42 (merged in 4f21d42) very recently because it was missing an ID in the schema. So that would be the simple answer why it started failing (though reading through the history I'm surprised my branch didn't have that change).

@rafaelfranca @arthurnn who would know best about the current implementation of scope inheritance and this test in particular? @sikachu backported @Ben-M's patch for this test a while back, though I'm not sure how much the implementation has changed or whether the test was failing correctly in the first place (before the fix was applied), if the association in Company.of_first_firm is supposed to be plural or singular, or what...

In fact, I'm noticing that the Company.of_first_firm definition provides a where('firms.id' => 1) but there's no firms table since Firm is an STI class of Company. Does it matter for the test? Clearly not, since we're just testing that the scope is inherited, not whether the scope is correct.

Following that line of reasoning, it would seem that it doesn't matter if the association actually exists at all, just that both Company, the class that defines of_first_firm, and Client, the class that inherits of_first_firm, both respond to that method without exploding. This means that the test is fine as is, we don't need to actually materialize the SQL validating the associations in the scope, just that the subclass responds to the method.

Am I understanding the situation correctly? Is this kind of test or test environment acceptable?

If this is the case, I don't mind adding a comment in to explain as much for anybody who stumbles upon the test and wonders what the intent was, and offers a warning that the actual scope itself doesn't work.

[mtodd]

Opened #22084 with a simple documentation change, assuming I'm understanding the nature of the test correctly.