Remove subclassing hack by not subclassing EMS #23211
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fryguy
commented
Sep 27, 2024
| @@ -263,6 +263,14 @@ def initialize(values = {}) | |||
| end | |||
| end | |||
|
|
|||
| let(:model) do | |||
There was a problem hiding this comment.
I moved this up because it was being defined after the tests that were using it.
kbrock
reviewed
Oct 1, 2024
| providera = define_subclass("ProviderA", ExtManagementSystem) | ||
| providerb = define_subclass("ProviderB", ExtManagementSystem) | ||
| providerc = define_subclass("ProviderC", ExtManagementSystem) | ||
| stub_const("ExtManagementSystem", model) |
There was a problem hiding this comment.
This is the main change here.
When we call providers_supporting / and therefore Ems.subclasses - it will fetch the subclasses from our generated/stubbed tree. So we've successfully created a separte tree and no longer need to modify the real Ems tree.
There was a problem hiding this comment.
Do we want to add a comment like "ExtManagementSystem is used by providers_supporting"?
Fryguy
force-pushed
the
fix_subclasses_hack
branch
from
d44b16a
to
1601832
Compare
Fryguy
force-pushed
the
fix_subclasses_hack
branch
from
1601832
to
a678a19
Compare
|
Checked commit Fryguy@a678a19 with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@kbrock Please review.
This is an alternative to #23190 which avoids the problem by not subclassing ExtManagementSystem, and instead just using the fake parent model like the other tests do. Since it is avoiding the subclassing it also avoids any possible other contamination that may arise in the future or that we just haven't seen yet.
Because
.providers_supportingactually calls against ExtManagementSystem literally, we just have to stub_const that particular const to point to our fake model class instead. IMO, this is still a fair test because it exercises the code paths we care about. The test against.provider_classes_supportingdoes this same type of fake model stubbing, and that method is really the meat of.providers_supportinganyway. The.providers_supportingtest is really just an extension of the.provider_classes_supportingtest but taking it to the next step of actually querying for the instances.The only caveat I've seen is that technically
ExtManagementSystemoverrides.supported_subclassesin order to callpermitted_subclasses, and so by stubbing awayExtManagementSystemwe're no longer testing that strange path. However, the.provider_classes_supportingtest is already ignoring this, and also the.permitted_subclassesis already tested in the ext_management_system_spec.