ManageIQ / manageiq

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

Sporadic test failures with SupportsFeatureMixin #23152

Closed agrare closed 1 month ago

agrare commented 2 months ago

There appear to be some sporadic test failures between supports_feature_mixin_spec and a number of other specs which test ExtManagementSystem classes.


Failures:

  1) BlacklistedEvent.seed does not re-seed existing event filters
     Failure/Error: missing_events = ems.default_blacklisted_event_names - (existing[ems.name] || [])

     NoMethodError:
       undefined method `default_blacklisted_event_names' for ProviderA::ExtManagementSystem:Class
     # ./app/models/blacklisted_event.rb:27:in `block in seed'
     # ./app/models/blacklisted_event.rb:26:in `each'
     # ./app/models/blacklisted_event.rb:26:in `seed'
     # ./spec/models/blacklisted_event_spec.rb:28:in `block (3 levels) in <top (required)>'

  2) BlacklistedEvent.seed re-seeds deleted event filters
     Failure/Error: missing_events = ems.default_blacklisted_event_names - (existing[ems.name] || [])

     NoMethodError:
       undefined method `default_blacklisted_event_names' for ProviderA::ExtManagementSystem:Class
     # ./app/models/blacklisted_event.rb:27:in `block in seed'
     # ./app/models/blacklisted_event.rb:26:in `each'
     # ./app/models/blacklisted_event.rb:26:in `seed'
     # ./spec/models/blacklisted_event_spec.rb:12:in `block (3 levels) in <top (required)>'

  3) BlacklistedEvent.seed loads event filters
     Failure/Error: missing_events = ems.default_blacklisted_event_names - (existing[ems.name] || [])

     NoMethodError:
       undefined method `default_blacklisted_event_names' for ProviderA::ExtManagementSystem:Class
     # ./app/models/blacklisted_event.rb:27:in `block in seed'
     # ./app/models/blacklisted_event.rb:26:in `each'
     # ./app/models/blacklisted_event.rb:26:in `seed'
     # ./spec/models/blacklisted_event_spec.rb:7:in `block (3 levels) in <top (required)>'

  4) ExtManagementSystem .model_name_from_emstype
     Failure/Error: emstype = emstype.downcase

     NoMethodError:
       undefined method `downcase' for nil:NilClass
     # ./app/models/ext_management_system.rb:402:in `model_from_emstype'
     # ./app/models/ext_management_system.rb:398:in `model_name_from_emstype'
     # ./spec/models/ext_management_system_spec.rb:43:in `block (3 levels) in <top (required)>'
     # ./spec/models/ext_management_system_spec.rb:42:in `each'
     # ./spec/models/ext_management_system_spec.rb:42:in `block (2 levels) in <top (required)>'

  5) ExtManagementSystem validates type
     Failure/Error: expect { ManageIQ::Providers::BaseManager.new(:hostname => "abc", :name => "abc", :zone => FactoryBot.build(:zone)).validate! }.to raise_error(ActiveRecord::RecordInvalid)
       expected ActiveRecord::RecordInvalid but nothing was raised
     # ./spec/models/ext_management_system_spec.rb:152:in `block (2 levels) in <top (required)>'

  6) EvmDatabase.seed doesn't fail
     Failure/Error: missing_events = ems.default_blacklisted_event_names - (existing[ems.name] || [])

     NoMethodError:
       undefined method `default_blacklisted_event_names' for ProviderA::ExtManagementSystem:Class
     # ./app/models/blacklisted_event.rb:27:in `block in seed'
     # ./app/models/blacklisted_event.rb:26:in `each'
     # ./app/models/blacklisted_event.rb:26:in `seed'
     # ./lib/evm_database.rb:136:in `block (4 levels) in seed_classes'
     # ./lib/evm_database.rb:136:in `block (3 levels) in seed_classes'
     # ./lib/evm_database.rb:134:in `each'
     # ./lib/evm_database.rb:134:in `block (2 levels) in seed_classes'
     # ./lib/extensions/ar_table_lock.rb:21:in `block in with_lock'
     # ./lib/extensions/ar_table_lock.rb:15:in `with_lock'
     # ./lib/evm_database.rb:133:in `block in seed_classes'
     # ./lib/evm_database.rb:131:in `seed_classes'
     # ./lib/evm_database.rb:84:in `block in seed'
     # ./lib/evm_database.rb:68:in `with_seed'
     # ./lib/evm_database.rb:76:in `seed'
     # ./spec/lib/evm_database_spec.rb:48:in `block (3 levels) in <top (required)>'

 Failed examples:

rspec ./spec/models/blacklisted_event_spec.rb:20 # BlacklistedEvent.seed does not re-seed existing event filters
rspec ./spec/models/blacklisted_event_spec.rb:11 # BlacklistedEvent.seed re-seeds deleted event filters
rspec ./spec/models/blacklisted_event_spec.rb:6 # BlacklistedEvent.seed loads event filters
rspec ./spec/models/ext_management_system_spec.rb:41 # ExtManagementSystem .model_name_from_emstype
rspec ./spec/models/ext_management_system_spec.rb:144 # ExtManagementSystem validates type
rspec ./spec/lib/evm_database_spec.rb:47 # EvmDatabase.seed doesn't fail

All of these failures reference ProviderA::ExtManagementSystem:Class and I only see ProviderA in spec/models/mixins/supports_feature_mixin_spec.rb

[ref]

agrare commented 2 months ago

cc @kbrock

Fryguy commented 1 month ago

@kbrock @jrafanie I figured out a simple reproducer for (part of) this:

SPEC_OPTS="--seed 3896" be rspec spec/models/mixins/supports_feature_mixin_spec.rb spec/models/blacklisted_event_spec.rb
kbrock commented 1 month ago
irb # not rails console

class A ; end     # => nil
class B < A ; end #=> nil
Object.send(:remove_const, :B) #=> B
B # uninitialized constant B (NameError)
A.subclasses.first # => B
A.subclasses.reject! { |x| x.name == "B" }
A.subclasses.first # => B

This was introduced to ruby 3.1, and the changes were incorporated in the rails 7.0 DependencyTracker. So depending upon the version of ruby running, DependencyTracker#subclasses can simply point to Class#subclasses.

Only 1 spec is causing this issue: supports_feature_mixin_spec.rb:318

I'm looking through the ruby code to see if there is a way to call the internal rb_class_remove_from_super_subclasses

of note, rspec has https://github.com/rspec/rspec-mocks/issues/1568 which states the same problem. When I find a solution, I'll share with them.

Fryguy commented 1 month ago

DescendantsTracker documents it, but doesn't really say if and when it will be used https://api.rubyonrails.org/classes/ActiveSupport/DescendantsTracker.html

This module provides an internal implementation to track descendants which is faster than iterating through ObjectSpace.

However Ruby 3.1 provide a fast native +Class#subclasses+ method, so if you know your code won’t be executed on older rubies, including ActiveSupport::DescendantsTracker does not provide any benefit.

kbrock commented 1 month ago

Well, descendants_tracker.rb is pretty clear:

module ActiveSupport
  # This module provides an internal implementation to track descendants
  # which is faster than iterating through ObjectSpace.
  module DescendantsTracker
    if RubyFeatures::CLASS_SUBCLASSES # RUBY_VERSION >= "3.1"
      # [...]
      class << self
        #[...]
        def subclasses(klass)
          klass.subclasses
        end

        def descendants(klass) # think this is deprecated
          klass.descendants
        end
      end
    end
  end
end
kbrock commented 1 month ago

I was getting sporadic results when calling 3.times { GC.start }. Sometimes subclasses returns the class and other times does not. Adding in a sleep(1) ; GC.start sometimes fixed this, other times it did not.

I converted over to manually adding and removing the objects and even running these tests from pure ruby with no rails or miq patches in place. Got the same results.

So my current solution is to change the temporary ExtManagementSystem defined returned so it doesn't break the other tests.