Add new cop rule RSpec/UselessDynamicDefinition (!172) · Merge requests · GitLab.org / ruby / gems / gitlab-styles

Peter Leitzen requested to merge pl-rspec-dynamic-definiition-via-each into master Apr 11, 2023

What does this MR do and why?

This MR adds a 🆕 👮 rule to prevent dynamic definition of hooks and lets in combination with .each/.each_key/.each_value without wrapping context.

The result of those definitions is unexpected:

Scenario	Expected	Actual
Hooks	Hook per value in separate context	Multiple hook definitions in the same context. Last wins.
Let	Let definition per context	Multiple definitions in the same context. Last wins.

It's important to note that every offense caused by this 👮 is a spec bug and can cause false spec results.

Example

# bad
context 'foo' do
  [true, false].each do |bool|
    before do # Two hook definitions. Last wins.
      stub_something(bool: bool)
    end
    
    # Two let definitions. Last wins.                                         
    let(:foo) { build(:model, bool: bool) }
                                              
    it 'works' do
      expect(foo.bool).to eq(bool) # `bool` is always `false`. PASSES ACCIDENTALLY.
    end
  end
end
                                              
# good
context 'foo' do
  [true, false].each do |bool|
    context "with bool #{bool}" do # The missing context
      before do
        stub_something(bool: bool)
      end
                                              
      let(:foo) { build(:model, bool: bool) }
                                              
      it 'works' do
        expect(foo.bool).to eq(bool) # `bool` alternates between true and false now.
      end
    end
  end
end

Closes #46 (closed).

Offenses in `gitlab-org/gitlab`

gitlab-org/gitlab#406761 (closed) to resolve current offenses.

30708 files inspected, 57 offenses detected

Note that factories were excluded:

RSpec/UselessDynamicDefinition:
  Exclude:
    - 'spec/factories/**/*.rb'
    - 'ee/spec/factories/**/*.rb'

Click to expand

Offenses:

ee/spec/helpers/nav_helper_spec.rb:19:35: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
      %w[your_work project group].each do |context_nav|
                                  ^^^^
ee/spec/helpers/push_rules_helper_spec.rb:51:44: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
    PushRule::SETTINGS_WITH_GLOBAL_DEFAULT.each do |rule_attr|
                                           ^^^^
ee/spec/lib/audit/project_changes_auditor_spec.rb:249:17: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
        columns.each do |column|
                ^^^^
ee/spec/lib/gitlab/graphql/loaders/bulk_epic_aggregate_loader_spec.rb:150:19: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
    [nil, [], ""].each do |empty_arg|
                  ^^^^
ee/spec/models/elasticsearch_indexed_namespace_spec.rb:38:29: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
    Plan::PAID_HOSTED_PLANS.each do |plan|
                            ^^^^
ee/spec/models/gitlab_subscription_spec.rb:8:56: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
  %i[free_plan bronze_plan premium_plan ultimate_plan].each do |plan|
                                                       ^^^^
ee/spec/services/ee/protected_branches/create_service_spec.rb:128:33: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
        %w(*ture *eatur* feat*).each do |wildcard|
                                ^^^^
ee/spec/services/package_metadata/data_object_spec.rb:31:50: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
        ['foo,v1,""', '"",v1,MIT', 'foo,"",MIT'].each do |t|
                                                 ^^^^
ee/spec/services/package_metadata/data_object_spec.rb:39:44: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
        ['foo,v1,', ',v1,MIT', 'foo,,MIT'].each do |t|
                                           ^^^^
ee/spec/support/shared_examples/features/protected_branches_access_control_shared_examples.rb:6:18: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
  %w[merge push].each do |git_operation|
                 ^^^^
ee/spec/support/shared_examples/lib/gitlab/elastic/search_results_shared_examples.rb:26:10: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
  scopes.each do |scope|
         ^^^^
ee/spec/workers/audit_events/audit_event_streaming_worker_spec.rb:230:33: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
      Gitlab::HTTP::HTTP_ERRORS.each do |error_klass|
                                ^^^^
ee/spec/workers/elastic_namespace_rollout_worker_spec.rb:13:27: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
  Plan::PAID_HOSTED_PLANS.each do |plan|
                          ^^^^
spec/controllers/import/bulk_imports_controller_spec.rb:217:71: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
          %w[https://localhost:3000 http://192.168.0.1 ftp://testing].each do |url|
                                                                      ^^^^
spec/controllers/import/bulk_imports_controller_spec.rb:234:59: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
            %w[https://localhost:3000 http://192.168.0.1].each do |url|
                                                          ^^^^
spec/controllers/import/gitea_controller_spec.rb:46:69: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
        %w[https://localhost:3000 http://192.168.0.1 ftp://testing].each do |url|
                                                                    ^^^^
spec/features/admin/admin_appearance_spec.rb:10:15: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
  flag_values.each do |val|
              ^^^^
spec/features/admin/admin_mode/login_spec.rb:17:17: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
    flag_values.each do |val|
                ^^^^
spec/features/users/signup_spec.rb:7:15: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
  flag_values.each do |val|
              ^^^^
spec/features/users/signup_spec.rb:67:15: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
  flag_values.each do |val|
              ^^^^
spec/finders/packages/group_packages_finder_spec.rb:206:21: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
      package_types.each { |pt| let_it_be("package_#{pt}") { create("#{pt}_package", project: project) } }
                    ^^^^
spec/haml_lint/linter/no_plain_nodes_spec.rb:71:37: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
    %w(&nbsp;&gt; &#x000A9; &#187;).each do |elem|
                                    ^^^^
spec/haml_lint/linter/no_plain_nodes_spec.rb:84:62: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
    %w(&nbsp;Test Test&gt; &#x000A9;Hello &nbsp;Hello&#187;).each do |elem|
                                                             ^^^^
spec/helpers/page_layout_helper_spec.rb:58:28: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
    %w(project user group).each do |type|
                           ^^^^
spec/lib/gitlab/consul/internal_spec.rb:79:15: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
    [nil, ''].each do |value|
              ^^^^
spec/lib/gitlab/diff/formatters/image_formatter_spec.rb:28:31: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
    [:width, :height, :x, :y].each do |attr|
                              ^^^^
spec/lib/gitlab/error_tracking_spec.rb:403:71: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
      ['Gitlab::SidekiqMiddleware::RetryError', 'SubclassRetryError'].each do |ex|
                                                                      ^^^^
spec/lib/gitlab/slug/environment_spec.rb:25:7: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
    }.each do |name, matcher|
      ^^^^
spec/lib/gitlab/utils/strong_memoize_spec.rb:264:41: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
    [nil, false, true, 'value', 0, [0]].each do |value|
                                        ^^^^
spec/models/award_emoji_spec.rb:71:53: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
      %i[issue merge_request note_on_issue snippet].each do |awardable_type|
                                                    ^^^^
spec/models/ci/job_artifact_spec.rb:485:65: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
        described_class.file_formats.except(file_format).values.each do |other_format|
                                                                ^^^^
spec/models/ci/job_token/allowlist_spec.rb:18:29: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
      [:inbound, :outbound].each do |d|
                            ^^^^
spec/models/ci/job_token/allowlist_spec.rb:49:27: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
    [:inbound, :outbound].each do |d|
                          ^^^^
spec/models/ci/job_token/scope_spec.rb:65:27: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
    [:inbound, :outbound].each do |d|
                          ^^^^
spec/models/ci/resource_group_spec.rb:121:39: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
    Ci::HasStatus::STATUSES_ENUM.keys.each do |status|
                                      ^^^^
spec/models/concerns/ci/has_status_spec.rb:395:61: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
    %w[ci_pipeline ci_stage ci_build generic_commit_status].each do |type|
                                                            ^^^^
spec/models/concerns/has_user_type_spec.rb:17:27: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
    User::USER_TYPES.keys.each do |type|
                          ^^^^
spec/models/merge_request_spec.rb:4726:26: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
      [:closed, :merged].each do |state|
                         ^^^^
spec/models/namespace/package_setting_spec.rb:49:42: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
      [:maven_package, :generic_package].each do |format|
                                         ^^^^
spec/models/namespace/package_setting_spec.rb:78:122: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
      [:npm_package, :conan_package, :nuget_package, :pypi_package, :composer_package, :golang_package, :debian_package].each do |format|
                                                                                                                         ^^^^
spec/models/note_spec.rb:808:52: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
      SystemNoteMetadata.new.cross_reference_types.each do |type|
                                                   ^^^^
spec/requests/api/badges_spec.rb:74:60: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
        %i[maintainer developer access_requester stranger].each do |type|
                                                           ^^^^
spec/requests/api/graphql/project/environments_spec.rb:105:27: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
    ::Deployment.statuses.each do |status, _|
                          ^^^^
spec/rubocop/cop/migration/create_table_with_foreign_keys_spec.rb:150:13: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
          ].each do |table|
            ^^^^
spec/services/clusters/cleanup/service_account_service_spec.rb:57:66: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
      ['Unauthorized', 'forbidden', 'Certificate verify Failed'].each do |message|
                                                                 ^^^^
spec/services/git/wiki_push_service/change_spec.rb:62:34: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
      %i[added renamed modified].each do |op|
                                 ^^^^
spec/services/metrics/dashboard/panel_preview_service_spec.rb:66:9: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
      ].each do |error_class|
        ^^^^
spec/services/packages/npm/generate_metadata_service_spec.rb:47:53: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
        ::Packages::DependencyLink.dependency_types.each_key do |dependency_type|
                                                    ^^^^^^^^
spec/services/packages/nuget/update_package_from_metadata_service_spec.rb:261:21: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
      invalid_names.each do |invalid_name|
                    ^^^^
spec/services/packages/nuget/update_package_from_metadata_service_spec.rb:280:24: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
      invalid_versions.each do |invalid_version|
                       ^^^^
spec/services/projects/update_service_spec.rb:359:31: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
      %w[issues wiki forking].each do |feature_name|
                              ^^^^
spec/services/spam/spam_verdict_service_spec.rb:320:47: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
           ::Spam::SpamConstants::BLOCK_USER].each do |verdict_value|
                                              ^^^^
spec/services/users/update_canonical_email_service_spec.rb:94:25: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
      [nil, 'nonsense'].each do |invalid_address|
                        ^^^^
spec/support/cycle_analytics_helpers/test_generation.rb:29:17: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
      scenarios.each do |start_time_conditions, end_time_conditions|
                ^^^^
spec/views/devise/sessions/new.html.haml_spec.rb:33:15: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
  flag_values.each do |val|
              ^^^^
spec/workers/packages/nuget/extraction_worker_spec.rb:75:21: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
      invalid_names.each do |invalid_name|
                    ^^^^
spec/workers/packages/nuget/extraction_worker_spec.rb:96:24: C: RSpec/UselessDynamicDefinition: Avoid useless dynamic definitions without context.
      invalid_versions.each do |invalid_version|
                       ^^^^

30708 files inspected, 57 offenses detected

Edited Apr 13, 2023 by Peter Leitzen

Add new cop rule RSpec/UselessDynamicDefinition

What does this MR do and why?

Example

Offenses in gitlab-org/gitlab

Merge request reports

Offenses in `gitlab-org/gitlab`