<details> <summary> Everyone can contribute. [Help move this issue forward](https://handbook.gitlab.com/handbook/marketing/developer-relations/contributor-success/community-contributors-workflows/#contributor-links) while earning points, leveling up and collecting rewards. </summary> - [Label this issue](https://contributors.gitlab.com/manage-issue?action=label&projectId=278964&issueIid=600963) </details> <details> <summary> Everyone can contribute. [Help move this issue forward](https://handbook.gitlab.com/handbook/marketing/developer-relations/contributor-success/community-contributors-workflows/#contributor-links) while earning points, leveling up and collecting rewards. </summary> - [Label this issue](https://contributors.gitlab.com/manage-issue?action=label&projectId=278964&issueIid=600963) </details> <details> <summary> Everyone can contribute. [Help move this issue forward](https://handbook.gitlab.com/handbook/marketing/developer-relations/contributor-success/community-contributors-workflows/#contributor-links) while earning points, leveling up and collecting rewards. </summary> - [Label this issue](https://contributors.gitlab.com/manage-issue?action=label&projectId=278964&issueIid=600963) </details> <details> <summary> Everyone can contribute. [Help move this issue forward](https://handbook.gitlab.com/handbook/marketing/developer-relations/contributor-success/community-contributors-workflows/#contributor-links) while earning points, leveling up and collecting rewards. </summary> - [Label this issue](https://contributors.gitlab.com/manage-issue?action=label&projectId=278964&issueIid=600963) </details> ## Context A new RSpec helper introduced in [!236979](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/236979) detects redundant `visit` calls across separate blocks within a single test example (e.g., a `before` hook and an `it` block visiting the same URL). Redundant visits cause unnecessary page loads and slow down the feature test suite. This builds on the prior cleanup effort in [!231073](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/231073) ("Reduce redundant visits in specs"), which already reduced visits by ~11% (924 → 819) across 34 spec files by moving `visit` calls so they don't happen twice or more times in the same test. When [!236979](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/236979) was run against the pipeline [#2539883238](https://gitlab.com/gitlab-org/gitlab/-/pipelines/2539883238), the detector flagged the following feature specs as having redundant `visit` calls (they currently fail because the detector raises an error on repeated visits). ## Goal Refactor each of the tests below so that **every example performs exactly one `visit` call** (the minimum required to load the page under test). Examples that already perform a single visit are fine; this issue targets examples where the same URL is loaded twice or more across separate `before`/`it`/nested context scopes. Beyond removing cross-scope redundancy inside a single example, also look for **per-file** reductions: sibling `it` blocks that each trigger the same `before { visit ... }` to assert independent facts about the same page can usually be collapsed into a single example with `:aggregate_failures`, turning N page loads into 1. ## How to fix (step-by-step) These instructions assume you are working on one of the spec files listed in **Tests to address** below. They describe the refactoring pattern used in [!237535](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/237535) for the admin general settings specs, which other agents can follow for the remaining files. ### 1. Set up local validation Pull the detector from [!236979](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/236979) into your local checkout (or rebase your branch onto it): ``` git fetch origin mrincon-reduce-reundant-3-navigations git checkout origin/mrincon-reduce-reundant-3-navigations -- spec/support/detect_redundant_visits.rb ``` The detector adds a `[VisitCount]` line to RSpec output for each example whose visit count is not 1, plus a per-spec-file summary at the end. It also raises an error inside the example body when a redundant cross-scope visit reaches the threshold. Run a single spec file to see the current state: ``` bundle exec rspec spec/features/.../foo_spec.rb ``` Look for `[VisitCount]` lines like: ``` [VisitCount] ./spec/features/.../foo_spec.rb[1:1:2:2] | 3 visit(s), 1 unique URL(s) - /some/path (spec/features/.../foo_spec.rb:42) - /some/path (spec/features/.../foo_spec.rb:55) - /some/path (spec/features/.../foo_spec.rb:73) ``` Each entry tells you which file + line invoked `visit`, so you can see exactly which scopes are duplicating work. ### 2. Identify the redundancy pattern The most common pattern is: a top-level / outer `before` block calls `visit some_path`, and then a nested `context`/`describe` also has its own `before` that calls `visit some_path` (after some additional setup). The detector flags the same URL visited from two or more different scopes within one example. Less common variants: - An `it` block calls `visit` itself even though the surrounding `before` already did. - A helper like `page.refresh` is called after an outer `visit` (`page.refresh` is implemented as `visit 'about:blank'` + `visit current_url`). - A `before` block does `current_settings.update!(...)` followed by `visit ...`, and the outer scope already visited. Additionally, look for **example-level redundancy** that the detector does not flag but that still causes extra page loads: - Multiple sibling `it` blocks inside the same `context`/`describe` that share a `before { visit ... }` and only assert different pieces of content on the same page. Each example triggers a fresh `visit`, multiplying page loads N-fold. ### 3. Apply one (or more) of these fixes Choose the approach that fits the structure of the test. Pick the smallest change that gets each example to exactly 1 visit (and, where possible, reduces the example count when several siblings exercise the same page). #### a. Move the outer `visit` into the nested scope that actually needs it If only some nested contexts need the page loaded, remove the `visit` from the outer `before` and add a `visit` to each `it`/nested `before` that does. Before: ```ruby RSpec.describe '...' do before do sign_in(user) visit some_path # every example inherits this end context 'feature flag enabled' do before do stub_feature_flags(foo: true) visit some_path # redundant end it { ... } end end ``` After: ```ruby RSpec.describe '...' do before do sign_in(user) end context 'feature flag enabled' do before do stub_feature_flags(foo: true) visit some_path # single visit end it { ... } end end ``` #### b. Push the `visit` into the `it` block when each example needs different setup When only one example in a `describe`/`context` needs the page loaded, do not put `visit` in a shared `before`. Put it inline in the `it`: ```ruby describe 'rare action' do it 'does the thing' do visit some_path click_button 'Do thing' expect(page).to have_content('Done') end end ``` #### c. Stage all setup before the single `visit` When an example uses `visit` + later `page.refresh` (or a second `visit`) to pick up DB state changes, reorder so all DB/feature-flag/setup happens **before** a single `visit`: ```ruby it 'reflects the new setting' do current_settings.update!(foo: true) create(:user) visit some_path expect(page).to have_content('Foo is enabled') end ``` #### d. Materialize `ApplicationSetting` without a `visit` (for admin settings specs) Specs that mutate `ApplicationSetting` in `before` hooks often rely on an earlier `visit` to lazily create the singleton record. Replace that implicit dependency by updating the local `current_settings` helper to materialize the record on first read: ```ruby def current_settings ApplicationSetting.current_without_cache || Gitlab::CurrentSettings.current_application_settings end ``` `Gitlab::CurrentSettings.current_application_settings` returns the existing record or creates one from defaults — it is the same accessor controllers use, so this is not a test-only hack. This unlocks moving the outer `visit` without breaking `current_settings.update!`/`update_attribute` calls in `before` blocks. The spec also needs `stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false')` (usually already present in the top-level `before`) so the record is persisted to the database rather than held only in memory. #### e. Collapse sibling `it` blocks into a single example with `:aggregate_failures` When several sibling `it` blocks share the same `before { visit ... }` and each only asserts a different piece of content on the same page, merge them into a single example with `:aggregate_failures`. Each `it` triggers the `before` independently — so N sibling examples means N page loads. Collapsing them turns those N loads into 1, and `:aggregate_failures` keeps the per-assertion reporting on failure. Before: ```ruby context 'when visiting the settings page' do before do visit some_path end it 'shows option A' do expect(page).to have_content 'Option A' end it 'shows option B' do expect(page).to have_content 'Option B' end it 'shows option C' do expect(page).to have_content 'Option C' end end ``` After: ```ruby context 'when visiting the settings page' do before do visit some_path end it 'shows all options', :aggregate_failures do expect(page).to have_content 'Option A' expect(page).to have_content 'Option B' expect(page).to have_content 'Option C' end end ``` Only collapse examples that genuinely describe a single behaviour (e.g., "renders the form options"). Do not merge examples that exercise different interactions or different paths — keep those separate so failures stay localised. ### 4. Add a completion check before reading the DB Where an example clicks "Save changes" via `expect_save_settings(...)` and then immediately reads `current_settings.foo` to verify persistence, add a visible-completion check first so the DB read does not race the POST: ```ruby expect_save_settings('some-section') expect(page).to have_content('Application settings saved successfully') expect(current_settings.foo).to be true ``` This is optional but worth doing whenever you touch one of these tests. ### 5. Verify the result Re-run the spec file. For each example, the detector should print either nothing (1 visit, the expected baseline) or a `[VisitCount]` line that you intentionally accept (e.g., a test that genuinely needs two distinct page loads). ``` bundle exec rspec spec/features/.../foo_spec.rb ``` The per-file summary at the end gives you the new totals to compare against the baseline. ### 6. Do not use `:allow_repeated_visits` The detector recognises an `:allow_repeated_visits` metadata tag that disables the check for an example or group. **Do not add this tag** unless the repeated visit is genuinely intentional (e.g., verifying persistence after a real browser reload), and justify it in the MR description. ## Tests to address ``` rspec ./ee/spec/features/admin/settings/general_settings_spec.rb:163 rspec ./ee/spec/features/admin/settings/general_settings_spec.rb:356 rspec ./spec/features/admin/settings/general_settings_spec.rb:562 rspec ./spec/features/admin/settings/general_settings_spec.rb:566 rspec ./spec/features/projects/settings/merge_requests_settings_spec.rb:83 rspec ./spec/features/projects/settings/merge_requests_settings_spec.rb:122 rspec ./spec/features/projects/settings/user_manages_merge_requests_settings_spec.rb:76 rspec ./spec/features/projects/settings/user_manages_merge_requests_settings_spec.rb:103 ``` Grouped by file: - `ee/spec/features/admin/settings/general_settings_spec.rb` — lines `163`, `356` ✅ Fixed in [!237535](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/237535) - `spec/features/admin/settings/general_settings_spec.rb` — lines `562`, `566` ✅ Fixed in [!237535](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/237535) - `spec/features/projects/settings/merge_requests_settings_spec.rb` — lines `83`, `122` — ✅ Fixed in [!237626](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/237626) - `spec/features/projects/settings/user_manages_merge_requests_settings_spec.rb` — lines `76`, `103` — open in [!237627](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/237627) ## Tests to address (threshold lowered to 2 redundant visits) The initial list above was produced with the detector configured to raise only when **3 or more** redundant visits were observed in a single example. Re-running the detector against pipeline [#2548950608](https://gitlab.com/gitlab-org/gitlab/-/pipelines/2548950608) with the threshold lowered to **2 redundant visits** surfaces a substantially larger set of feature specs. The same refactoring patterns (a–e) above apply. This list was extracted from the 10 most recent `rspec system pg17` / `rspec-ee system pg17` shards (FOSS shards 30–32, EE shards 1–16). Earlier FOSS shards (1–29) were not fully scraped; expect a few more entries when those are processed. ### FOSS specs - `spec/features/boards/boards_spec.rb` — lines `201`, `222` - `spec/features/merge_request/user_approves_spec.rb` — line `18` — open in [!237635](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/237635) - `spec/features/merge_request/user_awards_emoji_spec.rb` — lines `19`, `31` — open in [!237635](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/237635) - `spec/features/merge_request/user_suggests_changes_on_diff_spec.rb` — line `218` - `spec/features/projects/integrations/user_activates_issue_tracker_spec.rb` — `[1:3:1:1:2]` - `spec/features/snippets/notes_on_personal_snippets_spec.rb` — line `53` - `spec/features/uploads/user_uploads_avatar_to_profile_spec.rb` — line `10` - `spec/features/work_items/list/user_applies_work_items_list_filters_spec.rb` — `[1:1:9:1:1]`, `[1:1:9:1:2]` - `spec/features/work_items/list/user_views_work_items_list_spec.rb` — line `44`, `[1:1:1:1:1]`, `[1:1:1:1:2]`, `[1:1:1:1:3]` ### EE specs - `ee/spec/features/admin/admin_built_in_project_templates_settings_spec.rb` — lines `38`, `51`, `74`, `85` - `ee/spec/features/admin/admin_credentials_inventory_spec.rb` — `[1:2:2:1:1:1:3]` - `ee/spec/features/admin/admin_dashboard_spec.rb` — `[1:2:1:1:1:1:2:1:1]` - `ee/spec/features/admin/admin_merge_requests_approvals_spec.rb` — line `24` - `ee/spec/features/admin/admin_roles_spec.rb` — lines `55`, `83`, `104` - `ee/spec/features/admin/admin_security_and_compliance_settings_spec.rb` — line `48` - `ee/spec/features/admin/admin_users_spec.rb` — lines `45`, `49` - `ee/spec/features/admin/seat_alert_spec.rb` — line `46` - `ee/spec/features/admin/settings/ci_cd_settings_spec.rb` — lines `104`, `117` - `ee/spec/features/admin/settings/general_settings_spec.rb` — lines `146`, `163`, `195`, `210`, `311`, `322`, plus parameterised `[1:5:3:1:1]` through `[1:5:3:13:1]` - `ee/spec/features/admin/settings/reporting_settings_spec.rb` — lines `32`, `55` - `ee/spec/features/admin/settings/search_settings_spec.rb` — lines `113`, `159` - `ee/spec/features/admin/subscriptions/admin_views_subscription_spec.rb` — `[1:1:1:6:1:1]`, `[1:1:1:6:2:1]`, `[1:2:2:1:1]`, `[1:2:2:2:1]` - `ee/spec/features/ai_sidepanel_spec.rb` — `[1:5:1:1]`, `[1:5:1:2]` - `ee/spec/features/billings/qrtly_reconciliation_alert_spec.rb` — `[1:1:1:1:2:1:1]` - `ee/spec/features/boards/board_filters_spec.rb` — `[1:1:2:1:6:1]`, `[1:1:5:1:1]`, `[1:2:2:1:6:1]`, `[1:2:5:1:1]` - `ee/spec/features/boards/user_adds_lists_to_board_spec.rb` — `[1:1:5:1]`, `[1:2:5:1]` - `ee/spec/features/groups/analytics/cycle_analytics/multiple_value_streams_spec.rb` — `[1:2:2:1:3:1]` - `ee/spec/features/groups/group_roadmap_spec.rb` — lines `386`, `438` - `ee/spec/features/groups/group_settings_spec.rb` — lines `910`, `955`, `964`, `971`, `1063`, `1099` - `ee/spec/features/groups/iterations/user_edits_iteration_cadence_spec.rb` — line `41` - `ee/spec/features/groups/ldap_group_links_spec.rb` — lines `72`, `97`, `110` - `ee/spec/features/groups/saml_group_links_spec.rb` — lines `74`, `145` - `ee/spec/features/groups/saml_providers_spec.rb` — line `217` - `ee/spec/features/groups/settings/protected_environments_spec.rb` — lines `106`, `130`, `151` - `ee/spec/features/groups/settings/repository_spec.rb` — line `35` - `ee/spec/features/groups/usage_quotas/seats_tab_spec.rb` — lines `131`, `234` - `ee/spec/features/groups/wiki_spec.rb` — `[1:9:4:1]` - `ee/spec/features/invites_spec.rb` — line `25` - `ee/spec/features/merge_request/user_edits_approval_rules_mr_spec.rb` — line `108` — open in [!237635](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/237635) - `ee/spec/features/projects/quality/test_case_show_spec.rb` — line `183` - `ee/spec/features/projects/settings/protected_environments_spec.rb` — line `110` - `ee/spec/features/projects/settings/repository_spec.rb` — lines `47`, `66` - `ee/spec/features/trial_registrations/company_information_spec.rb` — line `105` - `ee/spec/features/users/identity_verification_spec.rb` — lines `54`, `72` - `ee/spec/features/work_items/detail/change_type_action_spec.rb` — line `35` - `ee/spec/features/work_items/epics/epic_work_item_detail_spec.rb` — `[1:1:3:1:1]`, `[1:1:11:2]`, `[1:1:12:5]`, `[1:1:13:2:1:1:1]`, `[1:1:13:2:1:2:1]`, `[1:1:13:2:1:3:1]`, `[1:1:13:2:1:4:1]`, `[1:1:13:2:1:4:2:1]` - `ee/spec/features/work_items/epics/epics_list_spec.rb` — line `130` — open in [!237882](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/237882) - `ee/spec/features/work_items/epics/referencing_epics_spec.rb` — line `130` — open in [!237882](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/237882) - `ee/spec/features/work_items/epics/user_comments_on_epic_spec.rb` — line `21` — open in [!237882](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/237882) - `ee/spec/features/work_items/issues/move_issue_resource_weight_events_spec.rb` — line `34` - `ee/spec/features/work_items/list/user_filters_work_items_spec.rb` — lines `220`, `310`, `353`, `464` - `ee/spec/features/work_items/okr_spec.rb` — `[1:1:3:1:1]`, `[1:1:8:2]`, `[1:1:9:5]`, `[1:1:12:1]`, `[1:1:12:2]`, `[1:2:2:2]`, `[1:2:3:5]`, `[1:2:8:1]`, `[1:2:8:2]` ## Acceptance criteria - [ ] Each test above runs cleanly with the repeated visits detector from [!236979](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/236979) enabled. - [ ] No new use of `:allow_repeated_visits` unless the repeated visit is genuinely intentional (justify in the MR). - [ ] Existing behaviour of the tests is preserved. - [ ] Every example in the touched spec file performs exactly 1 visit (verifiable in the detector's per-file summary). ## References - Detector MR: [!236979](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/236979) - Prior cleanup MR: [!231073](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/231073) - Worked example: [!237535](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/237535) (admin general settings specs) - Failing pipeline: [#2539883238](https://gitlab.com/gitlab-org/gitlab/-/pipelines/2539883238) - Lowered-threshold pipeline: [#2548950608](https://gitlab.com/gitlab-org/gitlab/-/pipelines/2548950608)