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

Allow more lines on scenario examples #210

Open
ehannes opened this issue Sep 12, 2016 · 18 comments
Open

Allow more lines on scenario examples #210

ehannes opened this issue Sep 12, 2016 · 18 comments

Comments

@ehannes
Copy link

ehannes commented Sep 12, 2016

I know I can set the allowed length of examples with:

RSpec/ExampleLength:
  Max: 19

Would it be possible to implement a specific limit to scenario examples (those examples used in feature specs)? For performance reason, especially when testing JavaScript, I tend to allow more than one example per scenario. Something like this:

RSpec/ExampleLength:
  Max: 5
RSpec/ScenarioLength:
  Max: 20
@backus
Copy link
Collaborator

backus commented Sep 12, 2016

I'd be open to something like that (ideally not a totally new cop). I don't use scenarios so I probably wont implement that in my free time but if you want to propose a specific implementation solution I'd be fine with accepting a PR for this

@ehannes
Copy link
Author

ehannes commented Sep 13, 2016

If one would like to implement this, where would you start?

@ehannes
Copy link
Author

ehannes commented Sep 13, 2016

This issue also concerns RSpec/MultipleExpectations.

@backus
Copy link
Collaborator

backus commented Sep 13, 2016

This is actually an issue for many of our cops (and I haven't been sure how to improve rubocop-rspec to fit the various use cases) so how do you feel about something like this:

---
AllCops:
  RSpec:
    Patterns:
    - _spec.rb
    - "(?:^|/)spec/"
    SpecTypes:
      Feature: 'spec/features/**/*'
      Unit: 'spec/unit/**/*'
      Scenario: 'spec/scenario/**/*'

# ...

RSpec/ExampleLength:
  Description: Checks for long examples.
  Enabled: true
  Max: 5
  SpecType:
    Feature:
      Max: 10
    Scenario:
      Max: 10

The idea here is that you could specify any arbitrary mapping of a spec type name to a file glob in the AllCops/RSpec/SpecTypes section. Then, in a cop that supports spec type overrides, you could use those spec type names you defined to define settings for just that part of your application. Seems like this would solve a lot more problems than adding new slightly different cops would.

@ehannes @bquorning @andyw8 thoughts on this approach?

@bquorning
Copy link
Collaborator

bquorning commented Sep 14, 2016

As I recall it, rspec-rails will tag your specs with a type, e.g. type: :feature, type: :controller, type: :view etc. I wonder if these types could be used for the mapping you’re looking for.

I realize that not all people use RSpec together with Rails, so perhaps this is a bad suggestion.

@ehannes
Copy link
Author

ehannes commented Sep 14, 2016

@bquorning Good point. It's true that rspec-rails uses types in specs (see rspec-rails Directory Structure). It would be sad if this this gem was dependent on rspec-rails, so the question is if you can use this feature in rspec as well. I have not tried yet, but I found that you can add type: :special to specific examples in rspec (see rspec --tag option).

@bquorning
Copy link
Collaborator

You can add metadata based on directory structure using config.define_derived_metadata: http://www.rubydoc.info/gems/rspec-core/RSpec%2FCore%2FConfiguration%3Adefine_derived_metadata

I guess the question is whether we want to force part of the configuration into people’s spec_helper.rb instead of their .rubocop.yml.

@ehannes
Copy link
Author

ehannes commented Sep 14, 2016

I tried the following:

# spec/support/integration_helper.rb
module IntegrationHelper
  # Includes things that integration tests need
end
# spec/integrations/test_class_spec.rb
describe TestClass, type: :integration do
  # Some examples that need IntegrationHelper
end
# spec/spec_helper.rb
RSpec.configure do |config|
# ...
  config.include IntegrationHelper, type: :integration
# ...
end

And it works like a charm.

@ehannes
Copy link
Author

ehannes commented Sep 14, 2016

I would rather add type: :whatever as I please to my specs than be forced to set that specific configuration in spec_helper.rb. Also, some users may use the type metadata for something else and forcing them to set the type according to the directory structure may break things for them.

@ehannes
Copy link
Author

ehannes commented Sep 14, 2016

If you use type, I guess you would get a .rubocop.xml file that looks something like this:

...
    SpecTypes:
      Feature: :feature
      Unit: :unit
      Whatever: :whatever

@backus
Copy link
Collaborator

backus commented Sep 14, 2016

I'd rather just define the globs. It would be extremely hard to statically determine the types for different specs.

@ehannes
Copy link
Author

ehannes commented Sep 15, 2016

@backus With globs, do you mean the approach you proposed at first, or the approach @bquorning suggested (using the spec_helper.rb)?

@backus
Copy link
Collaborator

backus commented Sep 15, 2016

@ehannes the explicit globs in the config file. IMO we couldn't reliably know what the different spec types are without loading the user's rspec environment and this gem is not in the business of dynamic analysis

@ehannes
Copy link
Author

ehannes commented Dec 6, 2016

I found a new feature in RSpec 3.3 today: aggregate_failures. This clearly requires a change to RSpec/MultipleExpectations since the purpose of it is to allow more than one expectation per example.

@backus
Copy link
Collaborator

backus commented Dec 6, 2016

Yeah I added #233 for that reason

@ehannes
Copy link
Author

ehannes commented Dec 7, 2016

Should we move this issue from "Discussion" to "Enhancement"? "Explicit globs" seems like the way to go.

@backus
Copy link
Collaborator

backus commented Dec 11, 2016

Sure!

@joshuapinter
Copy link

I, too, would love this. Feature/Integration specs tend to be much longer so it forces you to either disable the 👮 altogether or bump up the Max setting, which eliminates any value from it.

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

No branches or pull requests

5 participants