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

Remove subclassing hack by not subclassing EMS #23211

Merged
merged 1 commit into from
Oct 15, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 18 additions & 24 deletions spec/models/mixins/supports_feature_mixin_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,14 @@ def initialize(values = {})
end
end

let(:model) do
Copy link
Member Author

Choose a reason for hiding this comment

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

I moved this up because it was being defined after the tests that were using it.

define_model("Model", ActiveRecord::Base,
:publish => true,
:archive => true,
:delete => false,
:fake => 'We keep it real!')
end

describe ".subclasses_supporting" do
it 'detect' do
define_subclass("ProviderA", model, :fake => true)
Expand All @@ -285,14 +293,6 @@ def initialize(values = {})
end
end

let(:model) do
define_model("Model", ActiveRecord::Base,
:publish => true,
:archive => true,
:delete => false,
:fake => 'We keep it real!')
end

describe ".supporting" do
it 'detect' do
ca = define_subclass("ProviderA", model, :fake => true)
Expand All @@ -311,17 +311,21 @@ def initialize(values = {})

describe ".providers_supporting" do
it 'detect' do
providera = define_subclass("ProviderA", ExtManagementSystem)
providerb = define_subclass("ProviderB", ExtManagementSystem)
providerc = define_subclass("ProviderC", ExtManagementSystem)
# providers_supporting directly references the ExtMangementSystem model, so
# we stub it to our own model tree instead for testing.
stub_const("ExtManagementSystem", model)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Do we want to add a comment like "ExtManagementSystem is used by providers_supporting"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I could

Copy link
Member Author

Choose a reason for hiding this comment

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

Done


providera = define_subclass("ProviderA", model)
providerb = define_subclass("ProviderB", model)
providerc = define_subclass("ProviderC", model)

define_subclass(providera.name, model, :fake => true)
define_subclass(providerb.name, model, :publish => false, :delete => true, :fake => true)
define_subclass(providerc.name, model, :publish => false, :delete => true, :fake => true)

FactoryBot.create(:ext_management_system, :type => providera.name, :name => "a1")
FactoryBot.create(:ext_management_system, :type => providera.name, :name => "a2")
FactoryBot.create(:ext_management_system, :type => providerb.name, :name => "b1")
providera.create(:name => "a1")
providera.create(:name => "a2")
providerb.create(:name => "b1")

expect(model.providers_supporting(:publish).map(&:name)).to match_array(%w[a1 a2])
expect(model.providers_supporting(:delete).map(&:name)).to eq(%w[b1])
Expand All @@ -347,16 +351,6 @@ def define_supporting_class(class_name, parent, supports_values = {})
# This also helps when the active record class does not have a name.
self.table_name = "vms" if parent.ancestors.include?(ActiveRecord::Base)

# We sometimes create a temporary Ems child class (e.g.: Vm)
# When the spec finishes and the class is disposed, Ems.subclasses still returns the old class.
# It will go away after a few GC cycles, but before then, it causes BlacklistedEvents spec failures.
# Adding this method to have those specs work despite the bogus class returned from subclasses.
if parent == ExtManagementSystem
def self.default_blacklisted_event_names
[]
end
end

yield(self) if block_given?
supports_values.each do |feature, value|
case value
Expand Down