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

Sporadic test failures with SupportsFeatureMixin #23152

Closed
agrare opened this issue Aug 15, 2024 · 6 comments · Fixed by #23190
Closed

Sporadic test failures with SupportsFeatureMixin #23152

agrare opened this issue Aug 15, 2024 · 6 comments · Fixed by #23190

Comments

@agrare
Copy link
Member

agrare commented Aug 15, 2024

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
Copy link
Member Author

agrare commented Aug 15, 2024

cc @kbrock

@Fryguy
Copy link
Member

Fryguy commented Sep 13, 2024

@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
Copy link
Member

kbrock commented Sep 17, 2024

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 rspec/rspec-mocks#1568 which states the same problem. When I find a solution, I'll share with them.

@Fryguy
Copy link
Member

Fryguy commented Sep 17, 2024

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
Copy link
Member

kbrock commented Sep 17, 2024

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
Copy link
Member

kbrock commented Sep 17, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants