Open GCorbel opened 5 months ago
It seems that doing a garbage collection after RSpec::Mocks.teardown
fix the issue.
class Something; end
describe 'Test' do
before(:each) do
class A < Something; end
stub_const('B', Class.new(Something))
end
after(:each) do
RSpec::Mocks.teardown
GC.start
end
it 'something' do
pp Something.subclasses # => [B, A]
end
it 'something else' do
pp Something.subclasses # => [B, A]
end
end
I tried on a Rails project and doing a garbage collection is not enough. Defined classes without stub_const
are correctly removed and those that use stub_const
aren't.
What do you mean "correctly removed"?
describe 'Test', order: :defined do
context 'with A' do
before(:each) do
class A < Something; end
end
it 'something' do
pp Something.subclasses # => [A]
end
end
context 'presumably without A' do
it 'something else' do
pp Something.subclasses # => [A] 💥
end
end
end
A
won't go anywhere.
For stub_const
, wondering, what the @parent
is, and why calling remove_const
on it doesn't reset its subclasses
?
What do you mean "correctly removed"?
I mean, Something.subclasses
have to be the same for each spec. I edited the ticket's description.
RSpec::Mocks.space.instance_variable_get(:@constant_mutators)
gives :
[#<RSpec::Mocks::ConstantMutator::UndefinedConstantSetter:0x00007fb2c9e84c10
@const_name="B",
@context_parts=[],
@full_constant_name="B",
@mutated_value=B,
@parent=Object,
@reset_performed=false,
@transfer_nested_constants=nil>]
Object.send(:remove_const, :B)
gives :
NameError:
constant Object::B not defined
@parent.__send__(:remove_const, @const_name)
Note that B.new
raise no error.
I see than everytime we use Class.new(Something)
, it is added in subclasses of Something
but not with class A < Something; end
:
class A < Something; end
class A < Something; end
class A < Something; end
class A < Something; end
class A < Something; end
stub_const('B', Class.new(Something))
stub_const('B', Class.new(Something))
stub_const('B', Class.new(Something))
stub_const('B', Class.new(Something))
stub_const('B', Class.new(Something))
puts Something.subclasses.inspect
# => [B, B, B, B, B, A]
puts ObjectSpace.each_object(Class).select { |klass| klass.name == 'A' }.count
# => 1
puts ObjectSpace.each_object(Class).select { |klass| klass.name == 'B' }.count
# => 5
I understand that as Class.new
is a completly different Class, with a different object_id
while A
gives the same object_id
each time.
As we see in this example, B
is still in ObjectSpace
.
describe 'Test' do
before(:each) do
class A < Something; end
stub_const('B', Class.new(Something))
end
after(:each) do
pp ObjectSpace.each_object(Class).select { |klass| klass.name == 'A' }.count
# Gives 1 twice as supposed
pp ObjectSpace.each_object(Class).select { |klass| klass.name == 'B' }.count
# Gives 1 the first time but 2 the second time
end
it 'something' do
end
it 'something else' do
end
end
I don't know how to be sure the class than is removed.
Defining the class in a global variable fix the issue :
class Something; end
describe 'Test' do
before(:each) do
class A < Something; end
$b ||= Class.new(Something)
stub_const('B', $b)
end
after(:each) do
pp Something.subclasses
# => Gives [B, A] twice
end
it 'something' do
end
it 'something else' do
end
end
But I suppose it will create some other errors.
@pirj can you confirm this is a bug or, at least, something that is supposed to be handled in rspec-mocks ?
Something feels off, but the essence of the problem evades me so far. The class A
is not the correct contrast. Global var is closer.
What we know is that unreferenced classes are garbage-collected. But why subclasses show that there are two bound to consts with identical names?
I’m still not convienced that our observation about @parent
is correct. It doesn’t blow up in your test? Why it does blow up if you run it manually?
But why subclasses show that there are two bound to consts with identical names?
It doesn't surprise me that much. Every Class.new
is referenced as a completely different class so every time we do Object.send(:const_set, 'Bla', Class.new)
, another class is added in the ObjectSpace with the same name
I’m still not convienced that our observation about @parent is correct. It doesn’t blow up in your test? Why it does blow up if you run it manually?
The error is not actually raised when I do Object.send(:remove_const, :B)
but after. Here is the full trace :
Failure/Error: @parent.__send__(:remove_const, @const_name)
NameError:
constant Object::B not defined
@parent.__send__(:remove_const, @const_name)
^^^^^^^^^
# /usr/local/bundle/gems/rspec-mocks-3.12.6/lib/rspec/mocks/mutate_const.rb:300:in `remove_const'
# /usr/local/bundle/gems/rspec-mocks-3.12.6/lib/rspec/mocks/mutate_const.rb:300:in `reset'
# /usr/local/bundle/gems/rspec-mocks-3.12.6/lib/rspec/mocks/mutate_const.rb:161:in `idempotently_reset'
# /usr/local/bundle/gems/rspec-mocks-3.12.6/lib/rspec/mocks/space.rb:82:in `block in reset_all'
# /usr/local/bundle/gems/rspec-mocks-3.12.6/lib/rspec/mocks/space.rb:82:in `each'
# /usr/local/bundle/gems/rspec-mocks-3.12.6/lib/rspec/mocks/space.rb:82:in `reset_all'
# /usr/local/bundle/gems/rspec-mocks-3.12.6/lib/rspec/mocks.rb:52:in `teardown'
# /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/mocking_adapters/rspec.rb:27:in `teardown_mocks_for_rspec'
# /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/example.rb:521:in `run_after_example'
# /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/example.rb:283:in `block in run'
# /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/example.rb:511:in `block in with_around_and_singleton_context_hooks'
# /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/example.rb:468:in `block in with_around_example_hooks'
# /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/hooks.rb:486:in `block in run'
# /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/hooks.rb:624:in `run_around_example_hooks_for'
# /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/hooks.rb:486:in `run'
# /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/example.rb:468:in `with_around_example_hooks'
# /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/example.rb:511:in `with_around_and_singleton_context_hooks'
# /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/example.rb:259:in `run'
# /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/example_group.rb:646:in `block in run_examples'
# /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/example_group.rb:642:in `map'
# /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/example_group.rb:642:in `run_examples'
# /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/example_group.rb:607:in `run'
# /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/runner.rb:121:in `block (3 levels) in run_specs'
# /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/runner.rb:121:in `map'
# /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/runner.rb:121:in `block (2 levels) in run_specs'
# /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/configuration.rb:2070:in `with_suite_hooks'
# /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/runner.rb:116:in `block in run_specs'
# /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/reporter.rb:74:in `report'
# /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/runner.rb:115:in `run_specs'
# /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/runner.rb:89:in `run'
# /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/runner.rb:71:in `run'
# /usr/local/bundle/gems/rspec-core-3.12.2/lib/rspec/core/runner.rb:45:in `invoke'
# /usr/local/bundle/gems/rspec-core-3.12.2/exe/rspec:4:in `<top (required)>'
# /usr/local/bundle/bin/rspec:25:in `load'
# /usr/local/bundle/bin/rspec:25:in `<top (required)>'
# /usr/local/bundle/gems/bundler-2.3.4/lib/bundler/cli/exec.rb:58:in `load'
# /usr/local/bundle/gems/bundler-2.3.4/lib/bundler/cli/exec.rb:58:in `kernel_load'
# /usr/local/bundle/gems/bundler-2.3.4/lib/bundler/cli/exec.rb:23:in `run'
# /usr/local/bundle/gems/bundler-2.3.4/lib/bundler/cli.rb:484:in `exec'
# /usr/local/bundle/gems/bundler-2.3.4/lib/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
# /usr/local/bundle/gems/bundler-2.3.4/lib/bundler/vendor/thor/lib/thor/invocation.rb:127:in `invoke_command'
# /usr/local/bundle/gems/bundler-2.3.4/lib/bundler/vendor/thor/lib/thor.rb:392:in `dispatch'
# /usr/local/bundle/gems/bundler-2.3.4/lib/bundler/cli.rb:31:in `dispatch'
# /usr/local/bundle/gems/bundler-2.3.4/lib/bundler/vendor/thor/lib/thor/base.rb:485:in `start'
# /usr/local/bundle/gems/bundler-2.3.4/lib/bundler/cli.rb:25:in `start'
# /usr/local/bundle/gems/bundler-2.3.4/exe/bundle:48:in `block in <top (required)>'
# /usr/local/bundle/gems/bundler-2.3.4/lib/bundler/friendly_errors.rb:103:in `with_friendly_errors'
# /usr/local/bundle/gems/bundler-2.3.4/exe/bundle:36:in `<top (required)>'
# /usr/local/bundle/bin/bundle:25:in `load'
# /usr/local/bundle/bin/bundle:25:in `<main>'
#
# Showing full backtrace because every line was filtered out.
# See docs for RSpec::Configuration#backtrace_exclusion_patterns and
# RSpec::Configuration#backtrace_inclusion_patterns for more information.
I did those tests (none fix the issue)
# This does not raise an error
after(:each) do
RSpec::Mocks.space.reset_all
end
# This does not raise an error
after(:each) do
RSpec::Mocks.space.instance_variable_get(:@constant_mutators).map do |mutator|
mutator.idempotently_reset
end
end
# This raise constant Object::B not defined
after(:each) do
Object.send(:remove_const, :B)
end
# This raise constant Object::B not defined
after(:each) do
RSpec::Mocks.space.instance_variable_get(:@constant_mutators).map do |mutator|
mutator.instance_variable_get(:@parent).send(:remove_const, mutator.instance_variable_get(:@const_name))
end
end
# This raise constant Object::B not defined
after(:each) do
RSpec::Mocks.space.instance_variable_get(:@constant_mutators).map do |mutator|
mutator.reset
end
end
Sorry, I never meant you should call remove_const from the spec code. And if I did, I was meaning something else.
So we’re now at a point where we see that tge GC is involved, and yet-to-be-collected classes still appear through subclasses
even though are not anymore referenced or even accessible.
Do you think RSpec is the one responsible?
What is the original problem you are dealing with? Do you use subclasses
in your production code?
In our own spec suite using various means for better isolation between examples, even running another process of RSpec, forking etc.
I do think this is a bug but globals is not the answer, if the issue is Ruby is retaining the class after we have removed it, I'm not sure of the solution to that...
So we’re now at a point where we see that tge GC is involved, and yet-to-be-collected classes still appear through subclasses even though are not anymore referenced or even accessible.
Relying on garbage collection can be tricky as the class can be kept in memory elsewhere, and I'm pretty sure it is with Rails.
Do you think RSpec is the one responsible?
No, I don't.
What is the original problem you are dealing with? Do you use subclasses in your production code?
Yes, I do. We have something like :
class Content
def self.creatable_classes
self.subclasses
end
def self.creatable_classes_for(user)
creatable_classes.select { |klass| klass.creatable_for?(user) }
end
end
class Article < Content
def self.creatable_for?(user)
user.admin?
end
end
class Something < Content
def self.creatable_for?(user)
true
end
end
Content.creatable_classes_for(user)
# => [Article, Something] if the user is an admin and [Something] for others
I'm not sure if this is the best implementation but that's not the point.
Rails overrides subclasses
when RubyFeatures::CLASS_SUBCLASSES
is false. Maybe we can do something as :
module ExcludeClassesFromSubclasses
@@stubbed_classes = []
def self.stub_class(klass)
@@stubbed_classes += klass
end
def self.subclasses
super - @@stubbed_classes
end
end
Class.prepend(ExcludeClassesFromSubclasses)
It's pseudo code, it doesn't work. I don't like to override Class
but I suppose it will work.
That means that we’ll keep references to stubbed classes in between examples, effectively preventing from GC’ing them.
Also, the classes stubbed in an example should be returned by subclasses
while the example runs, right? We should push them to the exclude list after teardown.
So, we’ll need an extra step and WeakRef.
Even though this would fix your case, I have no certainty we should include this for everyone and what the performance penalty is.
Also, the classes stubbed in an example should be returned by subclasses while the example runs, right? We should push them to the exclude list after teardown.
Yes, that's it. The method name should probably be hide
or something like that.
I succeeded to fix my issue with :
diff --git a/lib/rspec/mocks.rb b/lib/rspec/mocks.rb
index 297779e5..b2beb79c 100644
--- a/lib/rspec/mocks.rb
+++ b/lib/rspec/mocks.rb
@@ -101,6 +101,23 @@ module RSpec
end
end
+ @@excluded_subclasses = []
+
+ def self.excluded_subclasses
+ @@excluded_subclasses.select(&:weakref_alive?).map(&:__getobj__)
+ end
+
+ def self.exclude_subclass(constant)
+ @@excluded_subclasses << WeakRef.new(constant)
+ end
+
+ module ExcludeClassesFromSubclasses
+ def subclasses
+ super - RSpec::Mocks.excluded_subclasses
+ end
+ end
+ Class.prepend(ExcludeClassesFromSubclasses)
+
class << self
# @private
attr_reader :space
diff --git a/lib/rspec/mocks/mutate_const.rb b/lib/rspec/mocks/mutate_const.rb
index 071d7a21..d702c79d 100644
--- a/lib/rspec/mocks/mutate_const.rb
+++ b/lib/rspec/mocks/mutate_const.rb
@@ -297,6 +297,7 @@ module RSpec
end
def reset
+ RSpec::Mocks.exclude_subclass(@mutated_value)
@parent.__send__(:remove_const, @const_name)
end
I don't like to do override a Ruby method but see no other solution. I will create a PR with this code.
Subject of the issue
When
stub_const
is used with a class that is a subclass of another, the class is still listed in subclasses of this parent after each spec.Your environment
Steps to reproduce
Expected behavior
Something.subclasses
always have to return[B, A]
and be the same for each spec.Actual behavior
Something.subclasses
still return every stubbed classes.