Exclude stubbed classes from subclasses after teardown #1570

GCorbel · 2024-02-20T22:56:34Z

This fix #1568.

Stubbed classes are excluded from parent subclasses after each spec.

The original issue :

begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
  raise e
end

gemfile(true) do
  source "https://rubygems.org"

  gem "rspec", "3.12.0" # Activate the gem and version you are reporting the issue against.
  gem 'rspec-mocks', '3.12.6'
end

puts "Ruby version is: #{RUBY_VERSION}" # Ruby version is: 3.1.4

class Something; end

describe 'Test' do
  before(:each) do
    class A < Something; end
    stub_const('B', Class.new(Something))
  end

  it 'something' do
    puts Something.subclasses # => [B, A]
  end

  it 'something else' do
    puts Something.subclasses # => [B, B, A]
    # Only one occurence of B should be listed
  end
end

Now, Something.subclasses always return [B, A].

pirj

It’s nice that you’ve found a reliable solution.
I’m skeptical, however, to include it for everyone.
There might be some performance impact, and since this can introduce flakiness due to the GC race condition and an exception is not reassuring.

Still, this should be kept as a reference for those who experience issues with subclasses.

lib/rspec/mocks.rb

pirj · 2024-02-21T06:22:47Z

lib/rspec/mocks.rb

@@ -101,6 +101,23 @@ def self.with_temporary_scope
      end
    end

+    @@excluded_subclasses = []
+
+    def self.excluded_subclasses


Is it necessary to getobj?
There’s a potential race condition, and the weakref itself should quack just like the object itself (including the === probably to allow to remove from the list.
https://stackoverflow.com/questions/69185508/ruby-weakref-has-implicit-race-condition

Yes, otherwise the substraction wasn't working.

I changed to rescue RefError.

JonRowe · 2024-02-21T15:13:21Z

lib/rspec/mocks.rb

+    @excluded_subclasses = []
+
+    def self.excluded_subclasses
+      @excluded_subclasses.select(&:weakref_alive?).map do |ref|


Why not just do this in the actual method, then you don't have to build the list.

You mean with ?

diff --git a/lib/rspec/mocks.rb b/lib/rspec/mocks.rb index a82055dd..ad697b59 100644 --- a/lib/rspec/mocks.rb +++ b/lib/rspec/mocks.rb @@ -104,11 +104,7 @@ module RSpec @excluded_subclasses = [] def self.excluded_subclasses - @excluded_subclasses.select(&:weakref_alive?).map do |ref| - ref.__getobj__ - rescue RefError - nil - end.compact + @excluded_subclasses end def self.exclude_subclass(constant) @@ -117,7 +113,11 @@ module RSpec module ExcludeClassesFromSubclasses def subclasses - super - RSpec::Mocks.excluded_subclasses + super - RSpec::Mocks.excluded_subclasses.select(&:weakref_alive?).map do |ref| + ref.__getobj__ + rescue RefError + nil + end.compact end end Class.prepend(ExcludeClassesFromSubclasses)

JonRowe

Just placing a block on this as I don't want it merged without final say so @pirj I'm concerned about the require.

GCorbel · 2024-02-21T15:25:38Z

I’m skeptical, however, to include it for everyone. There might be some performance impact, and since this can introduce flakiness due to the GC race condition and an exception is not reassuring.

I understand your worries. I would like to find a better solution but wasn't able.

Maybe we can use a config as excluded_stubbed_const_from_subclasses or enable it by default and do a config as thread_safe_garabage_collection ?

GCorbel · 2024-02-21T15:34:30Z

The more I think, the more I'm in favor of a config flag. Maybe a warning message that say the flag have to be switched should be displayed when there is an error.

pirj · 2024-02-21T15:42:13Z

I support your idea of opting in for this with a flag.

pirj · 2024-02-21T15:44:49Z

We could conditionally require weakref if this option is on.

GCorbel · 2024-02-21T16:19:40Z

Should we have the option on by default ?

pirj · 2024-02-21T17:45:20Z

No. But we’re working in RSpec 4, and we can think about enabling it there unless performance considerations come up.

GCorbel

I refactored, added a config flag and specs. I tried to respect the existing code. Let me know what you think.

GCorbel · 2024-02-28T21:17:57Z

lib/rspec/mocks/exclude_stubbed_classes_from_subclasses.rb

I took RSpec::Mocks::MarshalExtension for example.

GCorbel · 2024-02-29T20:20:43Z

I change to run specs only for Ruby 3.1 and ahead as in https://github.com/rails/rails/blob/v7.0.8/activesupport/lib/active_support/ruby_features.rb

GCorbel · 2024-03-04T18:11:46Z

Sorry, my latest commit was completely broke. I fixed it.

GCorbel · 2024-03-04T21:15:01Z

I fixed the code for Ruby <= 2.4

GCorbel · 2024-03-05T15:47:33Z

lib/rspec/mocks/exclude_stubbed_classes_from_subclasses.rb

+
+        Class.class_eval do
+          undef subclasses_with_rspec_mocks
+          alias subclasses subclasses_without_rspec_mocks


Rubocop triggers this warning :

lib/rspec/mocks/exclude_stubbed_classes_from_subclasses.rb:47:11: W: Lint/DuplicateMethods: Method RSpec::Mocks::ExcludeStubbedClassesFromSubclasses::Class#subclasses is defined at both lib/rspec/mocks/exclude_stubbed_classes_from_subclasses.rb:38 and lib/rspec/mocks/exclude_stubbed_classes_from_subclasses.rb:47. alias subclasses subclasses_without_rspec_mocks ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

It's wrong because the method is undefined at line 46. I disabled this warning.

GCorbel · 2024-03-05T16:14:08Z

The CI pass except for "ruby-head" which is not to my commit

GCorbel · 2024-03-06T14:46:51Z

I fixed a flap on specs. Now the CI should pass.

GCorbel · 2024-03-06T17:14:57Z

I finally succeeded to install Ruby 3.4-dev locally and confirm that the error on the CI is not related to the PR. It also fail on the main branch.

GCorbel · 2024-03-25T14:56:51Z

Hello, I don't want to bother you but is it possible to have a quick review, at least on the global behavior? I'm waiting for this to be able to move forward on the project I'm working on.

pirj

Please accept my apologies for the wait. The code is mostly good, thanks!

I’d love to hear what @JonRowe says.
Do we need a benchmark to measure the overhead in reset calls?

lib/rspec/mocks/exclude_stubbed_classes_from_subclasses.rb

lib/rspec/mocks.rb

lib/rspec/mocks/exclude_stubbed_classes_from_subclasses.rb

spec/rspec/mocks/exclude_stubbed_classes_from_subclasses_spec.rb

lib/rspec/mocks/exclude_stubbed_classes_from_subclasses.rb

spec/rspec/mocks/exclude_stubbed_classes_from_subclasses_spec.rb

GCorbel · 2024-03-26T16:36:18Z

Please accept my apologies for the wait. The code is mostly good, thanks!

No problem, it's open source. You don't even have to reply.

pirj

Looks solid, thank you! 🙌

Just a few optional, subjective and cosmetic notes - please feel free to ignore.

pirj · 2024-03-27T16:47:40Z

lib/rspec/mocks/exclude_stubbed_classes_from_subclasses.rb

+        def exclude_subclass(constant)
+          require 'weakref' unless defined?(::WeakRef)
+
+          @excluded_subclasses ||= []


This is in a few places. Might be it belongs to enable!, too?

pirj · 2024-03-27T16:47:50Z

lib/rspec/mocks/exclude_stubbed_classes_from_subclasses.rb

+        end
+
+        def excluded_subclasses
+          require 'weakref' unless defined?(::WeakRef)


Ok to move this line into enable!?

I'm not sure if it's a problem but now we can do RSpec::Mocks::ExcludeStubbedClassesFromSubclasses.excluded_subclasses without ExcludeStubbedClassesFromSubclasses.enable! first. If we move this line to enable!, it will raise an error.

As you wish, I can do :

diff --git a/lib/rspec/mocks/exclude_stubbed_classes_from_subclasses.rb b/lib/rspec/mocks/exclude_stubbed_classes_from_subclasses.rb index bb72101e..129ff0d2 100644 --- a/lib/rspec/mocks/exclude_stubbed_classes_from_subclasses.rb +++ b/lib/rspec/mocks/exclude_stubbed_classes_from_subclasses.rb @@ -8,6 +8,8 @@ module RSpec def enable! return unless RUBY_VERSION >= "3.1" return if Class.respond_to?(:subclasses_with_rspec_mocks) + @excluded_subclasses = [] + require 'weakref' unless defined?(::WeakRef) Class.class_eval do def subclasses_with_rspec_mocks @@ -32,9 +34,6 @@ module RSpec end def excluded_subclasses - require 'weakref' unless defined?(::WeakRef) - - @excluded_subclasses ||= [] @excluded_subclasses.select(&:weakref_alive?).map do |ref| begin ref.__getobj__ @@ -45,9 +44,6 @@ module RSpec end def exclude_subclass(constant) - require 'weakref' unless defined?(::WeakRef) - - @excluded_subclasses ||= [] @excluded_subclasses << ::WeakRef.new(constant) end end diff --git a/spec/rspec/mocks/exclude_stubbed_classes_from_subclasses_spec.rb b/spec/rspec/mocks/exclude_stubbed_classes_from_subclasses_spec.rb index 3b5ded94..16454922 100644 --- a/spec/rspec/mocks/exclude_stubbed_classes_from_subclasses_spec.rb +++ b/spec/rspec/mocks/exclude_stubbed_classes_from_subclasses_spec.rb @@ -63,6 +63,10 @@ if RUBY_VERSION >= '3.1' end describe '.excluded_subclasses' do + before do + described_class.enable! + end + it 'returns excluded subclasses' do subclass = Class.new described_class.exclude_subclass(subclass) @@ -75,14 +79,12 @@ if RUBY_VERSION >= '3.1' described_class.exclude_subclass(subclass) subclass = nil - GC.start expect(described_class.excluded_subclasses).to eq([]) end it 'does not return excluded subclasses that raises a ::WeakRef::RefError' do - require 'weakref' subclass = double(:weakref_alive? => true) described_class.instance_variable_set(:@excluded_subclasses, [subclass])

pirj · 2024-03-27T16:48:43Z

lib/rspec/mocks/exclude_stubbed_classes_from_subclasses.rb

+
+          @excluded_subclasses ||= []
+          @excluded_subclasses.select(&:weakref_alive?).map do |ref|
+            begin # rubocop:disable Style/RedundantBegin


It slipped my mind, isn’t it possible to squash that so that rescue is on the map itself?

Ah, nevermind, Ruby 1.8 probably doesn’t support that. Do you mind making a note to indicate that?

I removed the comment because the CI was failing because of that (I don't have the same behavior locally with the same version of Ruby/Rubocop).

GCorbel · 2024-05-14T16:35:50Z

@JonRowe can you give a look at this request ?

JonRowe · 2024-09-27T10:31:51Z

I'd like to apologise for leaving this hanging for so long, its been on my review list for ages but I just haven't been able to get around to it, I had a couple of concerns about weak ref and other such behaviour but following on from the discussion in #1568 I'm going to close this. Sorry to have wasted your time.

GCorbel added 3 commits February 20, 2024 17:43

Exclude stubbed classes from subclasses after teardown

62580be

require weakref

d3f54fd

Add a spec

efd11e7

pirj reviewed Feb 21, 2024

View reviewed changes

GCorbel added 3 commits February 21, 2024 09:49

Handle ref error

57cc8e9

minor changes

4068c21

fix specs

6dc7dc7

JonRowe reviewed Feb 21, 2024

View reviewed changes

JonRowe suggested changes Feb 21, 2024

View reviewed changes

GCorbel added 3 commits February 28, 2024 15:22

add the patch behind a flag

dc02871

Merge remote-tracking branch 'origin/main'

a3caeab

cleanup

3007ee7

GCorbel commented Feb 28, 2024

View reviewed changes

GCorbel added 2 commits February 29, 2024 16:11

restrict the path for ruby >= 3.1

0ba2dcd

fix

70b4503

fix for Ruby 2.4 and oldest

f4a1886

disable a rubocop warning

275c69d

GCorbel commented Mar 5, 2024

View reviewed changes

GCorbel added 2 commits March 5, 2024 14:21

Use full module name for WeakRef constants

18683bf

Correctly reset config after tests

5688b2a

GCorbel requested review from JonRowe and pirj March 6, 2024 17:15

pirj reviewed Mar 25, 2024

View reviewed changes

GCorbel added 2 commits March 26, 2024 12:31

apply comments

c3b17ce

minor change

43ce7c7

GCorbel added 3 commits March 27, 2024 11:25

refactor

45eb65f

fix rubocop issues

093f2ed

always match subclasses

1da3cfa

pirj approved these changes Mar 27, 2024

View reviewed changes

remove a rule disabled

04a59f1

pirj mentioned this pull request Sep 18, 2024

stub_const doesn't clear Class#subclasses #1568

Closed

JonRowe closed this Sep 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exclude stubbed classes from subclasses after teardown #1570

Exclude stubbed classes from subclasses after teardown #1570

GCorbel commented Feb 20, 2024

pirj left a comment

pirj Feb 21, 2024

GCorbel Feb 21, 2024

JonRowe Feb 21, 2024

GCorbel Feb 21, 2024 •

edited

Loading

JonRowe left a comment

GCorbel commented Feb 21, 2024

GCorbel commented Feb 21, 2024

pirj commented Feb 21, 2024

pirj commented Feb 21, 2024

GCorbel commented Feb 21, 2024

pirj commented Feb 21, 2024

GCorbel left a comment

GCorbel Feb 28, 2024

GCorbel commented Feb 29, 2024

GCorbel commented Mar 4, 2024

GCorbel commented Mar 4, 2024

GCorbel Mar 5, 2024

GCorbel commented Mar 5, 2024

GCorbel commented Mar 6, 2024

GCorbel commented Mar 6, 2024

GCorbel commented Mar 25, 2024 •

edited

Loading

pirj left a comment

GCorbel commented Mar 26, 2024

pirj left a comment

pirj Mar 27, 2024

pirj Mar 27, 2024

GCorbel Mar 27, 2024

GCorbel Mar 27, 2024

pirj Mar 27, 2024

pirj Mar 27, 2024

GCorbel Mar 27, 2024

GCorbel commented May 14, 2024

JonRowe commented Sep 27, 2024

Exclude stubbed classes from subclasses after teardown #1570

Exclude stubbed classes from subclasses after teardown #1570

Conversation

GCorbel commented Feb 20, 2024

pirj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GCorbel Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

JonRowe left a comment

Choose a reason for hiding this comment

GCorbel commented Feb 21, 2024

GCorbel commented Feb 21, 2024

pirj commented Feb 21, 2024

pirj commented Feb 21, 2024

GCorbel commented Feb 21, 2024

pirj commented Feb 21, 2024

GCorbel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GCorbel commented Feb 29, 2024

GCorbel commented Mar 4, 2024

GCorbel commented Mar 4, 2024

Choose a reason for hiding this comment

GCorbel commented Mar 5, 2024

GCorbel commented Mar 6, 2024

GCorbel commented Mar 6, 2024

GCorbel commented Mar 25, 2024 • edited Loading

pirj left a comment

Choose a reason for hiding this comment

GCorbel commented Mar 26, 2024

pirj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GCorbel commented May 14, 2024

JonRowe commented Sep 27, 2024

GCorbel Feb 21, 2024 •

edited

Loading

GCorbel commented Mar 25, 2024 •

edited

Loading