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

2.99 does not warn properly when using an inner describe :symbol #2025

Closed
myronmarston opened this issue Jul 9, 2015 · 6 comments
Closed
Assignees

Comments

@myronmarston
Copy link
Member

See rspec/rspec-rails#1415 for an example user report. 2.99 should warn for this case but it does not. The problem is here:

https://github.com/rspec/rspec-core/blob/v2.99.2/lib/rspec/core/metadata.rb#L184

It still treats symbols as a special case like strings. Compare to:

https://github.com/rspec/rspec-core/blob/v3.3.1/lib/rspec/core/metadata.rb#L298

...which does not treat symbols as a special case.

@xaviershay
Copy link
Member

Appropriate warning for this would be https://github.com/rspec/rspec-core/blob/v2.99.2/lib/rspec/core/metadata.rb#L139, right?

@xaviershay xaviershay self-assigned this Aug 7, 2015
@xaviershay
Copy link
Member

A warning is triggered currently only if subject is used. So the following will issue a warning (and we have a test case for exactly this):

describe String do
  describe :create do
    it 'works' do
      subject
    end
  end
end

But if you remove subject it does not warn... but also it works just fine in RSpec 3. So I see a couple of options here:

  1. Minimal repro provided in Upgrade from 2.99 to 3.3 is causing controller test failures rspec-rails#1415 is not correct and this is actually a different problem. @jeffheifetz might be too long ago for you to care/remember, but tagging you in case some other context occurs to you.
  2. rspec-rails is doing something weird to trigger it. I'm not sure how to get a set up to test this within a reasonable amount of time.

If no-one has any bright ideas, I recommend closing this as "cannot reproduce".

@myronmarston @JonRowe

(Thanks to @SalvatoreT who helped me look into this.)

@xaviershay xaviershay removed the Small label Aug 7, 2015
@JonRowe
Copy link
Member

JonRowe commented Aug 8, 2015

rspec-rails is doing something weird to trigger it. I'm not sure how to get a set up to test this within a reasonable amount of time.

How about described_class? Thats what we're using to signal to the Rails stuff which the controlled is, would also change with a nested describe, and might not be triggering the warning.

@xaviershay
Copy link
Member

https://github.com/rspec/rspec-core/blob/2-99-maintenance/spec/rspec/core/example_group_spec.rb#L360 suggests that described_class warnings are working as intended.

@myronmarston
Copy link
Member Author

I was on vacation Wednesday until today so I haven't had a chance to look into this yet. I hope to in the next day or so.

@myronmarston
Copy link
Member Author

I think you're right that this is handled properly. My confusion was comparing https://github.com/rspec/rspec-core/blob/v2.99.2/lib/rspec/core/metadata.rb#L184 vs https://github.com/rspec/rspec-core/blob/v3.3.1/lib/rspec/core/metadata.rb#L298 -- it looks like 2.99 is wrongly treating Symbol as a special case in that method. Looking more into it, the Symbol case is handled here instead:

https://github.com/rspec/rspec-core/blob/v2.99.2/lib/rspec/core/example_group.rb#L327-L331

I agree with closing this, barring a clear repro case of a situation where 2.99 does not warn but should.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants