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

MONGOID-5661 [Monkey Patch Removal] Add test which checks for monkey patches #5711

Merged

Conversation

johnnyshields
Copy link
Contributor

@johnnyshields johnnyshields commented Sep 3, 2023

Fixes MONGOID-5661

This PR adds a spec to check which kernel monkey patches Mongoid has added. It will help the team to remove them overtime, and will help ensure Mongoid developers do not add new monkey patches going forward.

@johnnyshields johnnyshields changed the title Add script to check monkey patches Add spec to check monkey patches Sep 3, 2023
@johnnyshields johnnyshields changed the title Add spec to check monkey patches MONGOID-5661 - [Monkey Patch Removal] Add test which checks for monkey patches Sep 3, 2023
@johnnyshields johnnyshields changed the title MONGOID-5661 - [Monkey Patch Removal] Add test which checks for monkey patches MONGOID-5661 [Monkey Patch Removal] Add test which checks for monkey patches Sep 3, 2023
@@ -13,6 +13,10 @@
end
end

after do
Symbol.undef_method(:fubar)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This spec dynamically monkey-patches Symbol, so I need to clean it up after the spec runs.

@jamis
Copy link
Contributor

jamis commented Nov 8, 2023

Hey Johnny, I like the idea behind this PR -- proactively looking for and reporting on new or removed monkey patches -- but it does not seem to play nice with the Mongoid.deprecate declarations. The source_location for deprecated methods are given a path to active_support/deprecation/method_wrappers.rb, where the wrapper methods are being declared, and thus are no longer reported as being under the mongoid source tree.

I'm not sure if there's a clean way to handle that case or not. Probably we can get by without this spec since the monkey patches in Mongoid seem to be encapsulated neatly in extension modules and are thus relatively easy to keep an eye on. Still, if you have any ideas for how to work around the deprecate issue, I'm interested.

@johnnyshields
Copy link
Contributor Author

We can just ignore deprecated methods (i.e. not whitelist them in the spec).

The point of this spec is only to verify that "no new Monkey Patch methods are inadvertently added." The spec is not intended to assert that "certain Monkey Patch methods are present."

@johnnyshields
Copy link
Contributor Author

@jamis I have added a @note comment explaining the above and have remove the deprecated methods from the list. This is ready for final review.

@johnnyshields johnnyshields force-pushed the add-script-to-check-monkey-patches branch from 43b852e to ea16412 Compare November 12, 2023 16:50
@jamis jamis merged commit 467cf48 into mongodb:master Nov 13, 2023
16 of 17 checks passed
@jamis
Copy link
Contributor

jamis commented Nov 13, 2023

Thanks Johnny. I'm not sure why, but there's just something about this test--the test itself, not the implementation--that bothers me the more I think about it. I think I'm dubious about whether it will actually add value, or just get in the way...but I'm willing to give it the benefit of the doubt for now. I've merged it. We'll see what it does for us!

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.

2 participants