From bc0151bf8a55d5f2a3a556d06c7a5bbbed12bb2d Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Thu, 4 Apr 2024 19:17:42 -0400 Subject: [PATCH] Fix supports_helper stub_supports to handle unsupported_reason We are now getting away from supports?(:supported_feature). Instead we are calling unsupported_reason(:supported_feature) It cuts down on our calls into the supports_feature_mixin. Before ====== When using stub_supports, unsupported_reason(:supported_feature) was not stubbed (and unpredictable) supports? was stubbed After ===== We are stubbing at the feature definition layer all code in unsupported_reason is run/tested all code in supports? is run/tested This puts more code under test and allows flexibility for refactoring Notes ===== Wasn't thrilled about kwarg supports, but a few tests still use that. I've changed them all but it wasn't too much work to keep this working so we can backport with minimal fuss This code better tests our features that defer to another feature --- spec/lib/supports_helper_spec.rb | 35 ++++++++++++++++--- .../container_manager/metrics_capture_spec.rb | 4 +-- .../mixins/supports_feature_mixin_spec.rb | 21 +++++++++++ spec/support/supports_helper.rb | 20 +++-------- 4 files changed, 58 insertions(+), 22 deletions(-) diff --git a/spec/lib/supports_helper_spec.rb b/spec/lib/supports_helper_spec.rb index f917f00465b9..9982dbbe9b95 100644 --- a/spec/lib/supports_helper_spec.rb +++ b/spec/lib/supports_helper_spec.rb @@ -6,33 +6,46 @@ include SupportsFeatureMixin supports :archive supports_not :delete + supports(:archive_ref) { unsupported_reason(:archive) } + supports(:delete_ref) { unsupported_reason(:delete) } end) end context "stub_supports" do - it "overrides supports from false to true" do + it "starts as false" do expect(Post.supports?(:delete)).to be false - + end + it "overrides supports from false to true" do stub_supports(Post, :delete) expect(Post.supports?(:delete)).to be true + expect(Post.unsupported_reason(:delete)).to be nil expect(Post.new.supports?(:delete)).to be true + expect(Post.new.unsupported_reason(:delete)).to be nil end it "overrides supports (string) from false to true" do - expect(Post.supports?(:delete)).to be false - stub_supports(Post, "delete") expect(Post.supports?(:delete)).to be true expect(Post.new.supports?(:delete)).to be true end + + it "overrides supports references" do + stub_supports_all_others(Post) + stub_supports(Post, :delete) + + expect(Post.new.supports?(:delete_ref)).to be true + expect(Post.new.unsupported_reason(:delete_ref)).to be nil + end end context "stub_supports_not" do - it "overrides supports from true to false" do + it "starts as true" do expect(Post.supports?(:archive)).to be true + end + it "overrides supports from true to false" do stub_supports_not(Post, :archive, "reasons") expect(Post.supports?(:archive)).to be false @@ -40,5 +53,17 @@ expect(Post.new.supports?(:archive)).to be false expect(Post.new.unsupported_reason(:archive)).to eq("reasons") end + + it "ref starts sa true" do + expect(Post.supports?(:archive_ref)).to be true + end + + it "overrides supports references" do + stub_supports_all_others(Post) + stub_supports_not(Post, :archive, "reasons") + + expect(Post.new.supports?(:archive_ref)).to be false + expect(Post.new.unsupported_reason(:archive_ref)).to eq("reasons") + end end end diff --git a/spec/models/manageiq/providers/container_manager/metrics_capture_spec.rb b/spec/models/manageiq/providers/container_manager/metrics_capture_spec.rb index b2605d270b2b..4bb7a6285418 100644 --- a/spec/models/manageiq/providers/container_manager/metrics_capture_spec.rb +++ b/spec/models/manageiq/providers/container_manager/metrics_capture_spec.rb @@ -7,7 +7,7 @@ describe "#perf_capture_all_queue" do context "with a provider not supporting metrics capture" do - before { stub_supports(ems, :metrics, :supported => false) } + before { stub_supports_not(ems, :metrics) } it "doesn't queue any targets captures" do ems.perf_capture_object.perf_capture_all_queue @@ -17,7 +17,7 @@ end context "with a provider supporting metrics capture" do - before { stub_supports(ems, :metrics, :supported => true) } + before { stub_supports(ems, :metrics) } context "with no inventory" do it "doesn't queue any targets captures" do diff --git a/spec/models/mixins/supports_feature_mixin_spec.rb b/spec/models/mixins/supports_feature_mixin_spec.rb index 8c2afb4745a4..7b34ae03d1c2 100644 --- a/spec/models/mixins/supports_feature_mixin_spec.rb +++ b/spec/models/mixins/supports_feature_mixin_spec.rb @@ -135,6 +135,27 @@ def initialize(values = {}) expect(test_inst.supports?(:dynamic_feature)).to be_truthy end + it "instance redirects work properly" do + test_class.instance_eval do + supports(:dynamic_attr) { unsupported_reason(:dynamic_operation) } + supports(:dynamic_operation) { "unsupported" } + end + + child_class = define_model(nil, test_class) + child_class.instance_eval do + supports(:dynamic_operation) { nil } + end + + # parent + expect(test_class.new.supports?(:dynamic_operation)).to be_falsey + expect(test_class.new.supports?(:dynamic_attr)).to be_falsey + + # child + expect(child_class.new.supports?(:dynamic_operation)).to be_truthy + expect(child_class.new.supports?(:dynamic_attr)).to be_truthy + expect(child_class.new.unsupported_reason(:dynamic_attr)).to eq(nil) + end + context "with dynamic child class" do let(:child_class) do define_model( diff --git a/spec/support/supports_helper.rb b/spec/support/supports_helper.rb index b399840766df..23ade9cd08da 100644 --- a/spec/support/supports_helper.rb +++ b/spec/support/supports_helper.rb @@ -4,7 +4,7 @@ module SupportsHelper # when testing a model that receives multiple supports, # put this down first to allow the other supports to work fine. def stub_supports_all_others(model) - allow_any_instance_of(model).to receive(:supports?).and_call_original + allow(model.supports_features).to receive(:[]).and_call_original end # when testing requests, ensure the model supports a certain attribute @@ -12,23 +12,13 @@ def stub_supports(model, feature = :update, supported: true) model = model.class unless model.kind_of?(Class) feature = feature.to_sym - receive_supports = receive(:supports?).with(feature).and_return(supported) - allow(model).to receive_supports - allow_any_instance_of(model).to receive_supports - - allow(model).to receive(:types_supporting).with(feature).and_return([nil] + model.descendants.map(&:name)) + allow(model.supports_features).to receive(:[]).with(feature).and_return(supported) + # TODO: verify this stub is necessary + allow(model).to receive(:types_supporting).with(feature).and_return([nil] + model.descendants.map(&:name)) if supported == true end def stub_supports_not(model, feature = :update, reason = nil) - model = model.class unless model.kind_of?(Class) - feature = feature.to_sym - - stub_supports(model, feature, :supported => false) - - reason ||= SupportsFeatureMixin.default_supports_reason - receive_reason = receive(:unsupported_reason).with(feature).and_return(reason) - allow(model).to(receive_reason) - allow_any_instance_of(model).to(receive_reason) + stub_supports(model, feature, :supported => (reason || false)) end end end