Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RSpec/SortMetadata autocorrect can break context groups having additional description #1946

Open
cbliard opened this issue Aug 6, 2024 · 11 comments · May be fixed by #1948
Open

RSpec/SortMetadata autocorrect can break context groups having additional description #1946

cbliard opened this issue Aug 6, 2024 · 11 comments · May be fixed by #1948
Labels

Comments

@cbliard
Copy link
Contributor

cbliard commented Aug 6, 2024

  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
  1. 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
  1. 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
  1. 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:

it 'registers an offense when using mixed metadata ' \
'and both symbols metadata and hash keys are not in alphabetical order ' \
'and the hash values are complex objects' do
expect_offense(<<~RUBY)
it 'Something', variable, 'B', :a, key => {}, foo: ->(x) { bar(x) }, Identifier.sample => true, baz: Snafu.new do
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Sort metadata alphabetically.
end
RUBY
expect_correction(<<~RUBY)
it 'Something', :a, 'B', variable, baz: Snafu.new, foo: ->(x) { bar(x) }, Identifier.sample => true, key => {} do
end
RUBY
end

@pirj
Copy link
Member

pirj commented Aug 6, 2024

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 rspec/rspec#40

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

@cbliard
Copy link
Contributor Author

cbliard commented Aug 6, 2024

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
Copy link
Member

pirj commented Aug 6, 2024

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.
image

@pirj
Copy link
Member

pirj commented Aug 6, 2024

Here’s the interesting part rspec/rspec-core#2681 (comment)

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
Copy link
Contributor Author

cbliard commented Aug 7, 2024

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
Copy link
Member

pirj commented Aug 7, 2024

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
Copy link
Member

pirj commented Aug 7, 2024

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

@cbliard
Copy link
Contributor Author

cbliard commented Aug 8, 2024

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
Copy link
Contributor Author

cbliard commented Aug 8, 2024

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
Copy link
Member

pirj commented Aug 8, 2024

Absolutely!

cbliard added a commit to cbliard/rubocop-rspec that referenced this issue Aug 12, 2024
Fixes rubocop#1946.

Symbols in metadata are processed by RSpec only when they are positioned
last, meaning the other parameter types must be positioned before the
symbols. RSpec `context`/`describe` accepts second non-symbol argument
as an additional description which is why strings are sorted first.
@cbliard cbliard linked a pull request Aug 12, 2024 that will close this issue
6 tasks
cbliard added a commit to cbliard/rubocop-rspec that referenced this issue Sep 11, 2024
Fixes rubocop#1946.

Symbols in metadata are processed by RSpec only when they are positioned
last, meaning the other parameter types must be positioned before the
symbols. RSpec `context`/`describe` accepts second non-symbol argument
as an additional description which is why strings are sorted first.
cbliard added a commit to cbliard/rubocop-rspec that referenced this issue Oct 10, 2024
Metadata processed by RSpec is:
- the last argument when it's a hash
- the trailing arguments when they are symbols

Only this metadata is sorted by this cop.

If the second argument to a `context`/`describe` block is used as an
additional description, it is not sorted anymore. This fixes rubocop#1946.
cbliard added a commit to cbliard/rubocop-rspec that referenced this issue Oct 10, 2024
Metadata processed by RSpec is:
- the last argument when it's a hash
- the trailing arguments when they are symbols

Only this metadata is sorted by this cop.

If the second argument to a `context`/`describe` block is used as an
additional description, it is not sorted anymore. This fixes rubocop#1946.
@ydah ydah added the bug label Oct 21, 2024
cbliard added a commit to cbliard/rubocop-rspec that referenced this issue Dec 23, 2024
Metadata processed by RSpec is:
- the last argument when it's a hash
- the trailing arguments when they are symbols

Only this metadata is sorted by this cop.

If the second argument to a `context`/`describe` block is used as an
additional description, it is not sorted anymore. This fixes rubocop#1946.

Co-authored-by: Phil Pirozhkov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants