(refactor): Avoid calling superclass.method

[dgmstuart]

...in favour of using a lambda.

This addresses #2008

But really, any third party code which causes problems with the original
code is probably doing something a bit dodgy (e.g. overriding the method method on an
object).

Core test suite passes locally, but I haven't been able to run the travis build locally because of the following (unrelated) error:

» script/run_build
============= Starting binstub check ===============
Checking required binstubs
============= Ending binstub check ===============
============= Starting specs ===============
/Users/dxwduncan/dev/ruby_and_rails/rspec-core/bin/rspec
script/rspec_with_simplecov:17:in `require': cannot load such file -- bundler/setup (LoadError)
        from script/rspec_with_simplecov:17:in `<main>'

(I'm using bundler 1.10.6)

[myronmarston]

Thanks, @dgmstuart! Do you mind adding a spec for this to ensure we don't regress in the future? As it is, someone could submit a PR with a refactoring back to using method and our test suite wouldn't catch it. For the spec, I think you can just write it so that there's a def self.method on a particular example group, and then make a nested group (which will subclass it). Does that make sense?

[dgmstuart]

I think so. I was thinking of this as a straight refactoring, but on reflection that's not the case.

On Tue, Nov 10, 2015 at 3:57 PM, Myron Marston [email protected]
wrote:

Thanks, @dgmstuart! Do you mind adding a spec for this to ensure we don't regress in the future? As it is, someone could submit a PR with a refactoring back to using method and our test suite wouldn't catch it. For the spec, I think you can just write it so that there's a def self.method on a particular example group, and then make a nested group (which will subclass it). Does that make sense?

Reply to this email directly or view it on GitHub:
#2112 (comment)

[dgmstuart]

@myronmarston Here's what I've tried - not working

# spec/rspec/core/example_group_spec.rb
it "doesn't conflict with subclasses which override `.method`" do
  # see https://github.com/rspec/rspec-core/issues/2008
  group = RSpec.describe('group') do
    shared_examples_for("outer") do
      def self.method(arg)
        fail StandardError, "Unexpected call"
      end
      shared_examples_for("inner") do
        example('ex 1') { expect(1).to eq(1) }
      end
      it_should_behave_like "inner"
    end
    it_should_behave_like "outer"
  end
  expect{ group.run }.to_not raise_error
end

I'm not 100% sure I understand how the specs in that file work: the call to self.method seems to run before the expect at the end, and this spec doesn't seem to behave differently before or after my change. Any suggestions?

[myronmarston]

This works and is quite a bit simpler than what you have:

    it 'allows extensions to add `method` to the DSL without causing problems' do
      http_testing_extension = Module.new do
        attr_reader :http_method
        def method(meth); @http_method = meth; end
      end

      describe_successfully do
        extend http_testing_extension
        method :get

        it "allows `method` to specify the HTTP method" do
          expect(self.class.http_method).to eq(:get)
        end
      end
    end

(describe_successfully is a helper method that allows you to define a group, runs it, and asserts the examples within it all pass).

Note that this spec fails with your change -- because a similar change has to be made in another place. Without a spec we wouldn't have realized that!

[dgmstuart]

@myronmarston so many thanks for this - I'll get on it when I can.

[fables-tales]

@dgmstuart Hi! Are you still able to work on this? I'd love to see it merged.

[dgmstuart]

Yes! Sorry - I do plan to and its on my list. Are you planning a release or just checking the PR isn't stale?

On Thu, Dec 10, 2015 at 2:01 PM, Sam Phippen [email protected]
wrote:

@dgmstuart Hi! Are you still able to work on this? I'd love to see it merged.

Reply to this email directly or view it on GitHub:
#2112 (comment)

[fables-tales]

@dgmstuart just doing staleness checks. Are you likely to be able to push the commits to finish this off within the next week?

[dgmstuart]

Yes I'll commit to that :)

On Thu, Dec 10, 2015 at 9:52 PM, Sam Phippen [email protected]
wrote:

@dgmstuart just doing staleness checks. Are you likely to be able to push the commits to finish this off within the next week?

Reply to this email directly or view it on GitHub:
#2112 (comment)

[dgmstuart]

So here's where I've got to:

@myronmarston's spec above covers the additional case in normal example groups (now covered in This is covered by dgmstuart@3a586e1), but doesn't seem to cover the shared example groups case: it doesn't fail when lib/rspec/core/example_group.rb uses superclass.method.

I've tried to add a corresponding spec for shared example groups in 4156907 but it raises an error relating to exception presentation:

rspec/core/formatters/exception_presenter.rb:371:in `with_truncated_backtrace': bad value for range (Argume
ntError)
        from /Users/dxwduncan/dev/ruby_and_rails/rspec-core/lib/rspec/core/formatters/exception_presenter.rb:336:in `block (2 levels) in sub_failure_list_form
atter'
...

This code gets called three times (??) and chokes when the child is

#<RSpec::Core::MultipleExceptionError: RSpec::Core::MultipleExceptionError>

...because child_bt and parent_bt are identical, so index_before_first_common_frame comes out as nil.

I'm not sure where to start in understanding this: It's probably an issue with my spec, but also potentially this scenario could be raising a more meaningful exception earlier (?): I don't really understand what this code is doing, but clearly it's never expecting the child and parent to have identical backtraces.

Looks like MultipleExceptionError might be what is expected to be raised from an aggregate_failures block, which isn't the case here, but might make sense if for aggregated failures you're talking about a parent backtrace for the whole thing, with multiple sub-traces for each of the failures (??), so would choke if a sub-trace is identical to the parent trace.

Can anyone point me in the right direction? (@samphippen?)

[dgmstuart]

OK, extremely confused now - on Travis it's actually failing with an apparently unrelated error, and when running the whole suite locally, the spec fails in the documentation output as it's going through, but doesn't show an error at the end :(

[fables-tales]

@dgmstuart you may want to try rebasing against master, and see if that helps.

[dgmstuart]

@samphippen rebased - same test output, but I realised I was reading it wrong: Travis correctly shows one failure, but the output at the end of the rspec run doesn't include that failure - just the output for the pending specs.

Running the bundle exec rspec spec/rspec/core/example_group_spec.rb in isolation gives the ArgumentError described above.

Not sure I can make any more progress without help: this MultipleExceptionError business feels like it's deep in the Matrix...

[fables-tales]

@dgmstuart I'll take a look at this for you now.

[fables-tales]

I think I've worked out the root cause here. It's going to require me to do a little more investigation. @dgmstuart are you ok waiting on a patch from me before continuing?

[dgmstuart]

Absolutely. Thanks so much :)

On Sun, Dec 13, 2015 at 5:47 PM, Sam Phippen [email protected]
wrote:

I think I've worked out the root cause here. It's going to require me to do a little more investigation. @dgmstuart are you ok waiting on a patch from me before continuing?

Reply to this email directly or view it on GitHub:
#2112 (comment)

[fables-tales]

@myronmarston I need some help here. With this branch applied, when @dgmstuart's test fails we generate a backtrace which has the same parent and child exception in the CommonBacktraceTruncator. This causes index_before_first_common_frame to be nil here.

I have tried:

• Straight up returning the child exception
• Returning the child exception with an empty backtrace

Both cause an entertaining infinite recursion bug. I've tried debugging the infinite recursion bug, but I can't seem to squash it. Any thoughts?

[fables-tales]

@myronmarston further to my previous I've opened #2133 but I'd really like your eyes on it before we merge. Would you mind taking a look?

@dgmstuart in the mean time, you can base your PR on top of the branch I pushed in #2133 to keep this one going.

[fables-tales]

@dgmstuart I just merged that pull request, so you should be good to rebase off master and continue working on this.

[dgmstuart]

Great! Thanks @samphippen. Rebasing now. Just for my curiosity, is the case which your PR fixes only happening when RSpec is testing itself, and not in a normal use case?

[fables-tales]

@dgmstuart as far as I can tell, it's an RSpec testing RSpec problem

[fables-tales]

@dgmstuart I'm going to close this because it's been stale since December. If you'd like to pick this up again, feel free to re-open and push new commits.