rspec / rspec-mocks

RSpec's 'test double' framework, with support for stubbing and mocking
https://rspec.info
MIT License
1.16k stars 357 forks source link

Exclude stubbed classes from subclasses after teardown #1570

Open GCorbel opened 4 months ago

GCorbel commented 4 months ago

This fix https://github.com/rspec/rspec-mocks/issues/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].

GCorbel commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

pirj commented 4 months ago

We could conditionally require weakref if this option is on.

GCorbel commented 4 months ago

Should we have the option on by default ?

pirj commented 4 months ago

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

GCorbel commented 4 months ago

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 commented 4 months ago

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

GCorbel commented 4 months ago

I fixed the code for Ruby <= 2.4

GCorbel commented 4 months ago

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

GCorbel commented 4 months ago

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

GCorbel commented 4 months ago

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 commented 3 months ago

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.

GCorbel commented 3 months ago

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.

GCorbel commented 2 months ago

@JonRowe can you give a look at this request ?