Rspec-core: described_class behavior doesn't make sense when inner block is not a Class

[betesh]

Gemfile:
gem 'rspec', '= 3.0.0'

spec/described_class_spec.rb:

describe Array do
  describe 'a string' do
     it { expect(described_class).to eq(Array) }
  end

  describe :a_symbol do
    it { expect(described_class).to eq(Array) }
  end

  describe Hash do
    it { expect(described_class).to eq(Hash) }
  end

  describe Hash.new do
    it { expect(described_class).to eq(Hash) }
  end
end

This results in two errors--one on :a_symbol and one on Hash.new. IMO this behavior is non-intuitive. #1114 seems to have made an exception for when the inner describe is an instance of String, which makes sense, because described_class.should be_a(Class). However, I argue that this should be consistent for all cases where the inner describe is not a Class. Either take its class, or skip it and move to the outer describe block, but don't return an instance.

[myronmarston]

This change, while potentially confusing for the example you gave, was a consequence of an intentional change. Consider if you use subject instead. These specs all pass:

describe Array do
  describe 'a string' do
     it { expect(subject).to eq([]) }
  end

  describe :a_symbol do
    it { expect(subject).to eq(:a_symbol) }
  end

  describe Hash do
    it { expect(subject).to eq({}) }
  end

  describe Hash.new do
    it { expect(subject).to eq({}) }
  end
end

The default implementation of subject is to use described_class.new if it's a class or described_class otherwise. When nesting example groups and passing an argument that's not a string, it makes the most sense to derive the subject from the innermost described arg. (That's the primary change we were making). The change in behavior to described_class was simply how the subject change was implemented, given how subject works.

I agree it's confusing, though :(. Really, with how described_class works (and has pretty much always worked -- describe [1, 2, 3] in RSpec 2.x would cause described_class to return the array), the method is misnamed. It's more like described_thing -- which is a class if you pass a class or whatever object you pass. With that understanding, the current behavior is correct, even if the method is a bit misnamed.

All that's to say I'm not sure what the best thing to do is here to prevent confusion. In general, I recommend sticking to describe <string arg> for nested example groups as it avoids confusion.

1114 seems to have made an exception for when the inner describe is an instance of String, which makes sense, because described_class.should be_a(Class)

The reason for the string exception is that strings, when passed to describe or context, are taken to be an English description of the context, and not the object-being-described. Any other type of object is assumed to be the thing the user is describing.

[betesh]

You make some excellent points. For starters, that described_class is poorly named. That in itself is a bug, given the confusion it can create.

I also argue that if a String instance implies an English description, a Symbol instance implies a method on the outer described thing. That, at least, is how I always organized things in Rspec 2.x. What I would like to be able to do is:

describe Array.new([1,2,3]) do
  describe :size do
    it { expect(described_thing).to eq(3) } # or `described_method`, perhaps?
  end
end

And given that it is very unlikely that I am interested in testing Symbol or an instance thereof (less likely, in fact, than that I am interested in testing an instance of String), this syntax should be_valid.

So there are two things to discuss here:
1) renaming described_class--given that it's new implementation is likely to break 2.x code anyway, this is pretty safe and makes it possible to raise an exception in described_class to inform users of the breaking change (which is not possible now, a definite source of confusion)
2) Making an assumption about describing a Symbol, just as we do with Strings

[myronmarston]

I also argue that if a String instance implies an English description, a Symbol instance implies a method on the outer described thing.

We intentionally changed this in RSpec 3. See 000a5d59aa57801125f0b551432f9fe5e2b3d56b and #1240. I've never heard of or seen people use describe :method_name convention -- I've always seen describe "#some_method" (for instance methods) or describe ".some_method" (for class or singleton methods). Maybe it was the wrong change, but everyone involved in the discussion felt it was confusing that describe <arbitrary object> would cause subject to change but describe :some_symbol would not.

renaming described_class--given that it's new implementation is likely to break 2.x code anyway, this is pretty safe and makes it possible to raise an exception in described_class to inform users of the breaking change (which is not possible now, a definite source of confusion)

As per SemVer, we can't rename public methods in minor releases. We can rename it in 4.0, or we can introduce a new alternate name and deprecate described_class, but I haven't been able to think of a better name than described_class, particularly given the common usage is describe SomeClass (a class). Any ideas for what the alternate name would be?

[JonRowe]

I also argue that if a String instance implies an English description, a Symbol instance implies a method on the outer described thing. That, at least, is how I always organized things in Rspec 2.x.

You're the first person I've ever heard of using that, most people use "#method" or ".method" because that's the convention the documentation formatter expects. Personally I'm against using symbols in the way you describe it'd add more magic than people would expect and that's something we've been trying to move away from.

FWIW I'd also discourage people using arbitrary objects with describe (in fact I also argue against using classes there, preferring strings)

[betesh]

@JonRowe I am satisfied with how you address my point about symbols--since there is already a convention for this, we do not need a second one.

As for not using arbitrary objects with describe, do you mean not passing instances (or classes) to the outer block, the inner block, or both?

What do you think of the following approach: If the (inner) describe block is_a?(Class), the subject calls #new on it and described_class returns it (same as current behavior). However, if it is not a class, than subject returns it and described_class calls #class on it.

describe Array.new([1,2,3]) do
  it { described_class.should eq(Array) }
  it { subject.should be_a(Array) }
end

That would address @myronmarston 's point about how it is poorly named--In all the cases where it is poorly named, it is an alias of subject, so why not just use subject instead, freeing described_class to have some other purpose that is more appropriate to its name.

[myronmarston]

I like your idea, @betesh. I wish we had thought of it before 3.0 shipped :(.

At this point, changing described_class like this would be a breaking change (particularly since all 2.x releases also allowed described_class to be any arbitrary object if your top-level was describe <arbitrary object>). We can plan on it for 4.0, though. We can also consider introducing a config option that would allow you to opt-in to the behavior sooner.

[betesh]

So shall we leave this ticket open for now, set a milestone of 4.0, and introduce a deprecation warning and configuration in 3.1?

[myronmarston]

Let's definitely keep it open. I don't think I want to introduce a deprecation warning in 3.1 (people grew weary of the many deprecations from 2.14 -> 2.99 -> 3.0, so I want to hold off on deprecations for a while) but we may introduce it in some 3.x release.

[cupakromer]

:+1: for the change to how described_class should work in the future.

As for making symbols, or strings of the form '#message' and '.message', change the subject, I'm strongly against that. It will only work in the case where the message doesn't take any arguments, which is a narrow scope. I'd also like to avoid adding any other special logic to handle different formats. @betesh the rspec-its gem provides similar behavior, perhaps it's worth a look for your use cases.

[xaviershay]

I think I prefer described_thing (or described) to having a conditional in described_class. With @betesh's proposed solution, I'd naively expect described_class to return "the class of the described thing" (i.e. Class when describing an actual class).

Leaving open because this is definitely still an inconsistency.

[phene]

Here's another example that better illustrates the problem:

class Foo do
  concerning :Things do
    def do_thing
       true
    end
  end

  concerning :OtherThings do
    def do_other_thing
      false
    end
  end
end

So imagine I want to write a foo_spec.rb, I might want something like this:

describe Foo do
  subject(:foo) { described_class.new }

  describe Foo::Things do
    describe '#do_thing' do
      subject(:do_thing) { foo.do_thing }
      it do
        expect(do_thing).to be_truthy
      end
    end
  end
  describe Foo::OtherThings do
    describe '#do_other_thing' do
      subject(:do_other_thing) { foo.do_other_thing }
      it do
        expect(do_other_thing).to be_falsey
      end
    end
  end
end

In this case, Foo::Things and Foo::OtherThings are both modules that add features to Foo. Separating these into different files wouldn't really make sense as they may share a lot of setup for Foo.

[JonRowe]

To be honest I think in RSpec 4 we should just remove described_class, we kind of consider it an anti-pattern (because using it means the class must be loaded when the file is evaluated rather than when it's run) and we thus don't recommend it, and it causes this kind of ambiguity.