`RSpec/SortMetadata` autocorrect can break context groups having additional description

[cbliard]

1. Create sample RSpec file like this:

RSpec.describe "Something", "in this context", :a, :b do
  subject { true }

  it "works well" do
    expect(subject).not_to be false
  end
end

2. Run it with documentation formatter: rspec --format documentation spec/test_spec.rb

Something in this context
  works well

Finished in 0.00172 seconds (files took 0.07348 seconds to load)
1 example, 0 failures

3. Run autocorrect: rubocop -A spec/test_spec.rb

Inspecting 1 file
C

Offenses:

spec/test_spec.rb:1:29: C: [Corrected] RSpec/SortMetadata: Sort metadata alphabetically.
RSpec.describe "Something", "in this context", :a, :b do
                            ^^^^^^^^^^^^^^^^^^^^^^^^^

1 file inspected, 1 offense detected, 1 offense corrected

File updated to:

RSpec.describe "Something", :a, :b, "in this context" do
  subject { true }

  it "works well" do
    expect(subject).not_to be false
  end
end

4. Run it again: rspec --format documentation spec/test_spec.rb

An error occurred while loading ./spec/test_spec.rb.
Failure/Error:
  RSpec.describe "Something", :a, :b, "in this context" do
    subject { true }

    it "works well" do
      expect(subject).not_to be false
    end
  end

ArgumentError:
  wrong number of arguments (given 4, expected 0..2)
# ./spec/test_spec.rb:1:in `<top (required)>'
No examples found.

Finished in 0.00001 seconds (files took 0.10503 seconds to load)
0 examples, 0 failures, 1 error occurred outside of examples

The number of arguments expected is from #build_description_from(parent_description=nil, my_description=nil) in lib/rspec/core/metadata.rb from rspec-core.

We sometimes use this ability to have an additional description parameter with RSpec.describe when writing spec for a specific feature flag or to split up a spec file in smaller parts when it becomes too big. Occasionally the RSpec/SortMetadata breaks it depending on the metadata we use by swapping the description and the symbols like above. We disable the cop for the line and that's ok, but it would be nice to have it fixed upstream.

It seems like it is somewhat an expected behavior though: https://github.com/rubocop/rubocop-rspec/blob/ff213aecd8579c638631d37a5de05468685a405a/spec/rubocop/cop/rspec/sort_metadata_spec.rb#L98-L111

[pirj]

Thanks for reporting. If I remember correctly, the second argument, if a string, is not considered metadata. There’s however, no confirmation for this in the code docs https://github.com/rspec/rspec-core/blob/2ba0f10fe68948e5ee9addd569b4694f0245aa71/lib/rspec/core/example_group.rb#L205

But there’s a related discussion here https://github.com/rspec/rspec/issues/40

We’re open to adjust the cop to match the RSpec semantics. Would you be interested in submitting a PR?

[cbliard]

Yes, I also searched for reference of this in the docs and in the Cucumber scenarios but without any success.

The args are passed to RSpec::Core::ExampleGroup.set_it_up where it's modified by Metadata.build_hash_from(args) which does hash[args.pop] = true while args.last.is_a?(Symbol)(source). Then from this point args only contains the strings. They are passed to Metadata::HashPopulator#populate which uses them to build up the description.

I'm not sure why it's not documented because it has been quite useful for us. The usage I'm referring to is in this spec.

Would you be interested in submitting a PR?

Yes, once we agree on the RSpec semantics, that could be interesting.

[pirj]

This is the spec we’re after https://github.com/rspec/rspec-core/blob/2ba0f10fe68948e5ee9addd569b4694f0245aa71/spec/rspec/core/example_group_spec.rb#L636

[pirj]

There’s also this PR. In the search, it mentions the “second docstring”, but I can’t quickly find it. Probably it’s in some collapsed comment thread or an older version of PR description.

[pirj]

Here’s the interesting part https://github.com/rspec/rspec-core/pull/2681#discussion_r361811453

I think the takeaway here is that the second string argument works as an additional docstring. And will still be, for years to come. It is not, and will never be considered metadata, because it’s not a symbol.

Basing on this, it makes sense to exclude literal str/xstr nodes from sorting. With static analysis it’s impossible to tell reliably if it’s a variable or a method, what type it is, but it doesn’t make sense to include it to sorted metadata, too:

describe "foo", may_be_a_string_or_a_symbol, :c, :d do

Only literal symbols should be passed to sorting.

I have some doubts that this weird out-of-order zoo of arguments is even accepted by RSpec if the variable holds a symbol:

it 'Something', variable, 'B', :a

What do you think?

[cbliard]

Nice findings!

I have some doubts that this weird out-of-order zoo of arguments is even accepted by RSpec if the variable holds a symbol:

it 'Something', variable, 'B', :a

In an example definition, only the first arg is considered the description, then the rest is used to build up metadata. When metadata is build up from args, only the last arg hash and the last symbol args are used. Anything else is discarded.

So it 'Something', variable, 'B', :a is equivalent to it 'Something', :a regardless of variable holding a symbol or a string, because symbols are read from the end of the args list, and processing stops because of 'B' not being a symbol. It does not raise any error though, extra args are silently discarded.

Same for shared_context/shared_examples, it is also using Metadata.build_hash_from(metadata_args) and then discards the args.

It's only for describe/context blocks that an additional docstring is possible.

And for this kind of usage:

> describe 'Something', variable, 'B', :a

It will fail because there are too many remaining args once the metadata are extracted from the args (['Something', variable, 'B'] after extracting :a from args). Maybe a cop could help detect this and warn. Or maybe the SortMetadata cop could reorder them to 'Something', 'B', variable, :a which has a chance to pass if variable is holding a symbol.

What the cop could do anyway is to always sort like this:

• for describe/context:
  • string (will fail if more than one, should an additional cop detect this?)
  • variables, not sorted because the first one could be a string, and the remaining ones symbols
  • symbols, sorted
  • hash, sorted
• for it/shared_examples:
  • variables, sorted
  • symbols, sorted
  • hash, sorted

Would that work?

[pirj]

Nice! A few practical examples to what you state above to help to dissect this:

Example group with string metadata-wannabe: fail vs swallow

RSpec.describe "docstring", :real, "fake" do

it fails with:

ArgumentError:
  wrong number of arguments (given 3, expected 0..2)
# /Users/pirj/source/rspec-dev/repos/rspec-core/lib/rspec/core/metadata.rb:178:in `build_description_from'
#

But it just swallows any non-symbol:

it "docstring", :swallowed, "swallowed" do

Tertiary docstring: fail vs swallow

it tolerates and swallows any "docstrings" after secondary:

it "docstring", "swallowed", "swallowed", "swallowed", :real_metadata do

with no regards if there is or not any real metadata. It swallows strings, lambdas, hashes, anything.

describe (as you describe this above) fails loudly when more than one secondary docstring is passed:

RSpec.describe "Primary docstring", "secondary", "BOOM" do

ArgumentError:
  wrong number of arguments (given 3, expected 0..2)
# /Users/pirj/source/rspec-dev/repos/rspec-core/lib/rspec/core/metadata.rb:178:in `build_description_from'

Maybe a cop could help detect this and warn

If there's an RSpec error (and there is for describe), or even just a warning, we don't care. There is no way we can safely autocorrect this, so it won't be of any use as on-the-fly tool in an IDE.

I agree with your conclusion, with one correction:

for it/shared_examples: variables, sorted

docsting = "docstring"
about = :about

it docstring, about do
end

will result in the lost docstring. I suggest not to sort variables.

Does this change to conclusion sound reasonable?

[pirj]

I'll also leave this small reference here just in case.

[cbliard]

Thanks for the clear examples.

will result in the lost docstring. I suggest not to sort variables.

Nice catch.

Does this change to conclusion sound reasonable?

Yes, perfect. I'll start working on a PR to fix this.

[cbliard]

Actually, this is not correct because of this case:

metadata = { foo: :bar }

it "works", :about, metadata do
end

If the cop blindly changes the order to [strings, variables, symbols, hash], then it change it to this and produce an error:

metadata = { foo: :bar }

it "works", metadata, :about do
end

I feel like the last arg should be kept last if it's a hash or a variable, then apply the sorting [strings, other args, symbols].

Does it sound reasonable?

[pirj]

Absolutely!