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

Constants (e.g. class, module, etc) defined inside an example_group leak. #2181

Closed
ioquatix opened this Issue Mar 1, 2016 · 13 comments

Comments

Projects
None yet
4 participants
@ioquatix

ioquatix commented Mar 1, 2016

Say I have a spec:

#!/usr/bin/env rspec

RSpec.describe "define some module" do
    module MyModule
    end
end

RSpec.describe "a different example group" do
    it "should not have module defined" do
        expect(defined?(MyModule)).to be == nil
    end
end

I get the following output

a different example group
  should not have module defined (FAILED - 1)

Failures:

  1) a different example group should not have module defined
     Failure/Error: expect(defined?(MyModule)).to be == nil

       expected: == nil
            got:    "constant"
     # ./test.rb:10:in `block (2 levels) in <top (required)>'

Finished in 0.01106 seconds (files took 0.09443 seconds to load)
1 example, 1 failure

Failed examples:

rspec ./test.rb:9 # a different example group should not have module defined

I don't know if this is expected, but it seems odd that constants defined in one example group would leak into another.

@ioquatix ioquatix changed the title from Classes defined inside an example_group leak. to Constants (e.g. class, module, etc) defined inside an example_group leak. Mar 1, 2016

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Mar 1, 2016

Member

While not ideal, it's known, expected behavior. In Ruby, when you define a constant in a block, it uses the module scoping of whatever is outside the block as the constant namespace. For example:

irb(main):001:0> Foo = Class.new do
irb(main):002:1* BAR = 5
irb(main):003:1> end
=> Foo
irb(main):004:0> BAR
=> 5

In this IRB session, BAR was defined at the top level scope, (e.g. ::BAR or Object::BAR), not as Foo::BAR like you might expect. This is simply how Ruby works and there's nothing RSpec can do about this. (And since it's how ruby works, I'm not sure that we should try to do anything different.)

In your example, module MyModule in the describe block has defined a top-level MyModule constant that is accessible from anywhere in the program. That's why you're seeing this behavior.

Member

myronmarston commented Mar 1, 2016

While not ideal, it's known, expected behavior. In Ruby, when you define a constant in a block, it uses the module scoping of whatever is outside the block as the constant namespace. For example:

irb(main):001:0> Foo = Class.new do
irb(main):002:1* BAR = 5
irb(main):003:1> end
=> Foo
irb(main):004:0> BAR
=> 5

In this IRB session, BAR was defined at the top level scope, (e.g. ::BAR or Object::BAR), not as Foo::BAR like you might expect. This is simply how Ruby works and there's nothing RSpec can do about this. (And since it's how ruby works, I'm not sure that we should try to do anything different.)

In your example, module MyModule in the describe block has defined a top-level MyModule constant that is accessible from anywhere in the program. That's why you're seeing this behavior.

@ioquatix

This comment has been minimized.

Show comment
Hide comment
@ioquatix

ioquatix Mar 1, 2016

Okay, if that's the case, one option is to document this somewhere and explain the following:

RSpec.describe "define some module" do
    module self::MyModule
    end
end

RSpec.describe "a different example group" do
    it "should not have module defined" do
        expect(defined?(MyModule)).to be == nil
    end
end

Prepending self:: does work as expected.

I believe there are some other options too, hold on while I check this.

ioquatix commented Mar 1, 2016

Okay, if that's the case, one option is to document this somewhere and explain the following:

RSpec.describe "define some module" do
    module self::MyModule
    end
end

RSpec.describe "a different example group" do
    it "should not have module defined" do
        expect(defined?(MyModule)).to be == nil
    end
end

Prepending self:: does work as expected.

I believe there are some other options too, hold on while I check this.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Mar 1, 2016

Member

Yep, Ruby allows you to explicitly namespace a constant with self:: so that works. Here's the same thing in IRB:

irb(main):001:0> Foo = Class.new do
irb(main):002:1* self::BAR = 5
irb(main):003:1> end
=> Foo
irb(main):004:0> BAR
NameError: uninitialized constant BAR
        from (irb):4
        from /Users/myron/.rubies/ruby-2.3.0/bin/irb:11:in `<main>'
irb(main):005:0> Foo::BAR
=> 5

Regarding documenting this -- we could, but bear in mind that this has nothing to do with RSpec. Any document would just be documenting how Ruby works with regard to constant definitions and blocks.

Member

myronmarston commented Mar 1, 2016

Yep, Ruby allows you to explicitly namespace a constant with self:: so that works. Here's the same thing in IRB:

irb(main):001:0> Foo = Class.new do
irb(main):002:1* self::BAR = 5
irb(main):003:1> end
=> Foo
irb(main):004:0> BAR
NameError: uninitialized constant BAR
        from (irb):4
        from /Users/myron/.rubies/ruby-2.3.0/bin/irb:11:in `<main>'
irb(main):005:0> Foo::BAR
=> 5

Regarding documenting this -- we could, but bear in mind that this has nothing to do with RSpec. Any document would just be documenting how Ruby works with regard to constant definitions and blocks.

@ioquatix

This comment has been minimized.

Show comment
Hide comment
@ioquatix

ioquatix Mar 1, 2016

I think it's a common enough tripping point, I've seen it in several projects and wondered if there was a better solution. In the end, people tend to try and shove stuff in a shared scope but it's a bit of a pain.

e.g. https://github.com/celluloid/timers/blob/fcb443006ed1770d3ba2ebf3db0e0d2e570c6672/spec/spec_helper.rb#L7-L8 used to actually be defined as Q = ... within the RSpec.describe block but it turned out it was just redefining the constant over and over again.

My work around in the past was to define specs inside their own module, but that also seems a bit disingenuous and makes the syntax of tests a bit cumbersome.

ioquatix commented Mar 1, 2016

I think it's a common enough tripping point, I've seen it in several projects and wondered if there was a better solution. In the end, people tend to try and shove stuff in a shared scope but it's a bit of a pain.

e.g. https://github.com/celluloid/timers/blob/fcb443006ed1770d3ba2ebf3db0e0d2e570c6672/spec/spec_helper.rb#L7-L8 used to actually be defined as Q = ... within the RSpec.describe block but it turned out it was just redefining the constant over and over again.

My work around in the past was to define specs inside their own module, but that also seems a bit disingenuous and makes the syntax of tests a bit cumbersome.

@ioquatix

This comment has been minimized.

Show comment
Hide comment
@ioquatix

ioquatix Mar 1, 2016

Hmm, so I found some additional problems, using the self:: prefix causes other classes to break, because they can't find each other. I guess I'll revert back to my original approach of wrapping the whole spec in a module.

ioquatix commented Mar 1, 2016

Hmm, so I found some additional problems, using the self:: prefix causes other classes to break, because they can't find each other. I guess I'll revert back to my original approach of wrapping the whole spec in a module.

@ioquatix

This comment has been minimized.

Show comment
Hide comment
@ioquatix

ioquatix Mar 1, 2016

By the way, thanks for your detailed explanation.

ioquatix commented Mar 1, 2016

By the way, thanks for your detailed explanation.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Mar 1, 2016

Member

You might also try stub_const as it provides a way to define a constant for a single example and have RSpec revert it when the example completes.

Member

myronmarston commented Mar 1, 2016

You might also try stub_const as it provides a way to define a constant for a single example and have RSpec revert it when the example completes.

@ioquatix

This comment has been minimized.

Show comment
Hide comment
@ioquatix

ioquatix Mar 1, 2016

Ah yep. In this particular example, I have several top level classes which I'd need to deal with.

https://github.com/ioquatix/mapping/blob/8fe2d7128df320f7375eadc3da83ad1b88e02979/spec/mapping/model_spec.rb

ioquatix commented Mar 1, 2016

Ah yep. In this particular example, I have several top level classes which I'd need to deal with.

https://github.com/ioquatix/mapping/blob/8fe2d7128df320f7375eadc3da83ad1b88e02979/spec/mapping/model_spec.rb

@bf4

This comment has been minimized.

Show comment
Hide comment
@bf4

bf4 May 31, 2016

Contributor

Or, if you need a module defined in just one spec, make it dynamically, and then clean up?

RSpec.describe 'my test', type: :awesome do
  some_module = Module.new do
    # some code
  end
  receiver = self
  receiver.const_set(:SomeModule, some_module)
  after(:all) do
    receiver.send(:remove_const, :SomeModule)
  end
end

This is one thing that is way more explicit in minitest land, since I can just use an ensure in the test aka method.

Rails does this sort of thing all the time and just handles it manually.

But, I guess the RSpec procsy object could check for const_added and then have them removed /shrug

Contributor

bf4 commented May 31, 2016

Or, if you need a module defined in just one spec, make it dynamically, and then clean up?

RSpec.describe 'my test', type: :awesome do
  some_module = Module.new do
    # some code
  end
  receiver = self
  receiver.const_set(:SomeModule, some_module)
  after(:all) do
    receiver.send(:remove_const, :SomeModule)
  end
end

This is one thing that is way more explicit in minitest land, since I can just use an ensure in the test aka method.

Rails does this sort of thing all the time and just handles it manually.

But, I guess the RSpec procsy object could check for const_added and then have them removed /shrug

@ioquatix

This comment has been minimized.

Show comment
Hide comment
@ioquatix

ioquatix May 31, 2016

@bf4 That's an interesting option.

I guess it would be nice if there was a simpler option.

Perhaps something like

#!/usr/bin/env ruby

module RSpec
    class Test
        def self.it(*args, &block)
            puts args
            yield
        end
    end

    def self.describe(*args)
        klass = Class.new(Test)

        klass.const_set('ARGS', args)

        return klass
    end
end

class MyTest < RSpec.describe 'my test'
    class Foo
    end

    it 'should be okay' do
        puts "Foo is defined: #{Foo.inspect}"
    end
end

ioquatix commented May 31, 2016

@bf4 That's an interesting option.

I guess it would be nice if there was a simpler option.

Perhaps something like

#!/usr/bin/env ruby

module RSpec
    class Test
        def self.it(*args, &block)
            puts args
            yield
        end
    end

    def self.describe(*args)
        klass = Class.new(Test)

        klass.const_set('ARGS', args)

        return klass
    end
end

class MyTest < RSpec.describe 'my test'
    class Foo
    end

    it 'should be okay' do
        puts "Foo is defined: #{Foo.inspect}"
    end
end
@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe May 31, 2016

Member

@bf4 that's no different to doing:

RSpec.describe 'my test', type: :awesome do
  module self::SomeModule
    # some code
  end
end

The module isn't then available outside the 'my test' class anyway so no need to clean it up. The downside to both these approaches is having to reference modules by self.class:: inside the tests as their blocks are also scoped to the outside by Ruby.

If you're just looking for sacrificial modules which to test things like inclusion etc, you're better off creating them attached to local variables or let's, and letting the garbage collector clean them up.

Member

JonRowe commented May 31, 2016

@bf4 that's no different to doing:

RSpec.describe 'my test', type: :awesome do
  module self::SomeModule
    # some code
  end
end

The module isn't then available outside the 'my test' class anyway so no need to clean it up. The downside to both these approaches is having to reference modules by self.class:: inside the tests as their blocks are also scoped to the outside by Ruby.

If you're just looking for sacrificial modules which to test things like inclusion etc, you're better off creating them attached to local variables or let's, and letting the garbage collector clean them up.

@ioquatix

This comment has been minimized.

Show comment
Hide comment
@ioquatix

ioquatix May 31, 2016

@JonRowe what are scarificial modules?

@JonRowe what are scarificial modules?

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe May 31, 2016

Member

A typo for sacrificial, e.g. a module you want for a test then throw away.

Member

JonRowe commented May 31, 2016

A typo for sacrificial, e.g. a module you want for a test then throw away.

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