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/MultipleSubjects and RSpec/LeadingSubject autocorrect removes subject used in tests #1835

Open
boardfish opened this issue Mar 14, 2024 · 2 comments
Labels

Comments

@boardfish
Copy link

boardfish commented Mar 14, 2024

Given this code:

describe "Something" do
  let(:something_unrelated) { "i exist" }

  subject(:named_subject) { 1 }
  subject do
    2
  end

  it "should be 2" do
    expect(subject).to eq(2)
  end
end

And this reduced example command (assuming a config that requires rubocop-rspec):

bundle exec rubocop --only RSpec/MultipleSubjects,RSpec/LeadingSubject spec/example_spec.rb -a -d

This autocorrects to:

describe "Something" do
  subject(:named_subject) { 1 }
  let(:something_unrelated) { "i exist" }


  it "should be 2" do
    expect(subject).to eq(2)
  end
end

Output:

Inspecting 1 file
C

Offenses:

spec/example_spec.rb:2:3: C: [Corrected] RSpec/MultipleSubjects: Do not set more than one subject per example group
  subject do ...
  ^^^^^^^^^^
spec/example_spec.rb:4:3: C: [Corrected] RSpec/LeadingSubject: Declare subject above any other let declarations.
  subject(:named_subject) { 1 }
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
spec/example_spec.rb:4:3: C: [Corrected] RSpec/MultipleSubjects: Do not set more than one subject per example group
  subject(:named_subject) { 1 }
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
spec/example_spec.rb:5:3: C: [Corrected] RSpec/LeadingSubject: Declare subject above any other let declarations.
  subject do ...
  ^^^^^^^^^^

I'm guessing the sequence of events is something like this:

  1. RSpec/MultipleSubjects no-ops. Ordinarily, if executed alone it would turn the named subject into a let.
  2. RSpec/LeadingSubject moves the named subject to the start of the block.
  3. RSpec/LeadingSubject moves the unnamed subject to the start of the block:
describe "Something" do
  subject do
    2
  end
  subject(:named_subject) { 1 }
  let(:something_unrelated) { "i exist" }


  it "should be 2" do
    expect(subject).to eq(2)
  end
end
  1. MultipleSubjects removes the unnamed subject because it will be overridden by the declaration of the named_subject.

This makes the test fail.


This was based on some relatively recent code (albeit a misuse of subjects). I agree with the autocorrect behaviour of MultipleSubjects in concept, but I'm not sure what should be done to prevent the conflict of these two cops.

@pirj
Copy link
Member

pirj commented Mar 14, 2024

Thanks for reporting. Do you think we should avoid changing the order of subject definitions and move on?

@boardfish
Copy link
Author

boardfish commented Mar 14, 2024

I think that's one possible option, and is probably the right one – I feel as though we should only avoid doing it if there are multiple subjects in the context, but I don't know how practical it is to suggest that. I've also considered:

  1. Remove reordering of subject definitions entirely.
  2. Get broader context of whether subject was called in the current context and use it to decide how to act here.
  3. Mark LeadingSubject and/or MultipleSubjects as unsafe for autocorrect.
  4. Have one cop act differently if another is being executed (unlikely to be possible).

@ydah ydah added the bug label Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants