Confusing described_class

[pirj]

Hey, everyone!

described_class is no doubt widely used in specs, there are 1376 usages in repositories listed in real-world-ruby-apps.

According to described_class docs:

If the first argument to [...] example group is a class, the class is exposed to each example via the described_class() method.

However, in fact it's slightly different:

It's more like described_thing -- which is a class if you pass a class or whatever object you pass.

-- Myron Marston

With one exception though, when a string is passed as the first argument.

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.

-- Myron Marston

One common mistake is passing metadata without a proper docstring, e.g.:

RSpec.describe SomeClass do
  describe controller: true do
    it { expect(described_class).to eq(SomeClass) }
  end
end

results in:

  1) SomeClass {:controller=>true} should eq SomeClass
     Failure/Error: it { expect(described_class).to eq(SomeClass) }

       expected: SomeClass
            got: {:controller=>true}

It may be unclear why the first argument is taken from the innermost example group, not outermost (SomeClass).
There was a breaking change introduced in RSpec 3.0 to take the first argument of the innermost example group as described_class, not outermost.
Unfortunately, this was not reflected in the docs (1, 2), and it's still not (it's on master but not on 3-8-maintenance), even after this pull request.

I also argue against using classes there (with describe) preferring strings

-- Jon Rowe

# bad
RSpec.describe Article do

# good
RSpec.describe “Article” do

I recommend sticking to describe <string arg> for nested example groups as it avoids confusion.

-- Myron Marston

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.

-- Xavier Shay

There was an idea to change described_class's behaviour:

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

Unfortunately, this idea was two weeks late for RSpec 3.0 release.

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

-- Myron Marston

With all this confusion in mind:

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.

-- Jon Rowe

Bottom Line

Pros of described_class:

• is widely used
• reads nicely (when used properly)
• reduces churn if the name of the class under test changes

Cons of described_class:

• leads to confusion with nested example groups
• leads to confusion when metadata is used without docstring
• leads to confusion when something that is neither a String nor a Class is passed to example group as the first argument
• might disappear in RSpec 4.0

How to Deal with This

What do you think about introducing a guideline to discourage passing anything than a literal class and a string as the first argument to an example group?
React with a 🚀 if you agree.

Or would you be radical and discourage the use of described_class altogether, and recommend using class names explicitly?
Place a 👍 if you decide to be explicit about what is being tested.

Please react with 👎 if you'd like to stick to described_class (at least until RSpec 4 is out).

Highly appreciate if you add additional arguments pro and contra.

[julik]

I think I can elaborate by explaining how we use described_class. We use it as an alternative for subject. The pattern is mostly

describe SomeModule do
  it 'performs frobulation' do
     result = described_class.frobulate(arg1, arg2)
  end
end

Sometimes we use it as an argument for ...to be(instance_of(described_class)) but less. The value of described_class vs. subject for us is pretty simple: we often use modules with either a number of methods defined on the module itself, or having a .call method so that the module itself is callable. When the module is being developed, and we want to be developing it TDD, it often so happens that we want to rename the module along the way. Sometimes multiple times. Sometimes even a week/month after the initial version of the module has been released - as choosing good names for identifiers is a known hard problem :-) In comparison to subject using described_class provides us with a substantial advantage, because it allows us to defer choosing the name for the module we are developing until later.

We also often test classes that have specific arguments to the constructor, while subject by default initialises the subject lazily in a "default" way. So the way subject works has some behavior which is not always desired, especially when dealing with modules or singletons or classes.

So while I don't think described_class is the greatest name, I think there is certainly very much a case to be made for having access to whatever thing was supplied to the outermost describe block as the argument, and let the author of the test to use that definition as-is without the magical subject behaviour attached.

I am voting with a 👎 because the wording of the issue implies a "class that can be instantiated", which is exactly something we do not have when testing callable modules, and which is not something subject easily allows us to test. On the contrary, more and more business logic in our codebases are becoming stateless, static modules.

[pirj]

I completely understand this point, @julik. What you describe is in the list of pros of described_class, "reduces churn if the name of the class under test changes". I don't have a ready-to-use solution that would fit every use case.

subject is a completely different (dark) story, I hope we'll get to it as well.
Though in the case when modules are tested subject is a good fit due to how it's defined, specifically "an instance of the described class, or the object itself", where the object is a module passed as the first argument. subject will withstand name changes just like described_class.

On a side note, be it a described_class or a subject, it adds an additional level of indirection defined implicitly. Also for implicitly defined subject it's not recommended to use it directly, and I believe the same applies to implicitly defined described_class, that semantically is "subject under test" for the case with modules.

While the examples below demonstrate how subject can be used as a user-facing concept, we recommend that you reserve it for support of custom matchers and/or extension libraries that hide its use from examples.

the wording of the issue implies a "class that can be instantiated"

It's one way to interpret it, though I did not intend to limit the focus of the problem to that specific case.

[julik]

I read you. It is a bit hard to argue that something that ends with _class implies an instance of Class though.

What are the situations where described_class does not work correctly? (except for the naming, which can be improved). Off the top of my head I can only think of shared examples (where it might be unclear what the outermost context is) - but actually in those described_class is also super helpful for us because both the setup code and the expectations all reference "the thing under test", whatever it must be. As long as it responds to the necessary messages.

[flanger001]

Adding my 👎. I don't think described_class is confusing at all and while there could be room to improve the internal api of it, the actual functionality it exposes is super useful. The example you provided is a mistaken usage by your own admission.

[dvandersluis]

I'd be okay with described_class changing to always be a class, but I don't think that's the purview of this project anyways. I use described_class extensively (and unambiguously apparently because my topmost describe is always a class), and it simplifies my test code IMO; I would not want it to change.

I'm not sure if I fully understand your 🚀 option, but I'd support rubocop-rspec enforcing that describe is given a class or string.

[arnoFleming]

I'd like to have a fourth option, to keep described_class (or at least that functionality) from now on, edit: and then I mean any RSpec >= 3.

I literally never ran in any of the issues. Maybe I have very solid testing hygiene (I doubt that), but I usualy describe one class (or module), and later I describe '#method' or context 'with doohickey enabled', and clearly that is what you describe as

reads nicely (when used properly)

I'm hoping for rubocop to help me not shooting myself in the foot writing things that are readable for end users and have a syntax that 'just works'. As you usualy do.

[pirj]

@arnoFleming I suggest you vote for 🚀 (enforce passing classes and strings exclusively as the first argument to example blocks).

Maybe I have very solid testing hygiene

It seems you really do.

[pirj]

I'd be okay with described_class changing to always be a class, but I don't think that's the purview of this project anyways.

@dvandersluis RSpec itself has to support legacy testing code between major version updates, but RuboCop-RSpec can enforce good practices here and now, preparing the users for the future changes, and preventing from making mistakes without making the spec suite red.

[pirj]

@julik It's innermost, not outermost. Agree that in shared examples the real value behind described_class can't be looked up, but that's the purpose of this abstraction, you may consider described_class as an argument that it accepts.

described_class misbehaves when an instance of a class is passed as the first argument to an example group.

I tend to think that "the thing under test":

• are both subject and described_class in case of class that can't be instantiated; those two are interchangeable
• is subject when in case when it can be instantiated; described_class can be used to make sure it produces an object of the same class, e.g., expect(subject.merge(a: 1)).to be_a(described_class)
• • one exception are classes that include Singleton; one would have to specify subject { described_class.instance } to use subject

[pirj]

@flanger001 It is a contrived example, but is demonstrating a real unfortunately.

In case described_class is used in that example group directly, or in an included example, its behaviour will be odd.

I've drafted a cop that on a test file results in:

spec/a_spec.rb:26:3: C: RSpec/ExampleGroupArgument: Bad first argument for example group
  describe [1,2,3] do ...
  ^^^^^^^^^^^^^^^^^^^
spec/a_spec.rb:29:3: C: RSpec/ExampleGroupArgument: Bad first argument for example group
  describe a: 1 do ...
  ^^^^^^^^^^^^^^^^
spec/a_spec.rb:32:3: C: RSpec/ExampleGroupArgument: Bad first argument for example group
  describe :sdfsf do ...
  ^^^^^^^^^^^^^^^^^^

Out of 13024 (spec) files from Real World Ruby apps there were no really no offences found with described_class usages inside an example group with non-string non-constant first argument. However, without the use of described_class in those example groups.

There were some cases with symbols being used as the first argument, but not many, just 60. This is not a recommended or canonical use, but is a matter of taste, and there's no real harm if described_class is not used down the lines.

Some examples below:

/Users/pirj/source/real-world-ruby-apps/apps/image_optim/spec/image_optim/cache_spec.rb:52:5: C: RSpec/ExampleGroupArgument: Bad first argument for example group
    describe :fetch do ...
/Users/pirj/source/real-world-ruby-apps/apps/puppet/spec/unit/etc_spec.rb:369:3: C: RSpec/ExampleGroupArgument: Bad first argument for example group
  describe :getgrent do ...

[pirj]

Closing as stale since there's no clear way RSpec is going with described_class in RSpec 4.0. We can reconsider this in the future.