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

Add new RSpec/FileFixture cop #1214

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -651,3 +651,4 @@ Compatibility release so users can upgrade RuboCop to 0.51.0. No new features.
[@dswij]: https://github.com/dswij
[@francois-ferrandis]: https://github.com/francois-ferrandis
[@r7kamura]: https://github.com/r7kamura
[@leoarnold]: https://github.com/leoarnold
1 change: 1 addition & 0 deletions changelog/new_add_new_rspecfilefixture_cop.md
@@ -0,0 +1 @@
* [#1214](https://github.com/rubocop/rubocop-rails/pull/1214): Add new `RSpec/FileFixture` cop. ([@leoarnold][])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're still using the old approach with just one CHANGELOG.md in this repo - it's not a major pain to merge it.

6 changes: 6 additions & 0 deletions config/default.yml
Expand Up @@ -353,6 +353,12 @@ RSpec/ExpectOutput:
VersionAdded: '1.10'
StyleGuide: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/ExpectOutput

RSpec/FileFixture:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have Rails sub-department.

Description: Favor the use of `file_fixture`.
Enabled: pending
VersionAdded: 2.7.0
StyleGuide: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/FileFixture

RSpec/FilePath:
Description: Checks that spec file paths are consistent and well-formed.
Enabled: true
Expand Down
1 change: 1 addition & 0 deletions docs/modules/ROOT/pages/cops.adoc
Expand Up @@ -32,6 +32,7 @@
* xref:cops_rspec.adoc#rspecexpectchange[RSpec/ExpectChange]
* xref:cops_rspec.adoc#rspecexpectinhook[RSpec/ExpectInHook]
* xref:cops_rspec.adoc#rspecexpectoutput[RSpec/ExpectOutput]
* xref:cops_rspec.adoc#rspecfilefixture[RSpec/FileFixture]
* xref:cops_rspec.adoc#rspecfilepath[RSpec/FilePath]
* xref:cops_rspec.adoc#rspecfocus[RSpec/Focus]
* xref:cops_rspec.adoc#rspechookargument[RSpec/HookArgument]
Expand Down
55 changes: 55 additions & 0 deletions docs/modules/ROOT/pages/cops_rspec.adoc
Expand Up @@ -1491,6 +1491,61 @@ expect { my_app.print_report }.to output('Hello World').to_stdout

* https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/ExpectOutput

== RSpec/FileFixture

|===
| Enabled by default | Safe | Supports autocorrection | Version Added | Version Changed

| Pending
| Yes
| Yes
| 2.7.0
| -
|===

Favor the use of `file_fixture`.

`RSpec` provides the convenience method

[source,ruby]
----
file_fixture('path/some_file.csv')
----

which (by default) is equivalent to

[source,ruby]
----
Rails.root.join('spec/fixtures/files/path/some_file.csv')
----

=== Examples

[source,ruby]
----
# bad
"#{Rails.root}/spec/fixtures/path/some_file.csv"
Rails.root.join('spec/fixtures/path/some_file.csv')
Rails.root.join('spec', 'fixtures', 'path', 'some_file.csv')

"#{Rails.root}/spec/fixtures/files/some_file.csv"
Rails.root.join('spec/fixtures/files/some_file.csv')
Rails.root.join('spec', 'fixtures', 'files', 'some_file.csv')

# good
file_fixture('../path/some_file.csv').path
file_fixture('../path/some_file.csv')
file_fixture('../path/some_file.csv')

file_fixture('some_file.csv').path
file_fixture('some_file.csv')
file_fixture('some_file.csv')
----

=== References

* https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/FileFixture

== RSpec/FilePath

|===
Expand Down
107 changes: 107 additions & 0 deletions lib/rubocop/cop/rspec/file_fixture.rb
@@ -0,0 +1,107 @@
# frozen_string_literal: true

module RuboCop
module Cop
module RSpec
# Favor the use of `file_fixture`.
#
# `RSpec` provides the convenience method
#
# [source,ruby]
# ----
# file_fixture('path/some_file.csv')
# ----
#
# which (by default) is equivalent to
#
# [source,ruby]
# ----
# Rails.root.join('spec/fixtures/files/path/some_file.csv')
# ----
#
# @example
# # bad
# "#{Rails.root}/spec/fixtures/path/some_file.csv"
# Rails.root.join('spec/fixtures/path/some_file.csv')
# Rails.root.join('spec', 'fixtures', 'path', 'some_file.csv')
#
# "#{Rails.root}/spec/fixtures/files/some_file.csv"
# Rails.root.join('spec/fixtures/files/some_file.csv')
# Rails.root.join('spec', 'fixtures', 'files', 'some_file.csv')
#
# # good
# file_fixture('../path/some_file.csv').path
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is path a Pathname method? I couldn't find it.

# file_fixture('../path/some_file.csv')
# file_fixture('../path/some_file.csv')
#
# file_fixture('some_file.csv').path
# file_fixture('some_file.csv')
# file_fixture('some_file.csv')
#
class FileFixture < Base
extend AutoCorrector

MSG = 'Use `file_fixture`.'

RESTRICT_ON_SEND = %i[join root].freeze

DEFAULT_FILE_FIXTURE_PATTERN = %r{^spec/fixtures/files/}.freeze
DEFAULT_FIXTURE_PATTERN = %r{^spec/fixtures/}.freeze

# @!method file_io?(node)
def_node_matcher :file_io?, <<~PATTERN
(send
(const {nil? cbase} :File) {:binread :binwrite :open :read :write} $...)
PATTERN

# @!method rails_root_join(node)
def_node_matcher :rails_root_join, <<~PATTERN
(send
(send
(const {nil? cbase} :Rails) :root) :join (str $_)+)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if it's not a literal string passed to join? E.g. a string variable or an interpolated string.

PATTERN

# @!method dstr_rails_root(node)
def_node_matcher :dstr_rails_root, <<~PATTERN
(dstr
(begin
(send
(const {nil? cbase} :Rails) :root))
(str $_))
PATTERN

def on_send(node)
return unless (crime_scene, strings, method = evidence(node))
return unless (new_path = new_path(strings))

add_offense(crime_scene) do |corrector|
replacement = "file_fixture('#{new_path}')"
replacement += ".#{method}" if method

corrector.replace(crime_scene, replacement)
end
end

private

def evidence(node)
if !file_io?(node.parent) && (string = rails_root_join(node))
[node, string, nil]
elsif (string = dstr_rails_root(node.parent.parent))
[node.parent.parent, string.gsub(%r{^/}, ''), :path]
end
end

def new_path(strings)
path = File.join(strings)

if DEFAULT_FILE_FIXTURE_PATTERN.match?(path)
path.gsub(DEFAULT_FILE_FIXTURE_PATTERN, '')
elsif DEFAULT_FIXTURE_PATTERN.match?(path)
path.gsub(DEFAULT_FIXTURE_PATTERN, '../')
end
end
end
end
end
end
1 change: 1 addition & 0 deletions lib/rubocop/cop/rspec_cops.rb
Expand Up @@ -45,6 +45,7 @@
require_relative 'rspec/expect_change'
require_relative 'rspec/expect_in_hook'
require_relative 'rspec/expect_output'
require_relative 'rspec/file_fixture'
require_relative 'rspec/file_path'
require_relative 'rspec/focus'
require_relative 'rspec/hook_argument'
Expand Down
145 changes: 145 additions & 0 deletions spec/rubocop/cop/rspec/file_fixture_spec.rb
@@ -0,0 +1,145 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::RSpec::FileFixture, :config do
it 'registers an offense when using ' \
'`"\#{Rails.root}/spec/fixtures/files/some_file.csv"`' do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This docstring just duplicates what the example itself tells. Does it make sense to write a free-form text explaining this case?

expect_offense(<<~RUBY)
"\#{Rails.root}/spec/fixtures/files/some_file.csv"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's possible to write heredoc as <<~'RUBY' to avoid escaping the hash.

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `file_fixture`.
RUBY

expect_correction(<<~RUBY)
file_fixture('some_file.csv').path
RUBY
end

it 'registers an offense when using ' \
'`"\#{::Rails.root}/spec/fixtures/path/some_file.csv"`' do
expect_offense(<<~RUBY)
"\#{::Rails.root}/spec/fixtures/path/some_file.csv"
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `file_fixture`.
RUBY

expect_correction(<<~RUBY)
file_fixture('../path/some_file.csv').path
RUBY
end

it 'registers an offense when using ' \
"`Rails.root.join('spec/fixtures/files/some_file.csv')`" do
expect_offense(<<~RUBY)
Rails.root.join('spec/fixtures/files/some_file.csv')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `file_fixture`.
RUBY

expect_correction(<<~RUBY)
file_fixture('some_file.csv')
RUBY
end

it 'registers an offense when using ' \
"`::Rails.root.join('spec/fixtures/path/some_file.csv')`" do
expect_offense(<<~RUBY)
::Rails.root.join('spec/fixtures/path/some_file.csv')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `file_fixture`.
RUBY

expect_correction(<<~RUBY)
file_fixture('../path/some_file.csv')
RUBY
end

it 'registers an offense when using ' \
"`Rails.root.join('spec', 'fixtures/path/some_file.csv')`" do
expect_offense(<<~RUBY)
Rails.root.join('spec', 'fixtures/path/some_file.csv')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `file_fixture`.
RUBY

expect_correction(<<~RUBY)
file_fixture('../path/some_file.csv')
RUBY
end

it 'registers an offense when using ' \
"`Rails.root.join('spec/fixtures', 'path/some_file.csv')`" do
expect_offense(<<~RUBY)
Rails.root.join('spec/fixtures', 'path/some_file.csv')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `file_fixture`.
RUBY

expect_correction(<<~RUBY)
file_fixture('../path/some_file.csv')
RUBY
end

it 'registers an offense when using ' \
"`Rails.root.join('spec/fixtures/path', 'some_file.csv')`" do
expect_offense(<<~RUBY)
Rails.root.join('spec/fixtures/path', 'some_file.csv')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `file_fixture`.
RUBY

expect_correction(<<~RUBY)
file_fixture('../path/some_file.csv')
RUBY
end

it 'registers an offense when using ' \
"`Rails.root.join('spec', 'fixtures', 'path/some_file.csv')`" do
expect_offense(<<~RUBY)
Rails.root.join('spec', 'fixtures', 'path/some_file.csv')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `file_fixture`.
RUBY

expect_correction(<<~RUBY)
file_fixture('../path/some_file.csv')
RUBY
end

it 'registers an offense when using ' \
"`Rails.root.join('spec', 'fixtures/path', 'some_file.csv')`" do
expect_offense(<<~RUBY)
Rails.root.join('spec', 'fixtures/path', 'some_file.csv')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `file_fixture`.
RUBY

expect_correction(<<~RUBY)
file_fixture('../path/some_file.csv')
RUBY
end

it 'registers an offense when using ' \
"`Rails.root.join('spec/fixtures', 'path', 'some_file.csv')`" do
expect_offense(<<~RUBY)
Rails.root.join('spec/fixtures', 'path', 'some_file.csv')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `file_fixture`.
RUBY

expect_correction(<<~RUBY)
file_fixture('../path/some_file.csv')
RUBY
end

it 'registers an offense when using ' \
"`Rails.root.join('spec', 'fixtures', 'path', 'some_file.csv')`" do
expect_offense(<<~RUBY)
Rails.root.join('spec', 'fixtures', 'path', 'some_file.csv')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `file_fixture`.
RUBY

expect_correction(<<~RUBY)
file_fixture('../path/some_file.csv')
RUBY
end

%i[binread binwrite open read write].each do |method|
# This is handled by `Rails/RootPathnameMethods`
it 'does not register an offense when using ' \
"`File.#{method}(Rails.root.join(...))`" do
expect_no_offenses(<<~RUBY)
File.#{method}(Rails.root.join('spec', 'fixtures', 'path', 'some_file.csv'))
RUBY
end
end
end