From 721a159efff3c83b12d02081b7e927f26bf0b3bb Mon Sep 17 00:00:00 2001
From: Alina Mihaila <amihaila@gitlab.com>
Date: Tue, 9 May 2023 14:07:58 +0000
Subject: [PATCH] Add AvoidConditionalStatements Rubocop rule
---
.rubocop.yml | 6 ++
.../rspec/avoid_conditional_statements.yml | 84 +++++++++++++++++++
.../cop/rspec/avoid_conditional_statements.rb | 35 ++++++++
.../avoid_conditional_statements_spec.rb | 42 ++++++++++
4 files changed, 167 insertions(+)
create mode 100644 .rubocop_todo/rspec/avoid_conditional_statements.yml
create mode 100644 rubocop/cop/rspec/avoid_conditional_statements.rb
create mode 100644 spec/rubocop/cop/rspec/avoid_conditional_statements_spec.rb
diff --git a/.rubocop.yml b/.rubocop.yml
index b3f24e12c2274..81ad4cd31f354 100644
--- a/.rubocop.yml
+++ b/.rubocop.yml
@@ -527,6 +527,12 @@ RSpec/AvoidTestProf:
- 'ee/spec/lib/gitlab/background_migration/**/*.rb'
- 'ee/spec/lib/ee/gitlab/background_migration/**/*.rb'
+RSpec/AvoidConditionalStatements:
+ Enabled: true
+ Include:
+ - 'spec/features/**/*.rb'
+ - 'ee/spec/features/**/*.rb'
+
RSpec/FactoriesInMigrationSpecs:
Enabled: true
Include:
diff --git a/.rubocop_todo/rspec/avoid_conditional_statements.yml b/.rubocop_todo/rspec/avoid_conditional_statements.yml
new file mode 100644
index 0000000000000..43ffaaa452ae3
--- /dev/null
+++ b/.rubocop_todo/rspec/avoid_conditional_statements.yml
@@ -0,0 +1,84 @@
+---
+RSpec/AvoidConditionalStatements:
+ Details: grace period
+ Exclude:
+ - 'ee/spec/features/admin/admin_settings_spec.rb'
+ - 'ee/spec/features/analytics/code_analytics_spec.rb'
+ - 'ee/spec/features/billings/billing_plans_spec.rb'
+ - 'ee/spec/features/boards/scoped_issue_board_spec.rb'
+ - 'ee/spec/features/boards/user_visits_board_spec.rb'
+ - 'ee/spec/features/ci_shared_runner_warnings_spec.rb'
+ - 'ee/spec/features/epic_boards/epic_boards_spec.rb'
+ - 'ee/spec/features/epics/epic_show_spec.rb'
+ - 'ee/spec/features/epics/gfm_autocomplete_spec.rb'
+ - 'ee/spec/features/group_protected_branches_spec.rb'
+ - 'ee/spec/features/groups/analytics/cycle_analytics/filters_and_data_spec.rb'
+ - 'ee/spec/features/groups/analytics/cycle_analytics/multiple_value_streams_spec.rb'
+ - 'ee/spec/features/groups/iterations/user_views_iteration_spec.rb'
+ - 'ee/spec/features/incidents/incident_details_spec.rb'
+ - 'ee/spec/features/issues/user_sees_empty_state_spec.rb'
+ - 'ee/spec/features/labels_hierarchy_spec.rb'
+ - 'ee/spec/features/profiles/usage_quotas_spec.rb'
+ - 'ee/spec/features/projects/analytics/visualization_designer_spec.rb'
+ - 'ee/spec/features/projects/licenses/maintainer_views_policies_spec.rb'
+ - 'ee/spec/features/projects/merge_requests/user_approves_merge_request_spec.rb'
+ - 'ee/spec/features/projects/settings/issues_settings_spec.rb'
+ - 'ee/spec/features/projects_spec.rb'
+ - 'ee/spec/features/registrations/email_confirmation_spec.rb'
+ - 'ee/spec/features/registrations/identity_verification_spec.rb'
+ - 'ee/spec/features/search/elastic/snippet_search_spec.rb'
+ - 'ee/spec/features/subscriptions/expiring_subscription_message_spec.rb'
+ - 'ee/spec/features/users/identity_verification_spec.rb'
+ - 'spec/features/admin/dashboard_spec.rb'
+ - 'spec/features/calendar_spec.rb'
+ - 'spec/features/groups/dependency_proxy_for_containers_spec.rb'
+ - 'spec/features/groups/empty_states_spec.rb'
+ - 'spec/features/groups/group_settings_spec.rb'
+ - 'spec/features/groups/members/sort_members_spec.rb'
+ - 'spec/features/groups/navbar_spec.rb'
+ - 'spec/features/issuables/issuable_list_spec.rb'
+ - 'spec/features/issuables/markdown_references/jira_spec.rb'
+ - 'spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb'
+ - 'spec/features/issues/user_bulk_edits_issues_labels_spec.rb'
+ - 'spec/features/issues/user_creates_branch_and_merge_request_spec.rb'
+ - 'spec/features/issues/user_edits_issue_spec.rb'
+ - 'spec/features/issues/user_interacts_with_awards_spec.rb'
+ - 'spec/features/labels_hierarchy_spec.rb'
+ - 'spec/features/markdown/keyboard_shortcuts_spec.rb'
+ - 'spec/features/merge_request/batch_comments_spec.rb'
+ - 'spec/features/merge_request/user_posts_diff_notes_spec.rb'
+ - 'spec/features/merge_request/user_reverts_merge_request_spec.rb'
+ - 'spec/features/merge_request/user_sees_avatar_on_diff_notes_spec.rb'
+ - 'spec/features/merge_request/user_squashes_merge_request_spec.rb'
+ - 'spec/features/merge_request/user_suggests_changes_on_diff_spec.rb'
+ - 'spec/features/monitor_sidebar_link_spec.rb'
+ - 'spec/features/oauth_login_spec.rb'
+ - 'spec/features/participants_autocomplete_spec.rb'
+ - 'spec/features/profiles/user_edit_profile_spec.rb'
+ - 'spec/features/projects/blobs/edit_spec.rb'
+ - 'spec/features/projects/branches_spec.rb'
+ - 'spec/features/projects/commit/cherry_pick_spec.rb'
+ - 'spec/features/projects/commit/user_reverts_commit_spec.rb'
+ - 'spec/features/projects/compare_spec.rb'
+ - 'spec/features/projects/deploy_keys_spec.rb'
+ - 'spec/features/projects/environments/environment_spec.rb'
+ - 'spec/features/projects/files/template_selector_menu_spec.rb'
+ - 'spec/features/projects/integrations/user_activates_issue_tracker_spec.rb'
+ - 'spec/features/projects/integrations/user_activates_jira_spec.rb'
+ - 'spec/features/projects/labels/user_removes_labels_spec.rb'
+ - 'spec/features/projects/members/sorting_spec.rb'
+ - 'spec/features/projects/milestones/milestone_spec.rb'
+ - 'spec/features/projects/releases/user_views_releases_spec.rb'
+ - 'spec/features/projects/settings/project_settings_spec.rb'
+ - 'spec/features/projects/settings/repository_settings_spec.rb'
+ - 'spec/features/projects/settings/user_transfers_a_project_spec.rb'
+ - 'spec/features/projects/show/user_sees_git_instructions_spec.rb'
+ - 'spec/features/projects/tree/create_directory_spec.rb'
+ - 'spec/features/projects/tree/create_file_spec.rb'
+ - 'spec/features/projects_spec.rb'
+ - 'spec/features/search/user_uses_header_search_field_spec.rb'
+ - 'spec/features/snippets/explore_spec.rb'
+ - 'spec/features/tags/developer_creates_tag_spec.rb'
+ - 'spec/features/usage_stats_consent_spec.rb'
+ - 'spec/features/users/login_spec.rb'
+ - 'spec/features/users/overview_spec.rb'
diff --git a/rubocop/cop/rspec/avoid_conditional_statements.rb b/rubocop/cop/rspec/avoid_conditional_statements.rb
new file mode 100644
index 0000000000000..48c230a6a7ae1
--- /dev/null
+++ b/rubocop/cop/rspec/avoid_conditional_statements.rb
@@ -0,0 +1,35 @@
+# frozen_string_literal: true
+
+require 'rubocop-rspec'
+
+module RuboCop
+ module Cop
+ module RSpec
+ # This cop checks for the usage of conditional statements in specs.
+ #
+ # @example
+ #
+ # # bad
+ #
+ # page.has_css?('[data-testid="begin-commit-button"]') ? find('[data-testid="begin-commit-button"]').click : nil
+ #
+ # if page.has_css?('[data-testid="begin-commit-button"]')
+ # find('[data-testid="begin-commit-button"]').click
+ # end
+ #
+ # unless page.has_css?('[data-testid="begin-commit-button"]')
+ # find('[data-testid="begin-commit-button"]').click
+ # end
+ class AvoidConditionalStatements < RuboCop::Cop::Base
+ MESSAGE = "Don't use `%{conditional}` conditional statments in specs, it might create flakiness. " \
+ "See https://gitlab.com/gitlab-org/gitlab/-/issues/385304#note_1345437109"
+
+ def on_if(node)
+ conditional = node.ternary? ? "#{node.condition.to_s.delete!("\n")} ? (ternary)" : node.keyword
+
+ add_offense(node, message: format(MESSAGE, conditional: conditional))
+ end
+ end
+ end
+ end
+end
diff --git a/spec/rubocop/cop/rspec/avoid_conditional_statements_spec.rb b/spec/rubocop/cop/rspec/avoid_conditional_statements_spec.rb
new file mode 100644
index 0000000000000..d2f5e4aa6190a
--- /dev/null
+++ b/spec/rubocop/cop/rspec/avoid_conditional_statements_spec.rb
@@ -0,0 +1,42 @@
+# frozen_string_literal: true
+
+require 'rubocop_spec_helper'
+
+require_relative '../../../../rubocop/cop/rspec/avoid_conditional_statements'
+
+RSpec.describe RuboCop::Cop::RSpec::AvoidConditionalStatements, feature_category: :tooling do
+ context 'when using conditionals' do
+ it 'flags if conditional' do
+ expect_offense(<<~RUBY)
+ if page.has_css?('[data-testid="begin-commit-button"]')
+ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Don't use `if` conditional statments in specs, it might create flakiness. See https://gitlab.com/gitlab-org/gitlab/-/issues/385304#note_1345437109
+ find('[data-testid="begin-commit-button"]').click
+ end
+ RUBY
+ end
+
+ it 'flags unless conditional' do
+ expect_offense(<<~RUBY)
+ RSpec.describe 'Multi-file editor new directory', :js, feature_category: :web_ide do
+ it 'creates directory in current directory' do
+ unless page.has_css?('[data-testid="begin-commit-button"]')
+ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Don't use `unless` conditional statments in specs, it might create flakiness. See https://gitlab.com/gitlab-org/gitlab/-/issues/385304#note_1345437109
+ find('[data-testid="begin-commit-button"]').click
+ end
+ end
+ end
+ RUBY
+ end
+
+ it 'flags ternary operator' do
+ expect_offense(<<~RUBY)
+ RSpec.describe 'Multi-file editor new directory', :js, feature_category: :web_ide do
+ it 'creates directory in current directory' do
+ user.present ? user : nil
+ ^^^^^^^^^^^^^^^^^^^^^^^^^ Don't use `(send (send nil :user) :present) ? (ternary)` conditional statments in specs, it might create flakiness. See https://gitlab.com/gitlab-org/gitlab/-/issues/385304#note_1345437109
+ end
+ end
+ RUBY
+ end
+ end
+end
--
GitLab