rubocop / rubocop-rspec

Code style checking for RSpec files.
https://docs.rubocop.org/rubocop-rspec
MIT License
809 stars 276 forks source link

RSpec/FilePath and metadata #1137

Open schwern opened 3 years ago

schwern commented 3 years ago

We have some spec files which are isolated by their metadata. For example, specs for services which touch real APIs using test accounts. We want them to be completely isolated from other tests.

# spec/some_service/live_spec.rb

describe SomeService, :live do
  # tests which contact real services
end

This is in spec/some_service/live_spec.rb. RSpec/FilePath does not like this and I don't see any way to configure it to be satisfied. If we put it into spec/some_service_spec.rb then RSpec/MultipleDescribes is unhappy. I do not want to nest it inside the other describe SomeService for risk of the configuration of the live tests polluting the mock tests, or vice-versa.

Is there a preferred way of handling this?

pirj commented 3 years ago

Do you use this metadata in some way, or is it there for purely informational purpose?

An option to trick the cop would be to:

describe SomeService, 'live', :live do

There is certainly some duplication, but at the same time documentation formatter output makes it clear what this example group is about.

Do you have some change to the cop in mind?

schwern commented 3 years ago

Do you use this metadata in some way, or is it there for purely informational purpose?

By default, live tests are skipped. They are run as a release test.

RSpec.configure do |config|
  config.filter_run_excluding live: true unless ENV["TEST_ALL"]
end

I would prefer not to add a fake method to the describe. That would be bad style which defeats the point.

Perhaps the cop could be configured to treat metadata like methods? So spec/some_service/live_spec.rb is acceptable for both describe SomeService, :live and describe SomeService, '#live'. I'm also open to changing how we handle these tests.

pirj commented 3 years ago

I'm puzzled to find a better option off the top of my head. On the other head I'm not certain if this is a widespread naming scheme, and in this case, I'm not convinced to introduce such an option.

pirj commented 2 years ago

Frankly, I find it puzzling why the following passes:

    expect_no_offenses(<<-RUBY, 'foo_bar_spec.rb')
      describe Foo do; end
    RUBY

while this fails:

    expect_no_offenses(<<-RUBY, 'foo/bar_spec.rb')
      describe Foo do; end
    RUBY

Was it a deliberate design decision?

What do you think of relaxing the cop to allow a more flexible directory structure, @bquorning, @Darhazer ?

The ticket in question is just one case, I hope I could hint at the other with the above examples.

real-world-rspec highlights:

57497 files inspected, 1999 offenses detected


/Users/pirj/source/real-world-rspec/spree/core/spec/models/spree/order/updating_spec.rb:3:1: C: RSpec/FilePath: Spec path should end with spree/order*_spec.rb.
describe Spree::Order, type: :model do
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

solidus/core/spec/models/spree/order/state_machine_spec.rb:5:1: C: RSpec/FilePath: Spec path should end with spree/order*_spec.rb. RSpec.describe Spree::Order, type: :model do ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

rspec-core/spec/rspec/core_spec.rb:3:1: C: RSpec/FilePath: Spec path should end with rspec*_spec.rb. RSpec.describe RSpec do ^^^^^^^^^^^^^^^^^^^^

refinerycms/core/spec/lib/refinery/core_spec.rb:3:1: C: RSpec/FilePath: Spec path should end with refinery*_spec.rb. describe Refinery do ^^^^^^^^^^^^^^^^^

gitlabhq/spec/services/ci/create_pipeline_service/tags_spec.rb:4:1: C: RSpec/FilePath: Spec path should end with ci/create_pipeline_service*_spe c.rb. RSpec.describe Ci::CreatePipelineService do ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

canvas-lms/spec/models/content_migration/course_copy_pace_plans_spec.rb:22:1: C: RSpec/FilePath: Spec path should end with content_migration*_sp ec.rb. describe ContentMigration do ^^^^^^^^^^^^^^^^^^^^^^^^^