MONGOID-5654 [Monkey Patch Removal] Move Hash#__consolidate__ into Mongoid::Contextual

[johnnyshields]

Fixes MONGOID-5654

Hash#__consolidate__ monkey-patch method is used only once in the entire Mongoid codebase, in a private capacity inside Contextual::Mongo class. There is no reason to monkey-patch the kernel Hash class for this.

This PR does the following:

• Move the method into Contextual::Mongo as a private method.
• Move the associated Hash#mongoize_for private method, which is only called once inside __consolidate__
• Rename and refactor the methods.

• TODO: Move private method tests to cover a public method behavior.

The method name has __ prefix so we can assume this is an internal only method that is safe to remove.

[johnnyshields]

The failing spec for ruby-2.7 is not related to this PR.

[jamis]

@johnnyshields - I love the idea of pulling this out of Hash. However, when it comes to testing a private method, it raises a bit of a red flag for me. I think that if the method needs to be tested in isolation, it should probably be pulled out into its own module, much like you did for Embedded.traverse. What do you think?

[johnnyshields]

Hmm... I interpret the guideline as "private methods are not required to be tested, but it is ok to test them if we judge it benefits stability." Private methods make future refactoring much easier. Making a method public solely because we'd like to test it would be "the tail wagging the dog." Since we already have the tests, let's keep them.

Re: Embedded.traverse, the method needed to be public because it was called by multiple unrelated classes.

I am all for refactoring Mongoid::Contextual into smaller classes, but it's beyond the scope of this PR.

[jamis]

I understand your point, but I still am strongly opposed to writing tests for methods with private visibility. I know there are instances of this in the Mongoid codebase, but when I encounter them, I've been rewriting them.

If you feel delegation is too heavy-handed for this feature, there are a couple of other options you might consider:

1. Make the method public, but prefix the name with an underscore and document it as @api private. The tests can then invoke it without resorting to send.
2. Instead of testing the method directly, test it indirectly by observing its effects in other, publicly exposed methods.

Also, just to clarify my original point: moving the method to a new module so it can be tested in isolation does not mean the new module and method must be (or even should be) part of Mongoid's public API. In many instances, I think it makes sense to document the new module and/or method with @api private. As you mentioned, it definitely makes refactoring easier.

There are probably other ways to approach this, as well, but adding new tests (or refactoring old tests) that directly invoke private methods is a non-starter for me.

[johnnyshields]

@jamis would you accept tests for the public update_documents method which use RSpec's mock expectation syntax instead? Specifically I would be calling update_documents then asserting the view (which is a Mongo Ruby Driver object) is receiving a certain argument hash. I think this is fair game, what do you think?

[jamis]

@johnnyshields -- yeah, that sounds reasonable. Thanks for meeting me halfway. :)

[johnnyshields]

OK cool, will do. By the way, most of the monkey patch PRs do not include private method tests.

[jamis]

@johnnyshields -- I've opened a new PR that builds on your work here (#5740), and I'm going to close this PR in favor of that one. I've described the reasoning for it on that PR. Thanks for your work on this!