ManageIQ / manageiq

ManageIQ Open-Source Management Platform
https://manageiq.org
Apache License 2.0
1.35k stars 900 forks source link

Remove subclassing hack by not subclassing EMS #23211

Closed Fryguy closed 1 month ago

Fryguy commented 1 month ago

@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_supporting actually 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_supporting does this same type of fake model stubbing, and that method is really the meat of .providers_supporting anyway. The .providers_supporting test is really just an extension of the .provider_classes_supporting test but taking it to the next step of actually querying for the instances.

The only caveat I've seen is that technically ExtManagementSystem overrides .supported_subclasses in order to call permitted_subclasses, and so by stubbing away ExtManagementSystem we're no longer testing that strange path. However, the .provider_classes_supporting test is already ignoring this, and also the .permitted_subclasses is already tested in the ext_management_system_spec.

miq-bot commented 1 month ago

Checked commit https://github.com/Fryguy/manageiq/commit/a678a19f7f69bccad920b057e2ea977cec9d39bf with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint 1 file checked, 0 offenses detected Everything looks fine. :cookie: