diff options
Diffstat (limited to 'spec/rubocop/cop')
| -rw-r--r-- | spec/rubocop/cop/gitlab/mark_used_feature_flags_spec.rb | 1 | ||||
| -rw-r--r-- | spec/rubocop/cop/migration/add_limit_to_text_columns_spec.rb | 36 | ||||
| -rw-r--r-- | spec/rubocop/cop/migration/prevent_index_creation_spec.rb | 86 | ||||
| -rw-r--r-- | spec/rubocop/cop/migration/versioned_migration_class_spec.rb | 81 | ||||
| -rw-r--r-- | spec/rubocop/cop/performance/active_record_subtransaction_methods_spec.rb | 29 | ||||
| -rw-r--r-- | spec/rubocop/cop/performance/active_record_subtransactions_spec.rb | 62 | ||||
| -rw-r--r-- | spec/rubocop/cop/sidekiq_load_balancing/worker_data_consistency_spec.rb (renamed from spec/rubocop/cop/worker_data_consistency_spec.rb) | 4 | ||||
| -rw-r--r-- | spec/rubocop/cop/sidekiq_load_balancing/worker_data_consistency_with_deduplication_spec.rb | 166 |
8 files changed, 444 insertions, 21 deletions
diff --git a/spec/rubocop/cop/gitlab/mark_used_feature_flags_spec.rb b/spec/rubocop/cop/gitlab/mark_used_feature_flags_spec.rb index 35b21477d80..6b5b07fb357 100644 --- a/spec/rubocop/cop/gitlab/mark_used_feature_flags_spec.rb +++ b/spec/rubocop/cop/gitlab/mark_used_feature_flags_spec.rb @@ -16,6 +16,7 @@ RSpec.describe RuboCop::Cop::Gitlab::MarkUsedFeatureFlags do stub_const("#{described_class}::DYNAMIC_FEATURE_FLAGS", []) allow(cop).to receive(:defined_feature_flags).and_return(defined_feature_flags) allow(cop).to receive(:usage_data_counters_known_event_feature_flags).and_return([]) + described_class.feature_flags_already_tracked = false end def feature_flag_path(feature_flag_name) 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 index 899872859a9..f6bed0d74fb 100644 --- a/spec/rubocop/cop/migration/add_limit_to_text_columns_spec.rb +++ b/spec/rubocop/cop/migration/add_limit_to_text_columns_spec.rb @@ -11,6 +11,7 @@ RSpec.describe RuboCop::Cop::Migration::AddLimitToTextColumns do before do allow(cop).to receive(:in_migration?).and_return(true) + allow(cop).to receive(:version).and_return(described_class::TEXT_LIMIT_ATTRIBUTE_ALLOWED_SINCE + 5) end context 'when text columns are defined without a limit' do @@ -26,7 +27,7 @@ RSpec.describe RuboCop::Cop::Migration::AddLimitToTextColumns do ^^^^ #{msg} end - create_table_with_constraints :test_text_limits_create do |t| + create_table :test_text_limits_create do |t| t.integer :test_id, null: false t.text :title t.text :description @@ -61,13 +62,10 @@ RSpec.describe RuboCop::Cop::Migration::AddLimitToTextColumns do t.text :name end - create_table_with_constraints :test_text_limits_create do |t| + create_table :test_text_limits_create do |t| t.integer :test_id, null: false - t.text :title - t.text :description - - t.text_limit :title, 100 - t.text_limit :description, 255 + t.text :title, limit: 100 + t.text :description, limit: 255 end add_column :test_text_limits, :email, :text @@ -82,6 +80,30 @@ RSpec.describe RuboCop::Cop::Migration::AddLimitToTextColumns do end RUBY end + + context 'for migrations before 2021_09_10_00_00_00' do + it 'when limit: attribute is used (which is not supported yet for this version): registers an offense' do + allow(cop).to receive(:version).and_return(described_class::TEXT_LIMIT_ATTRIBUTE_ALLOWED_SINCE - 5) + + expect_offense(<<~RUBY) + class TestTextLimits < ActiveRecord::Migration[6.0] + def up + create_table :test_text_limit_attribute do |t| + t.integer :test_id, null: false + t.text :name, limit: 100 + ^^^^ Text columns should always have a limit set (255 is suggested). Using limit: is not supported in this version. You can add a limit to a `text` column by using `add_text_limit` or `.text_limit` inside `create_table` + end + + create_table_with_constraints :test_text_limit_attribute do |t| + t.integer :test_id, null: false + t.text :name, limit: 100 + ^^^^ Text columns should always have a limit set (255 is suggested). Using limit: is not supported in this version. You can add a limit to a `text` column by using `add_text_limit` or `.text_limit` inside `create_table` + end + end + end + RUBY + end + end end context 'when text array columns are defined without a limit' do diff --git a/spec/rubocop/cop/migration/prevent_index_creation_spec.rb b/spec/rubocop/cop/migration/prevent_index_creation_spec.rb index a3965f54bbd..ed7c8974d8d 100644 --- a/spec/rubocop/cop/migration/prevent_index_creation_spec.rb +++ b/spec/rubocop/cop/migration/prevent_index_creation_spec.rb @@ -6,28 +6,76 @@ require_relative '../../../../rubocop/cop/migration/prevent_index_creation' RSpec.describe RuboCop::Cop::Migration::PreventIndexCreation do subject(:cop) { described_class.new } + let(:forbidden_tables) { %w(ci_builds) } + let(:forbidden_tables_list) { forbidden_tables.join(', ') } + context 'when in migration' do before do allow(cop).to receive(:in_migration?).and_return(true) end context 'when adding an index to a forbidden table' do - it 'registers an offense when add_index is used' do - expect_offense(<<~RUBY) - def change - add_index :ci_builds, :protected - ^^^^^^^^^ Adding new index to ci_builds is forbidden, see https://gitlab.com/gitlab-org/gitlab/-/issues/332886 + context 'when table_name is a symbol' do + it "registers an offense when add_index is used", :aggregate_failures do + forbidden_tables.each do |table_name| + expect_offense(<<~RUBY) + def change + add_index :#{table_name}, :protected + ^^^^^^^^^ Adding new index to #{forbidden_tables_list} is forbidden, see https://gitlab.com/gitlab-org/gitlab/-/issues/332886 + end + RUBY end - RUBY + end + + it "registers an offense when add_concurrent_index is used", :aggregate_failures do + forbidden_tables.each do |table_name| + expect_offense(<<~RUBY) + def change + add_concurrent_index :#{table_name}, :protected + ^^^^^^^^^^^^^^^^^^^^ Adding new index to #{forbidden_tables_list} is forbidden, see https://gitlab.com/gitlab-org/gitlab/-/issues/332886 + end + RUBY + end + end end - it 'registers an offense when add_concurrent_index is used' do - expect_offense(<<~RUBY) - def change - add_concurrent_index :ci_builds, :protected - ^^^^^^^^^^^^^^^^^^^^ Adding new index to ci_builds is forbidden, see https://gitlab.com/gitlab-org/gitlab/-/issues/332886 + context 'when table_name is a string' do + it "registers an offense when add_index is used", :aggregate_failures do + forbidden_tables.each do |table_name| + expect_offense(<<~RUBY) + def change + add_index "#{table_name}", :protected + ^^^^^^^^^ Adding new index to #{forbidden_tables_list} is forbidden, see https://gitlab.com/gitlab-org/gitlab/-/issues/332886 + end + RUBY end - RUBY + end + + it "registers an offense when add_concurrent_index is used", :aggregate_failures do + forbidden_tables.each do |table_name| + expect_offense(<<~RUBY) + def change + add_concurrent_index "#{table_name}", :protected + ^^^^^^^^^^^^^^^^^^^^ Adding new index to #{forbidden_tables_list} is forbidden, see https://gitlab.com/gitlab-org/gitlab/-/issues/332886 + end + RUBY + end + end + end + + context 'when table_name is a constant' do + it "registers an offense when add_concurrent_index is used", :aggregate_failures do + expect_offense(<<~RUBY) + INDEX_NAME = "index_name" + TABLE_NAME = :ci_builds + disable_ddl_transaction! + + def change + add_concurrent_index TABLE_NAME, :protected + ^^^^^^^^^^^^^^^^^^^^ Adding new index to #{forbidden_tables_list} is forbidden, see https://gitlab.com/gitlab-org/gitlab/-/issues/332886 + end + RUBY + end end end @@ -39,6 +87,20 @@ RSpec.describe RuboCop::Cop::Migration::PreventIndexCreation do end RUBY end + + context 'when using a constant' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + disable_ddl_transaction! + + TABLE_NAME = "not_forbidden" + + def up + add_concurrent_index TABLE_NAME, :protected + end + RUBY + end + end end end diff --git a/spec/rubocop/cop/migration/versioned_migration_class_spec.rb b/spec/rubocop/cop/migration/versioned_migration_class_spec.rb new file mode 100644 index 00000000000..d9b0cd4546c --- /dev/null +++ b/spec/rubocop/cop/migration/versioned_migration_class_spec.rb @@ -0,0 +1,81 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' +require_relative '../../../../rubocop/cop/migration/versioned_migration_class' + +RSpec.describe RuboCop::Cop::Migration::VersionedMigrationClass do + subject(:cop) { described_class.new } + + let(:migration) do + <<~SOURCE + class TestMigration < Gitlab::Database::Migration[1.0] + def up + execute 'select 1' + end + + def down + execute 'select 1' + end + end + SOURCE + end + + shared_examples 'a disabled cop' do + it 'does not register any offenses' do + expect_no_offenses(migration) + end + end + + context 'outside of a migration' do + it_behaves_like 'a disabled cop' + end + + context 'in migration' do + before do + allow(cop).to receive(:in_migration?).and_return(true) + end + + context 'in an old migration' do + before do + allow(cop).to receive(:version).and_return(described_class::ENFORCED_SINCE - 5) + end + + it_behaves_like 'a disabled cop' + end + + context 'that is recent' do + before do + allow(cop).to receive(:version).and_return(described_class::ENFORCED_SINCE + 5) + end + + it 'adds an offence if inheriting from ActiveRecord::Migration' do + expect_offense(<<~RUBY) + class MyMigration < ActiveRecord::Migration[6.1] + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Don't inherit from ActiveRecord::Migration but use Gitlab::Database::Migration[1.0] instead. See https://docs.gitlab.com/ee/development/migration_style_guide.html#migration-helpers-and-versioning. + end + RUBY + end + + it 'adds an offence if including Gitlab::Database::MigrationHelpers directly' do + expect_offense(<<~RUBY) + class MyMigration < Gitlab::Database::Migration[1.0] + include Gitlab::Database::MigrationHelpers + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Don't include migration helper modules directly. Inherit from Gitlab::Database::Migration[1.0] instead. See https://docs.gitlab.com/ee/development/migration_style_guide.html#migration-helpers-and-versioning. + end + RUBY + end + + it 'excludes ActiveRecord classes defined inside the migration' do + expect_no_offenses(<<~RUBY) + class TestMigration < Gitlab::Database::Migration[1.0] + class TestModel < ApplicationRecord + end + + class AnotherTestModel < ActiveRecord::Base + end + end + RUBY + end + end + end +end diff --git a/spec/rubocop/cop/performance/active_record_subtransaction_methods_spec.rb b/spec/rubocop/cop/performance/active_record_subtransaction_methods_spec.rb new file mode 100644 index 00000000000..df18121e2df --- /dev/null +++ b/spec/rubocop/cop/performance/active_record_subtransaction_methods_spec.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' +require 'rspec-parameterized' + +require_relative '../../../../rubocop/cop/performance/active_record_subtransaction_methods' + +RSpec.describe RuboCop::Cop::Performance::ActiveRecordSubtransactionMethods do + subject(:cop) { described_class.new } + + let(:message) { described_class::MSG } + + shared_examples 'a method that uses a subtransaction' do |method_name| + it 'registers an offense' do + expect_offense(<<~RUBY, method_name: method_name, message: message) + Project.%{method_name} + ^{method_name} %{message} + RUBY + end + end + + context 'when the method uses a subtransaction' do + where(:method) { described_class::DISALLOWED_METHODS.to_a } + + with_them do + include_examples 'a method that uses a subtransaction', params[:method] + end + end +end diff --git a/spec/rubocop/cop/performance/active_record_subtransactions_spec.rb b/spec/rubocop/cop/performance/active_record_subtransactions_spec.rb new file mode 100644 index 00000000000..0da2e30062a --- /dev/null +++ b/spec/rubocop/cop/performance/active_record_subtransactions_spec.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' +require_relative '../../../../rubocop/cop/performance/active_record_subtransactions' + +RSpec.describe RuboCop::Cop::Performance::ActiveRecordSubtransactions do + subject(:cop) { described_class.new } + + let(:message) { described_class::MSG } + + context 'when calling #transaction with only requires_new: true' do + it 'registers an offense' do + expect_offense(<<~RUBY) + ApplicationRecord.transaction(requires_new: true) do + ^^^^^^^^^^^^^^^^^^ #{message} + Project.create!(name: 'MyProject') + end + RUBY + end + end + + context 'when passing multiple arguments to #transaction, including requires_new: true' do + it 'registers an offense' do + expect_offense(<<~RUBY) + ApplicationRecord.transaction(isolation: :read_committed, requires_new: true) do + ^^^^^^^^^^^^^^^^^^ #{message} + Project.create!(name: 'MyProject') + end + RUBY + end + end + + context 'when calling #transaction with requires_new: false' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + ApplicationRecord.transaction(requires_new: false) do + Project.create!(name: 'MyProject') + end + RUBY + end + end + + context 'when calling #transaction with other options' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + ApplicationRecord.transaction(isolation: :read_committed) do + Project.create!(name: 'MyProject') + end + RUBY + end + end + + context 'when calling #transaction with no arguments' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + ApplicationRecord.transaction do + Project.create!(name: 'MyProject') + end + RUBY + end + end +end diff --git a/spec/rubocop/cop/worker_data_consistency_spec.rb b/spec/rubocop/cop/sidekiq_load_balancing/worker_data_consistency_spec.rb index 5fa42bf2b87..cf8d0d1b66f 100644 --- a/spec/rubocop/cop/worker_data_consistency_spec.rb +++ b/spec/rubocop/cop/sidekiq_load_balancing/worker_data_consistency_spec.rb @@ -1,9 +1,9 @@ # frozen_string_literal: true require 'fast_spec_helper' -require_relative '../../../rubocop/cop/worker_data_consistency' +require_relative '../../../../rubocop/cop/sidekiq_load_balancing/worker_data_consistency' -RSpec.describe RuboCop::Cop::WorkerDataConsistency do +RSpec.describe RuboCop::Cop::SidekiqLoadBalancing::WorkerDataConsistency do subject(:cop) { described_class.new } before do diff --git a/spec/rubocop/cop/sidekiq_load_balancing/worker_data_consistency_with_deduplication_spec.rb b/spec/rubocop/cop/sidekiq_load_balancing/worker_data_consistency_with_deduplication_spec.rb new file mode 100644 index 00000000000..6e7212b1002 --- /dev/null +++ b/spec/rubocop/cop/sidekiq_load_balancing/worker_data_consistency_with_deduplication_spec.rb @@ -0,0 +1,166 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' +require 'rspec-parameterized' +require_relative '../../../../rubocop/cop/sidekiq_load_balancing/worker_data_consistency_with_deduplication' + +RSpec.describe RuboCop::Cop::SidekiqLoadBalancing::WorkerDataConsistencyWithDeduplication do + using RSpec::Parameterized::TableSyntax + + subject(:cop) { described_class.new } + + before do + allow(cop) + .to receive(:in_worker?) + .and_return(true) + end + + where(:data_consistency) { %i[delayed sticky] } + + with_them do + let(:strategy) { described_class::DEFAULT_STRATEGY } + let(:corrected) do + <<~CORRECTED + class SomeWorker + include ApplicationWorker + + data_consistency :#{data_consistency} + + deduplicate #{strategy}, including_scheduled: true + idempotent! + end + CORRECTED + end + + context 'when deduplication strategy is not explicitly set' do + it 'registers an offense and corrects using default strategy' do + expect_offense(<<~CODE) + class SomeWorker + include ApplicationWorker + + data_consistency :#{data_consistency} + + idempotent! + ^^^^^^^^^^^ Workers that declare either `:sticky` or `:delayed` data consistency [...] + end + CODE + + expect_correction(corrected) + end + + context 'when identation is different' do + let(:corrected) do + <<~CORRECTED + class SomeWorker + include ApplicationWorker + + data_consistency :#{data_consistency} + + deduplicate #{strategy}, including_scheduled: true + idempotent! + end + CORRECTED + end + + it 'registers an offense and corrects with correct identation' do + expect_offense(<<~CODE) + class SomeWorker + include ApplicationWorker + + data_consistency :#{data_consistency} + + idempotent! + ^^^^^^^^^^^ Workers that declare either `:sticky` or `:delayed` data consistency [...] + end + CODE + + expect_correction(corrected) + end + end + end + + context 'when deduplication strategy does not include including_scheduling option' do + let(:strategy) { ':until_executed' } + + it 'registers an offense and corrects' do + expect_offense(<<~CODE) + class SomeWorker + include ApplicationWorker + + data_consistency :#{data_consistency} + + deduplicate :until_executed + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Workers that declare either `:sticky` or `:delayed` data consistency [...] + idempotent! + end + CODE + + expect_correction(corrected) + end + end + + context 'when deduplication strategy has including_scheduling option disabled' do + let(:strategy) { ':until_executed' } + + it 'registers an offense and corrects' do + expect_offense(<<~CODE) + class SomeWorker + include ApplicationWorker + + data_consistency :#{data_consistency} + + deduplicate :until_executed, including_scheduled: false + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Workers that declare either `:sticky` or `:delayed` data consistency [...] + idempotent! + end + CODE + + expect_correction(corrected) + end + end + + context "when deduplication strategy is :none" do + it 'does not register an offense' do + expect_no_offenses(<<~CODE) + class SomeWorker + include ApplicationWorker + + data_consistency :always + + deduplicate :none + idempotent! + end + CODE + end + end + + context "when deduplication strategy has including_scheduling option enabled" do + it 'does not register an offense' do + expect_no_offenses(<<~CODE) + class SomeWorker + include ApplicationWorker + + data_consistency :always + + deduplicate :until_executing, including_scheduled: true + idempotent! + end + CODE + end + end + end + + context "data_consistency: :always" do + it 'does not register an offense' do + expect_no_offenses(<<~CODE) + class SomeWorker + include ApplicationWorker + + data_consistency :always + + idempotent! + end + CODE + end + end +end |