Mixed in class methods detection should be best effort #252
Conversation
We try to do something sneaky to detect modules that are mixed into a class through inclusion of another module, where we try to infer that information from the change in the ancestor list of a test class after including the said module to that class. However, that operation is risky since a `self.included` method on the target module could potentially run any Ruby code and it turns out some code in the wild even try to require optional gems when the concern is being included. Since this detection is best effort, it should be fine to catch all kinds of `Exception`s and bail out of the operation without side-effects. Fixes: #237
That's not quite what we are doing here, though. It would have been simpler if that was the case, indeed. What we are trying to detect here is given a random module |
| mixins_from_modules[mod] = singleton_class.ancestors - before | ||
| end | ||
| rescue Exception # rubocop:disable Lint/RescueException | ||
| # this is a best effort, bail if we can't perform this |
There was a problem hiding this comment.
Might be nice to warn here, otherwise this "silent failure" could make future debugging difficult. @paracycle
There was a problem hiding this comment.
Agreed in principle, but I would like to address the few places where we unconditionally and silently rescue errors similar to this into a more uniform framework. The output is already pretty noisy and I would not like to make it more noisy by surfacing these errors as is.
I had created the #238 issue to handle that in the DSL generation part of the codebase, but will most certainly extend the same thing to other parts of the codebase too. It will probably be a file-based logging operation with an indication in the UI that there might have been some errors encountered during the operation, so best to inspect the file.
Motivation
Fixes: #237
We try to do something sneaky to detect modules that are mixed into a class through inclusion of another module, where we try to infer that information from the change in the ancestor list of a test class after including the said module to that class.
However, that operation is risky since a
self.includedmethod on the target module could potentially run any Ruby code and it turns out some code in the wild even try to require optional gems when the concern is being included.Implementation
Since this detection is best effort, it should be fine to catch all kinds of
Exceptions duringincludeand bail out of the operation without further side-effects.Tests
Added a test to verify that we can still generate module definitions even if
self.includedfails.