From 9f46488805e86b1bc341ea1620b866016c2ce5ed Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 20 May 2020 14:34:42 +0000 Subject: Add latest changes from gitlab-org/gitlab@13-0-stable-ee --- ...id_keyword_arguments_in_sidekiq_workers_spec.rb | 49 ++++ spec/rubocop/cop/gitlab/change_timezone_spec.rb | 21 ++ spec/rubocop/cop/gitlab/json_spec.rb | 39 +++ .../cop/inject_enterprise_edition_module_spec.rb | 11 + .../cop/migration/add_column_with_default_spec.rb | 45 +--- .../migration/add_columns_to_wide_tables_spec.rb | 4 +- .../migration/add_concurrent_foreign_key_spec.rb | 6 + .../migration/add_limit_to_string_columns_spec.rb | 268 --------------------- .../migration/add_limit_to_text_columns_spec.rb | 160 ++++++++++++ spec/rubocop/cop/migration/prevent_strings_spec.rb | 143 +++++++++++ .../reversible_add_column_with_default_spec.rb | 43 ---- .../with_lock_retries_disallowed_method_spec.rb | 68 ++++++ ...th_lock_retries_without_ddl_transaction_spec.rb | 46 ---- .../ar_exists_and_present_blank_spec.rb | 111 +++++++++ .../rspec/empty_line_after_shared_example_spec.rb | 86 +++++++ spec/rubocop/cop/rspec/env_assignment_spec.rb | 12 +- 16 files changed, 710 insertions(+), 402 deletions(-) create mode 100644 spec/rubocop/cop/avoid_keyword_arguments_in_sidekiq_workers_spec.rb create mode 100644 spec/rubocop/cop/gitlab/change_timezone_spec.rb create mode 100644 spec/rubocop/cop/gitlab/json_spec.rb delete mode 100644 spec/rubocop/cop/migration/add_limit_to_string_columns_spec.rb create mode 100644 spec/rubocop/cop/migration/add_limit_to_text_columns_spec.rb create mode 100644 spec/rubocop/cop/migration/prevent_strings_spec.rb delete mode 100644 spec/rubocop/cop/migration/reversible_add_column_with_default_spec.rb create mode 100644 spec/rubocop/cop/migration/with_lock_retries_disallowed_method_spec.rb delete mode 100644 spec/rubocop/cop/migration/with_lock_retries_without_ddl_transaction_spec.rb create mode 100644 spec/rubocop/cop/performance/ar_exists_and_present_blank_spec.rb create mode 100644 spec/rubocop/cop/rspec/empty_line_after_shared_example_spec.rb (limited to 'spec/rubocop') diff --git a/spec/rubocop/cop/avoid_keyword_arguments_in_sidekiq_workers_spec.rb b/spec/rubocop/cop/avoid_keyword_arguments_in_sidekiq_workers_spec.rb new file mode 100644 index 00000000000..11d63d8e0ee --- /dev/null +++ b/spec/rubocop/cop/avoid_keyword_arguments_in_sidekiq_workers_spec.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'rubocop' +require 'rubocop/rspec/support' +require_relative '../../../rubocop/cop/avoid_keyword_arguments_in_sidekiq_workers' + +describe RuboCop::Cop::AvoidKeywordArgumentsInSidekiqWorkers do + include CopHelper + + subject(:cop) { described_class.new } + + it 'flags violation for keyword arguments usage in perform method signature' do + expect_offense(<<~RUBY) + def perform(id:) + ^^^^^^^^^^^^^^^^ Do not use keyword arguments in Sidekiq workers. For details, check https://github.com/mperham/sidekiq/issues/2372 + end + RUBY + end + + it 'flags violation for optional keyword arguments usage in perform method signature' do + expect_offense(<<~RUBY) + def perform(id: nil) + ^^^^^^^^^^^^^^^^^^^^ Do not use keyword arguments in Sidekiq workers. For details, check https://github.com/mperham/sidekiq/issues/2372 + end + RUBY + end + + it 'does not flag a violation for standard optional arguments usage in perform method signature' do + expect_no_offenses(<<~RUBY) + def perform(id = nil) + end + RUBY + end + + it 'does not flag a violation for keyword arguments usage in non-perform method signatures' do + expect_no_offenses(<<~RUBY) + def helper(id:) + end + RUBY + end + + it 'does not flag a violation for optional keyword arguments usage in non-perform method signatures' do + expect_no_offenses(<<~RUBY) + def helper(id: nil) + end + RUBY + end +end diff --git a/spec/rubocop/cop/gitlab/change_timezone_spec.rb b/spec/rubocop/cop/gitlab/change_timezone_spec.rb new file mode 100644 index 00000000000..af76559a9fa --- /dev/null +++ b/spec/rubocop/cop/gitlab/change_timezone_spec.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'rubocop' +require 'rubocop/rspec/support' +require_relative '../../../../rubocop/cop/gitlab/change_timzone' + +describe RuboCop::Cop::Gitlab::ChangeTimezone do + include CopHelper + + subject(:cop) { described_class.new } + + context 'Time.zone=' do + it 'registers an offense with no 2nd argument' do + expect_offense(<<~PATTERN.strip_indent) + Time.zone = 'Awkland' + ^^^^^^^^^^^^^^^^^^^^^ Do not change timezone in the runtime (application or rspec), it could result in silently modifying other behavior. + PATTERN + end + end +end diff --git a/spec/rubocop/cop/gitlab/json_spec.rb b/spec/rubocop/cop/gitlab/json_spec.rb new file mode 100644 index 00000000000..d64f60c8583 --- /dev/null +++ b/spec/rubocop/cop/gitlab/json_spec.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'rubocop' +require 'rubocop/rspec/support' +require_relative '../../../../rubocop/cop/gitlab/json' + +describe RuboCop::Cop::Gitlab::Json do + include CopHelper + + subject(:cop) { described_class.new } + + shared_examples('registering call offense') do |options| + let(:offending_lines) { options[:offending_lines] } + + it 'registers an offense when the class calls JSON' do + inspect_source(source) + + aggregate_failures do + expect(cop.offenses.size).to eq(offending_lines.size) + expect(cop.offenses.map(&:line)).to eq(offending_lines) + end + end + end + + context 'when JSON is called' do + it_behaves_like 'registering call offense', offending_lines: [3] do + let(:source) do + <<~RUBY + class Foo + def bar + JSON.parse('{ "foo": "bar" }') + end + end + RUBY + end + end + end +end diff --git a/spec/rubocop/cop/inject_enterprise_edition_module_spec.rb b/spec/rubocop/cop/inject_enterprise_edition_module_spec.rb index ce20d494542..3cb1dbbbc2c 100644 --- a/spec/rubocop/cop/inject_enterprise_edition_module_spec.rb +++ b/spec/rubocop/cop/inject_enterprise_edition_module_spec.rb @@ -159,6 +159,17 @@ describe RuboCop::Cop::InjectEnterpriseEditionModule do SOURCE end + it 'does not flag the double use of `X_if_ee` on the last line' do + expect_no_offenses(<<~SOURCE) + class Foo + end + + Foo.extend_if_ee('EE::Foo') + Foo.include_if_ee('EE::Foo') + Foo.prepend_if_ee('EE::Foo') + SOURCE + end + it 'autocorrects offenses by just disabling the Cop' do source = <<~SOURCE class Foo diff --git a/spec/rubocop/cop/migration/add_column_with_default_spec.rb b/spec/rubocop/cop/migration/add_column_with_default_spec.rb index a8cf965a3ef..5d4fc59fb95 100644 --- a/spec/rubocop/cop/migration/add_column_with_default_spec.rb +++ b/spec/rubocop/cop/migration/add_column_with_default_spec.rb @@ -27,44 +27,15 @@ describe RuboCop::Cop::Migration::AddColumnWithDefault do allow(cop).to receive(:in_migration?).and_return(true) end - let(:offense) { '`add_column_with_default` without `allow_null: true` may cause prolonged lock situations and downtime, see https://gitlab.com/gitlab-org/gitlab/issues/38060' } + let(:offense) { '`add_column_with_default` is deprecated, use `add_column` instead' } - context 'for blacklisted table' do - it 'registers an offense when specifying allow_null: false' do - expect_offense(<<~RUBY) - def up - add_column_with_default(:merge_request_diff_files, :artifacts, :boolean, default: true, allow_null: false) - ^^^^^^^^^^^^^^^^^^^^^^^ #{offense} - end - RUBY - end - - it 'registers no offense when specifying allow_null: true' do - expect_no_offenses(<<~RUBY) - def up - add_column_with_default(:merge_request_diff_files, :artifacts, :boolean, default: true, allow_null: true) - end - RUBY - end - - it 'registers an offense when allow_null is not specified' do - expect_offense(<<~RUBY) - def up - add_column_with_default(:merge_request_diff_files, :artifacts, :boolean, default: true) - ^^^^^^^^^^^^^^^^^^^^^^^ #{offense} - end - RUBY - end - end - - context 'for tables not on the blacklist' do - it 'registers no offense for application_settings (not on blacklist)' do - expect_no_offenses(<<~RUBY) - def up - add_column_with_default(:application_settings, :another_column, :boolean, default: true, allow_null: false) - end - RUBY - end + it 'registers an offense ' do + expect_offense(<<~RUBY) + def up + add_column_with_default(:merge_request_diff_files, :artifacts, :boolean, default: true, allow_null: false) + ^^^^^^^^^^^^^^^^^^^^^^^ #{offense} + end + RUBY end end end diff --git a/spec/rubocop/cop/migration/add_columns_to_wide_tables_spec.rb b/spec/rubocop/cop/migration/add_columns_to_wide_tables_spec.rb index f0c64740e63..5b179168eab 100644 --- a/spec/rubocop/cop/migration/add_columns_to_wide_tables_spec.rb +++ b/spec/rubocop/cop/migration/add_columns_to_wide_tables_spec.rb @@ -42,8 +42,8 @@ describe RuboCop::Cop::Migration::AddColumnsToWideTables do expect_offense(<<~RUBY) def up - add_column_with_default(:users, :another_column, :boolean, default: false) - ^^^^^^^^^^^^^^^^^^^^^^^ #{offense} + add_column(:users, :another_column, :boolean, default: false) + ^^^^^^^^^^ #{offense} end RUBY end diff --git a/spec/rubocop/cop/migration/add_concurrent_foreign_key_spec.rb b/spec/rubocop/cop/migration/add_concurrent_foreign_key_spec.rb index 419d74c298a..dfc3898af24 100644 --- a/spec/rubocop/cop/migration/add_concurrent_foreign_key_spec.rb +++ b/spec/rubocop/cop/migration/add_concurrent_foreign_key_spec.rb @@ -33,5 +33,11 @@ describe RuboCop::Cop::Migration::AddConcurrentForeignKey do expect(cop.offenses.map(&:line)).to eq([1]) end end + + it 'does not register an offense when a `NOT VALID` foreign key is added' do + inspect_source('def up; add_foreign_key(:projects, :users, column: :user_id, validate: false); end') + + expect(cop.offenses).to be_empty + end end end diff --git a/spec/rubocop/cop/migration/add_limit_to_string_columns_spec.rb b/spec/rubocop/cop/migration/add_limit_to_string_columns_spec.rb deleted file mode 100644 index 97a3ae8f2bc..00000000000 --- a/spec/rubocop/cop/migration/add_limit_to_string_columns_spec.rb +++ /dev/null @@ -1,268 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' -require 'rubocop' -require 'rubocop/rspec/support' - -require_relative '../../../../rubocop/cop/migration/add_limit_to_string_columns' - -describe RuboCop::Cop::Migration::AddLimitToStringColumns do - include CopHelper - - subject(:cop) { described_class.new } - - context 'in migration' do - before do - allow(cop).to receive(:in_migration?).and_return(true) - - inspect_source(migration) - end - - context 'when creating a table' do - context 'with string columns and limit' do - let(:migration) do - %q( - class CreateUsers < ActiveRecord::Migration[5.2] - DOWNTIME = false - - def change - create_table :users do |t| - t.string :username, null: false, limit: 255 - t.timestamps_with_timezone null: true - end - end - end - ) - end - - it 'register no offense' do - expect(cop.offenses.size).to eq(0) - end - - context 'with limit in a different position' do - let(:migration) do - %q( - class CreateUsers < ActiveRecord::Migration[5.2] - DOWNTIME = false - - def change - create_table :users do |t| - t.string :username, limit: 255, null: false - t.timestamps_with_timezone null: true - end - end - end - ) - end - - it 'registers an offense' do - expect(cop.offenses.size).to eq(0) - end - end - end - - context 'with string columns and no limit' do - let(:migration) do - %q( - class CreateUsers < ActiveRecord::Migration[5.2] - DOWNTIME = false - - def change - create_table :users do |t| - t.string :username, null: false - t.timestamps_with_timezone null: true - end - end - end - ) - end - - it 'registers an offense' do - expect(cop.offenses.size).to eq(1) - expect(cop.offenses.first.message) - .to eq('String columns should have a limit constraint. 255 is suggested') - end - end - - context 'with no string columns' do - let(:migration) do - %q( - class CreateMilestoneReleases < ActiveRecord::Migration[5.2] - DOWNTIME = false - - def change - create_table :milestone_releases do |t| - t.integer :milestone_id - t.integer :release_id - end - end - end - ) - end - - it 'register no offense' do - expect(cop.offenses.size).to eq(0) - end - end - end - - context 'when adding columns' do - context 'with string columns with limit' do - let(:migration) do - %q( - class AddEmailToUsers < ActiveRecord::Migration[5.2] - DOWNTIME = false - - def change - add_column :users, :email, :string, limit: 255 - end - end - ) - end - - it 'registers no offense' do - expect(cop.offenses.size).to eq(0) - end - - context 'with limit in a different position' do - let(:migration) do - %q( - class AddEmailToUsers < ActiveRecord::Migration[5.2] - DOWNTIME = false - - def change - add_column :users, :email, :string, limit: 255, default: 'example@email.com' - end - end - ) - end - - it 'registers no offense' do - expect(cop.offenses.size).to eq(0) - end - end - end - - context 'with string columns with no limit' do - let(:migration) do - %q( - class AddEmailToUsers < ActiveRecord::Migration[5.2] - DOWNTIME = false - - def change - add_column :users, :email, :string - end - end - ) - end - - it 'registers offense' do - expect(cop.offenses.size).to eq(1) - expect(cop.offenses.first.message) - .to eq('String columns should have a limit constraint. 255 is suggested') - end - end - - context 'with no string columns' do - let(:migration) do - %q( - class AddEmailToUsers < ActiveRecord::Migration[5.2] - DOWNTIME = false - - def change - add_column :users, :active, :boolean, default: false - end - end - ) - end - - it 'registers no offense' do - expect(cop.offenses.size).to eq(0) - end - end - end - - context 'with add_column_with_default' do - context 'with a limit' do - let(:migration) do - %q( - class AddRuleTypeToApprovalMergeRequestRules < ActiveRecord::Migration[5.2] - DOWNTIME = false - - def change - add_column_with_default(:approval_merge_request_rules, :rule_type, :string, limit: 2, default: 1) - end - end - ) - end - - it 'registers no offense' do - expect(cop.offenses.size).to eq(0) - end - end - - context 'without a limit' do - let(:migration) do - %q( - class AddRuleTypeToApprovalMergeRequestRules < ActiveRecord::Migration[5.2] - DOWNTIME = false - - def change - add_column_with_default(:approval_merge_request_rules, :rule_type, :string, default: 1) - end - end - ) - end - - it 'registers an offense' do - expect(cop.offenses.size).to eq(1) - end - end - end - - context 'with methods' do - let(:migration) do - %q( - class AddEmailToUsers < ActiveRecord::Migration[5.2] - DOWNTIME = false - - def change - add_column_if_table_not_exists :users, :first_name, :string, limit: 255 - search_namespace(user_name) - end - - def add_column_if_not_exists(table, name, *args) - add_column(table, name, *args) unless column_exists?(table, name) - end - - def search_namespace(username) - Uniquify.new.string(username) do |str| - query = "SELECT id FROM namespaces WHERE parent_id IS NULL AND path='#{str}' LIMIT 1" - connection.exec_query(query) - end - end - end - ) - end - - it 'registers no offense' do - expect(cop.offenses.size).to eq(0) - end - end - end - - context 'outside of migrations' do - let(:active_record_model) do - %q( - class User < ApplicationRecord - end - ) - end - - it 'registers no offense' do - inspect_source(active_record_model) - - expect(cop.offenses.size).to eq(0) - end - end -end diff --git a/spec/rubocop/cop/migration/add_limit_to_text_columns_spec.rb b/spec/rubocop/cop/migration/add_limit_to_text_columns_spec.rb new file mode 100644 index 00000000000..514260a4306 --- /dev/null +++ b/spec/rubocop/cop/migration/add_limit_to_text_columns_spec.rb @@ -0,0 +1,160 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'rubocop' +require 'rubocop/rspec/support' + +require_relative '../../../../rubocop/cop/migration/add_limit_to_text_columns' + +describe RuboCop::Cop::Migration::AddLimitToTextColumns do + include CopHelper + + subject(:cop) { described_class.new } + + context 'in migration' do + before do + allow(cop).to receive(:in_migration?).and_return(true) + end + + context 'when text columns are defined without a limit' do + it 'registers an offense' do + expect_offense(<<~RUBY) + class TestTextLimits < ActiveRecord::Migration[6.0] + DOWNTIME = false + disable_ddl_transaction! + + def up + create_table :test_text_limits, id: false do |t| + t.integer :test_id, null: false + t.text :name + ^^^^ #{described_class::MSG} + end + + add_column :test_text_limits, :email, :text + ^^^^^^^^^^ #{described_class::MSG} + + add_column_with_default :test_text_limits, :role, :text, default: 'default' + ^^^^^^^^^^^^^^^^^^^^^^^ #{described_class::MSG} + + change_column_type_concurrently :test_text_limits, :test_id, :text + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{described_class::MSG} + end + end + RUBY + + expect(cop.offenses.map(&:cop_name)).to all(eq('Migration/AddLimitToTextColumns')) + end + end + + context 'when text columns are defined with a limit' do + it 'registers no offense' do + expect_no_offenses(<<~RUBY) + class TestTextLimits < ActiveRecord::Migration[6.0] + DOWNTIME = false + disable_ddl_transaction! + + def up + create_table :test_text_limits, id: false do |t| + t.integer :test_id, null: false + t.text :name + end + + add_column :test_text_limits, :email, :text + add_column_with_default :test_text_limits, :role, :text, default: 'default' + change_column_type_concurrently :test_text_limits, :test_id, :text + + add_text_limit :test_text_limits, :name, 255 + add_text_limit :test_text_limits, :email, 255 + add_text_limit :test_text_limits, :role, 255 + add_text_limit :test_text_limits, :test_id, 255 + end + end + RUBY + end + end + + # Make sure that the cop is properly checking for an `add_text_limit` + # over the same {table, attribute} as the one that triggered the offence + context 'when the limit is defined for a same name attribute but different table' do + it 'registers an offense' do + expect_offense(<<~RUBY) + class TestTextLimits < ActiveRecord::Migration[6.0] + DOWNTIME = false + disable_ddl_transaction! + + def up + create_table :test_text_limits, id: false do |t| + t.integer :test_id, null: false + t.text :name + ^^^^ #{described_class::MSG} + end + + add_column :test_text_limits, :email, :text + ^^^^^^^^^^ #{described_class::MSG} + + add_column_with_default :test_text_limits, :role, :text, default: 'default' + ^^^^^^^^^^^^^^^^^^^^^^^ #{described_class::MSG} + + change_column_type_concurrently :test_text_limits, :test_id, :text + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{described_class::MSG} + + add_text_limit :wrong_table, :name, 255 + add_text_limit :wrong_table, :email, 255 + add_text_limit :wrong_table, :role, 255 + add_text_limit :wrong_table, :test_id, 255 + end + end + RUBY + + expect(cop.offenses.map(&:cop_name)).to all(eq('Migration/AddLimitToTextColumns')) + end + end + + context 'on down' do + it 'registers no offense' do + expect_no_offenses(<<~RUBY) + class TestTextLimits < ActiveRecord::Migration[6.0] + DOWNTIME = false + + def up + drop_table :no_offence_on_down + end + + def down + create_table :no_offence_on_down, id: false do |t| + t.integer :test_id, null: false + t.text :name + end + + add_column :no_offence_on_down, :email, :text + + add_column_with_default :no_offence_on_down, :role, :text, default: 'default' + end + end + RUBY + end + end + end + + context 'outside of migration' do + it 'registers no offense' do + expect_no_offenses(<<~RUBY) + class TestTextLimits < ActiveRecord::Migration[6.0] + DOWNTIME = false + disable_ddl_transaction! + + def up + create_table :test_text_limits, id: false do |t| + t.integer :test_id, null: false + t.text :name + end + + add_column :test_text_limits, :email, :text + add_column_with_default :test_text_limits, :role, :text, default: 'default' + change_column_type_concurrently :test_text_limits, :test_id, :text + end + end + RUBY + end + end +end diff --git a/spec/rubocop/cop/migration/prevent_strings_spec.rb b/spec/rubocop/cop/migration/prevent_strings_spec.rb new file mode 100644 index 00000000000..2702ce1c090 --- /dev/null +++ b/spec/rubocop/cop/migration/prevent_strings_spec.rb @@ -0,0 +1,143 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'rubocop' +require 'rubocop/rspec/support' + +require_relative '../../../../rubocop/cop/migration/prevent_strings' + +describe RuboCop::Cop::Migration::PreventStrings do + include CopHelper + + subject(:cop) { described_class.new } + + context 'in migration' do + before do + allow(cop).to receive(:in_migration?).and_return(true) + end + + context 'when the string data type is used' do + it 'registers an offense' do + expect_offense(<<~RUBY) + class Users < ActiveRecord::Migration[6.0] + DOWNTIME = false + + def up + create_table :users do |t| + t.string :username, null: false + ^^^^^^ #{described_class::MSG} + + t.timestamps_with_timezone null: true + + t.string :password + ^^^^^^ #{described_class::MSG} + end + + add_column(:users, :bio, :string) + ^^^^^^^^^^ #{described_class::MSG} + + add_column_with_default(:users, :url, :string, default: '/-/user', allow_null: false, limit: 255) + ^^^^^^^^^^^^^^^^^^^^^^^ #{described_class::MSG} + + change_column_type_concurrently :users, :commit_id, :string + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{described_class::MSG} + end + end + RUBY + + expect(cop.offenses.map(&:cop_name)).to all(eq('Migration/PreventStrings')) + end + end + + context 'when the string data type is not used' do + it 'registers no offense' do + expect_no_offenses(<<~RUBY) + class Users < ActiveRecord::Migration[6.0] + DOWNTIME = false + + def up + create_table :users do |t| + t.integer :not_a_string, null: false + t.timestamps_with_timezone null: true + end + + add_column(:users, :not_a_string, :integer) + end + end + RUBY + end + end + + context 'when the text data type is used' do + it 'registers no offense' do + expect_no_offenses(<<~RUBY) + class Users < ActiveRecord::Migration[6.0] + DOWNTIME = false + + def up + create_table :users do |t| + t.text :username, null: false + t.timestamps_with_timezone null: true + t.text :password + end + + add_column(:users, :bio, :text) + add_column_with_default(:users, :url, :text, default: '/-/user', allow_null: false, limit: 255) + change_column_type_concurrently :users, :commit_id, :text + end + end + RUBY + end + end + + context 'on down' do + it 'registers no offense' do + expect_no_offenses(<<~RUBY) + class Users < ActiveRecord::Migration[6.0] + DOWNTIME = false + + def up + remove_column :users, :bio + remove_column :users, :url + + drop_table :test_strings + end + + def down + create_table :test_strings, id: false do |t| + t.integer :test_id, null: false + t.string :name + end + + add_column(:users, :bio, :string) + add_column_with_default(:users, :url, :string, default: '/-/user', allow_null: false, limit: 255) + change_column_type_concurrently :users, :commit_id, :string + end + end + RUBY + end + end + end + + context 'outside of migration' do + it 'registers no offense' do + expect_no_offenses(<<~RUBY) + class Users < ActiveRecord::Migration[6.0] + DOWNTIME = false + + def up + create_table :users do |t| + t.string :username, null: false + t.timestamps_with_timezone null: true + t.string :password + end + + add_column(:users, :bio, :string) + add_column_with_default(:users, :url, :string, default: '/-/user', allow_null: false, limit: 255) + change_column_type_concurrently :users, :commit_id, :string + end + end + RUBY + end + end +end diff --git a/spec/rubocop/cop/migration/reversible_add_column_with_default_spec.rb b/spec/rubocop/cop/migration/reversible_add_column_with_default_spec.rb deleted file mode 100644 index b3c5b855004..00000000000 --- a/spec/rubocop/cop/migration/reversible_add_column_with_default_spec.rb +++ /dev/null @@ -1,43 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -require 'rubocop' -require 'rubocop/rspec/support' - -require_relative '../../../../rubocop/cop/migration/reversible_add_column_with_default' - -describe RuboCop::Cop::Migration::ReversibleAddColumnWithDefault do - include CopHelper - - subject(:cop) { described_class.new } - - context 'in migration' do - before do - allow(cop).to receive(:in_migration?).and_return(true) - end - - it 'registers an offense when add_column_with_default is used inside a change method' do - inspect_source('def change; add_column_with_default :table, :column, default: false; end') - - aggregate_failures do - expect(cop.offenses.size).to eq(1) - expect(cop.offenses.map(&:line)).to eq([1]) - end - end - - it 'registers no offense when add_column_with_default is used inside an up method' do - inspect_source('def up; add_column_with_default :table, :column, default: false; end') - - expect(cop.offenses.size).to eq(0) - end - end - - context 'outside of migration' do - it 'registers no offense' do - inspect_source('def change; add_column_with_default :table, :column, default: false; end') - - expect(cop.offenses.size).to eq(0) - end - end -end diff --git a/spec/rubocop/cop/migration/with_lock_retries_disallowed_method_spec.rb b/spec/rubocop/cop/migration/with_lock_retries_disallowed_method_spec.rb new file mode 100644 index 00000000000..48570c1c8d8 --- /dev/null +++ b/spec/rubocop/cop/migration/with_lock_retries_disallowed_method_spec.rb @@ -0,0 +1,68 @@ +# frozen_string_literal: true + +require 'spec_helper' + +require 'rubocop' +require 'rubocop/rspec/support' + +require_relative '../../../../rubocop/cop/migration/with_lock_retries_disallowed_method' + +describe RuboCop::Cop::Migration::WithLockRetriesDisallowedMethod do + include CopHelper + + subject(:cop) { described_class.new } + + context 'in migration' do + before do + allow(cop).to receive(:in_migration?).and_return(true) + end + + it 'registers an offense when `with_lock_retries` block has disallowed method' do + inspect_source('def change; with_lock_retries { disallowed_method }; end') + + aggregate_failures do + expect(cop.offenses.size).to eq(1) + expect(cop.offenses.map(&:line)).to eq([1]) + end + end + + it 'registers an offense when `with_lock_retries` block has disallowed methods' do + source = <<~HEREDOC + def change + with_lock_retries do + disallowed_method + + create_table do |t| + t.text :text + end + + other_disallowed_method + + add_column :users, :name + end + end + HEREDOC + + inspect_source(source) + + aggregate_failures do + expect(cop.offenses.size).to eq(2) + expect(cop.offenses.map(&:line)).to eq([3, 9]) + end + end + + it 'registers no offense when `with_lock_retries` has only allowed method' do + inspect_source('def up; with_lock_retries { add_foreign_key :foo, :bar }; end') + + expect(cop.offenses.size).to eq(0) + end + end + + context 'outside of migration' do + it 'registers no offense' do + inspect_source('def change; with_lock_retries { disallowed_method }; end') + + expect(cop.offenses.size).to eq(0) + end + end +end diff --git a/spec/rubocop/cop/migration/with_lock_retries_without_ddl_transaction_spec.rb b/spec/rubocop/cop/migration/with_lock_retries_without_ddl_transaction_spec.rb deleted file mode 100644 index b42a4a14c67..00000000000 --- a/spec/rubocop/cop/migration/with_lock_retries_without_ddl_transaction_spec.rb +++ /dev/null @@ -1,46 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -require 'rubocop' -require 'rubocop/rspec/support' - -require_relative '../../../../rubocop/cop/migration/with_lock_retries_without_ddl_transaction' - -describe RuboCop::Cop::Migration::WithLockRetriesWithoutDdlTransaction do - include CopHelper - - let(:valid_source) { 'class MigrationClass < ActiveRecord::Migration[6.0]; def up; with_lock_retries {}; end; end' } - let(:invalid_source) { 'class MigrationClass < ActiveRecord::Migration[6.0]; disable_ddl_transaction!; def up; with_lock_retries {}; end; end' } - - subject(:cop) { described_class.new } - - context 'in migration' do - before do - allow(cop).to receive(:in_migration?).and_return(true) - end - - it 'registers an offense when `with_lock_retries` is used with `disable_ddl_transaction!` method' do - inspect_source(invalid_source) - - aggregate_failures do - expect(cop.offenses.size).to eq(1) - expect(cop.offenses.map(&:line)).to eq([1]) - end - end - - it 'registers no offense when `with_lock_retries` is used inside an `up` method' do - inspect_source(valid_source) - - expect(cop.offenses.size).to eq(0) - end - end - - context 'outside of migration' do - it 'registers no offense' do - inspect_source(invalid_source) - - expect(cop.offenses.size).to eq(0) - end - end -end diff --git a/spec/rubocop/cop/performance/ar_exists_and_present_blank_spec.rb b/spec/rubocop/cop/performance/ar_exists_and_present_blank_spec.rb new file mode 100644 index 00000000000..ce4fdac56b0 --- /dev/null +++ b/spec/rubocop/cop/performance/ar_exists_and_present_blank_spec.rb @@ -0,0 +1,111 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' +require_relative '../../../support/helpers/expect_offense' +require_relative '../../../../rubocop/cop/performance/ar_exists_and_present_blank.rb' + +describe RuboCop::Cop::Performance::ARExistsAndPresentBlank do + include CopHelper + include ExpectOffense + + subject(:cop) { described_class.new } + + context 'when it is not haml file' do + it 'does not flag it as an offense' do + expect(subject).to receive(:in_haml_file?).with(anything).at_least(:once).and_return(false) + + expect_no_offenses <<~SOURCE + return unless @users.exists? + show @users if @users.present? + SOURCE + end + end + + context 'when it is haml file' do + before do + expect(subject).to receive(:in_haml_file?).with(anything).at_least(:once).and_return(true) + end + + context 'the same object uses exists? and present?' do + it 'flags it as an offense' do + expect_offense <<~SOURCE + return unless @users.exists? + show @users if @users.present? + ^^^^^^^^^^^^^^^ Avoid `@users.present?`, because it will generate database query 'Select TABLE.*' which is expensive. Suggest to use `@users.any?` to replace `@users.present?` + SOURCE + + expect(cop.offenses.map(&:cop_name)).to contain_exactly('Performance/ARExistsAndPresentBlank') + end + end + + context 'the same object uses exists? and blank?' do + it 'flags it as an offense' do + expect_offense <<~SOURCE + return unless @users.exists? + show @users if @users.blank? + ^^^^^^^^^^^^^ Avoid `@users.blank?`, because it will generate database query 'Select TABLE.*' which is expensive. Suggest to use `@users.empty?` to replace `@users.blank?` + SOURCE + + expect(cop.offenses.map(&:cop_name)).to contain_exactly('Performance/ARExistsAndPresentBlank') + end + end + + context 'the same object uses exists?, blank? and present?' do + it 'flags it as an offense' do + expect_offense <<~SOURCE + return unless @users.exists? + show @users if @users.blank? + ^^^^^^^^^^^^^ Avoid `@users.blank?`, because it will generate database query 'Select TABLE.*' which is expensive. Suggest to use `@users.empty?` to replace `@users.blank?` + show @users if @users.present? + ^^^^^^^^^^^^^^^ Avoid `@users.present?`, because it will generate database query 'Select TABLE.*' which is expensive. Suggest to use `@users.any?` to replace `@users.present?` + SOURCE + + expect(cop.offenses.map(&:cop_name)).to contain_exactly('Performance/ARExistsAndPresentBlank', 'Performance/ARExistsAndPresentBlank') + end + end + + RSpec.shared_examples 'different object uses exists? and present?/blank?' do |another_method| + it 'does not flag it as an offense' do + expect_no_offenses <<~SOURCE + return unless @users.exists? + present @emails if @emails.#{another_method} + SOURCE + end + end + + it_behaves_like 'different object uses exists? and present?/blank?', 'present?' + it_behaves_like 'different object uses exists? and present?/blank?', 'blank?' + + RSpec.shared_examples 'Only using one present?/blank? without exists?' do |non_exists_method| + it 'does not flag it as an offense' do + expect_no_offenses "@users.#{non_exists_method}" + end + end + + it_behaves_like 'Only using one present?/blank? without exists?', 'present?' + it_behaves_like 'Only using one present?/blank? without exists?', 'blank?' + + context 'when using many present?/empty? without exists?' do + it 'does not flag it as an offense' do + expect_no_offenses <<~SOURCE + @user.present? + @user.blank? + @user.present? + @user.blank? + SOURCE + end + end + + context 'when just using exists? without present?/blank?' do + it 'does not flag it as an offense' do + expect_no_offenses '@users.exists?' + + expect_no_offenses <<~SOURCE + @users.exists? + @users.some_other_method? + @users.exists? + SOURCE + end + end + end +end diff --git a/spec/rubocop/cop/rspec/empty_line_after_shared_example_spec.rb b/spec/rubocop/cop/rspec/empty_line_after_shared_example_spec.rb new file mode 100644 index 00000000000..cee593fe535 --- /dev/null +++ b/spec/rubocop/cop/rspec/empty_line_after_shared_example_spec.rb @@ -0,0 +1,86 @@ +# frozen_string_literal: true + +require 'spec_helper' + +require_relative '../../../../rubocop/cop/rspec/empty_line_after_shared_example' + +describe RuboCop::Cop::RSpec::EmptyLineAfterSharedExample do + subject(:cop) { described_class.new } + + it 'flags a missing empty line after `it_behaves_like` block' do + expect_offense(<<-RUBY) + RSpec.describe Foo do + it_behaves_like 'does this' do + end + ^^^ Add an empty line after `it_behaves_like` block. + it_behaves_like 'does that' do + end + end + RUBY + + expect_correction(<<-RUBY) + RSpec.describe Foo do + it_behaves_like 'does this' do + end + + it_behaves_like 'does that' do + end + end + RUBY + end + + it 'ignores one-line shared examples before shared example blocks' do + expect_no_offenses(<<-RUBY) + RSpec.describe Foo do + it_behaves_like 'does this' + it_behaves_like 'does that' do + end + end + RUBY + end + + it 'flags a missing empty line after `shared_examples`' do + expect_offense(<<-RUBY) + RSpec.context 'foo' do + shared_examples do + end + ^^^ Add an empty line after `shared_examples` block. + shared_examples 'something gets done' do + end + end + RUBY + + expect_correction(<<-RUBY) + RSpec.context 'foo' do + shared_examples do + end + + shared_examples 'something gets done' do + end + end + RUBY + end + + it 'ignores consecutive one-liners' do + expect_no_offenses(<<-RUBY) + RSpec.describe Foo do + it_behaves_like 'do this' + it_behaves_like 'do that' + end + RUBY + end + + it 'flags mixed one-line and multi-line shared examples' do + expect_offense(<<-RUBY) + RSpec.context 'foo' do + it_behaves_like 'do this' + it_behaves_like 'do that' + it_behaves_like 'does this' do + end + ^^^ Add an empty line after `it_behaves_like` block. + it_behaves_like 'do this' + it_behaves_like 'do that' + end + RUBY + end +end diff --git a/spec/rubocop/cop/rspec/env_assignment_spec.rb b/spec/rubocop/cop/rspec/env_assignment_spec.rb index 2a2bd1434d6..1c7cfb5c827 100644 --- a/spec/rubocop/cop/rspec/env_assignment_spec.rb +++ b/spec/rubocop/cop/rspec/env_assignment_spec.rb @@ -10,8 +10,8 @@ require_relative '../../../../rubocop/cop/rspec/env_assignment' describe RuboCop::Cop::RSpec::EnvAssignment do include CopHelper - OFFENSE_CALL_SINGLE_QUOTES_KEY = %(ENV['FOO'] = 'bar').freeze - OFFENSE_CALL_DOUBLE_QUOTES_KEY = %(ENV["FOO"] = 'bar').freeze + offense_call_single_quotes_key = %(ENV['FOO'] = 'bar').freeze + offense_call_double_quotes_key = %(ENV["FOO"] = 'bar').freeze let(:source_file) { 'spec/foo_spec.rb' } @@ -36,12 +36,12 @@ describe RuboCop::Cop::RSpec::EnvAssignment do end context 'with a key using single quotes' do - it_behaves_like 'an offensive ENV#[]= call', OFFENSE_CALL_SINGLE_QUOTES_KEY - it_behaves_like 'an autocorrected ENV#[]= call', OFFENSE_CALL_SINGLE_QUOTES_KEY, %(stub_env('FOO', 'bar')) + it_behaves_like 'an offensive ENV#[]= call', offense_call_single_quotes_key + it_behaves_like 'an autocorrected ENV#[]= call', offense_call_single_quotes_key, %(stub_env('FOO', 'bar')) end context 'with a key using double quotes' do - it_behaves_like 'an offensive ENV#[]= call', OFFENSE_CALL_DOUBLE_QUOTES_KEY - it_behaves_like 'an autocorrected ENV#[]= call', OFFENSE_CALL_DOUBLE_QUOTES_KEY, %(stub_env("FOO", 'bar')) + it_behaves_like 'an offensive ENV#[]= call', offense_call_double_quotes_key + it_behaves_like 'an autocorrected ENV#[]= call', offense_call_double_quotes_key, %(stub_env("FOO", 'bar')) end end -- cgit v1.2.3