Cleanup bug_report_xyz_spec.rb files

[myronmarston]

There are a bunch of bug_report_xyz_spec.rb files that warrant being cleaned up:

• The xyz number refers to old lighthouse tickets which we no longer have, and the filenames don't mean anything to us anymore.
• These specs should be moved into the appropriate spec files that cover the area the bug spec is covering. In some cases, they probably duplicate some other specs and can simply be deleted.
• Some of the specs are written using a very old style (e.g. manually rescue exceptions rather than using the raise_error matcher) and should be updated.

Anyone want to take a stab at this? (It's low priority, obviously).

[gvc]

Hey @myronmarston I can try to tackle that if that's ok! :smile:

[myronmarston]

Sounds good, @gvc!

[gvc]

Hey @myronmarston, could you take a look at file bug_report_7611_spec.rb. The specs in those files aren't doing much on their own, and when I join both 'it's the spec fails. Removing this spec altogether doesn't change coverage at all, so should I just ditch this spec?

[myronmarston]

I think it's fine to drop it. It's not clear to me what it's even testing :(.

[JonRowe]

I'd say remove it, it's > 5 years old in this form. It appears to be testing that stubbing a sub class isn't contaminated across examples, but I fear it's more likely it's just been mis translated at some point. Unless anyone fancies finding an rspec repo older than @dchelimsky's and sperlunking through it...

[dchelimsky]

Here's the original bug report from my mail:

Summary: Partial Mocks override Subclass methods

Initial Comment:
Here is a quick error case that is order dependent. The first spec must run first. I'll post a better spec later.

require "rubygems"
require "spec"

class Foo
end

class Bar < Foo
end

context "A Partial Mock" do
  specify "should respect subclasses" do
    Foo.stub!(:new).and_return(Object.new)
  end

  specify "should" do
    Bar.new.class.should == Bar
  end
end

# output

.F

1)
'A Partial Mock should' FAILED
Foo should == Bar
/home/btakita/workspace/scratch/lib/bdd/partial_mock_subclass_bug.rb:17:
/home/btakita/workspace/scratch/lib/bdd/partial_mock_subclass_bug.rb:11:

The issue was that stubbing a class method on a superclass clobbered the same method on subclasses, something we should definitely have spec'd for regression testing if we don't already.

[myronmarston]

The issue was that stubbing a class method on a superclass clobbered the same method on subclasses, something we should definitely have spec'd for regression testing if we don't already.

The example sets the stub in one example, then calls the stubbed method in another example, so it seems to be more related to stubs leaking between examples than subclassing...

[myronmarston]

Here's an alternate spec I just wrote up in a more modern style:

    describe "A partial class mock that has been subclassed" do
      Foo = Class.new
      Bar = Class.new(Foo)

      it "can stub #new without affecting the subclass's #new" do
        allow(Foo).to receive(:new).and_return(:new_foo)

        expect(Foo.new).to be(:new_foo)
        expect(Bar.new).to be_a(Bar)
      end
    end

It fails:

  1) A partial class mock that has been subclassed can stub #new without affecting the subclass's #new
     Failure/Error: expect(Bar.new).to be_a(Bar)
       expected :new_foo to be a kind of RSpec::Mocks::Bar
     # /Users/myron/code/rspec-dev/repos/rspec-expectations/lib/rspec/expectations/fail_with.rb:32:in `fail_with'
     # /Users/myron/code/rspec-dev/repos/rspec-expectations/lib/rspec/expectations/handler.rb:36:in `handle_matcher'
     # /Users/myron/code/rspec-dev/repos/rspec-expectations/lib/rspec/expectations/expectation_target.rb:34:in `to'
     # ./spec/rspec/mocks/partial_mock_spec.rb:213:in `block (2 levels) in <module:Mocks>'
     # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/example.rb:114:in `instance_exec'
     # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/example.rb:114:in `block in run'
     # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/example.rb:251:in `with_around_each_hooks'
     # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/example.rb:111:in `run'
     # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/example_group.rb:404:in `block in run_examples'
     # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/example_group.rb:400:in `map'
     # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/example_group.rb:400:in `run_examples'
     # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/example_group.rb:385:in `run'
     # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/command_line.rb:28:in `block (2 levels) in run'
     # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/command_line.rb:28:in `map'
     # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/command_line.rb:28:in `block in run'
     # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/reporter.rb:58:in `report'
     # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/command_line.rb:25:in `run'
     # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/runner.rb:90:in `run'
     # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/runner.rb:17:in `block in autorun'

However, this seems like the right behavior to me: given how ruby's inheritance works, if Bar has not defined its own new, it'll look up the ancestor chain and use the implementation in Foo.

[JonRowe]

The original bug report was when the stub leaked across examples, which is definitely something we don't want happening...

[dchelimsky]

@myronmarston - initialize defaults to super, so it makes sense that Bar.new returns :new_foo, which is the result of calling super, in your (correctly) failing example. The problem was that if you stubbed Foo.new in one example and called Bar.new in a later example, it would still return :new_foo because the stub wasn't getting cleaned up correctly.

[myronmarston]

How's this for a modern, re-written (and hopefully much clearer) spec?

    describe "A partial class mock that has been subclassed" do
      it "cleans up stubs during #reset to prevent leakage onto subclasses between examples" do
        klass = Class.new
        subklass = Class.new(klass)
        allow(klass).to receive(:new).and_return(:new_foo)

        reset(klass)

        expect(subklass.new).to be_a(subklass)
      end
    end

[dchelimsky]

I mostly like it, but how about adding a call to subklass.new for context:

    describe "A partial class mock that has been subclassed" do
      it "cleans up stubs during #reset to prevent leakage onto subclasses between examples" do
        klass = Class.new
        subklass = Class.new(klass)
        allow(klass).to receive(:new).and_return(:new_foo)
        expect(subklass.new).to eq :new_foo

        reset(klass)

        expect(subklass.new).to be_a(subklass)
      end
    end

[myronmarston]

I mostly like it, but how about adding a call to subklass.new for context:

:+1:

[gvc]

Oh god, I feel awful that I missed all this discussion. I had to go back to the books to try to make sense of that spec there. Only to find out it was testing for leaks. I'll submit a pull request now and open up a new discussion there.

Thanks for all your input!

[myronmarston]

Closing in favor of #406.