## Context Mentioned by @engwan in Slack: > I think removing our patches in [spec/support/capybara_wait_for_requests.rb](https://gitlab.com/gitlab-org/gitlab/-/blob/60aa9151b23e1a4d208c2f87445ec9bc74f517bf/spec/support/capybara_wait_for_requests.rb) could really help our feature spec run times. > > We should write our specs in a better way and not rely on waiting for requests after every visit. ## Goal Investigate the impact on pipeline stability/speed if we are removing `spec/support/capybara_wait_for_requests.rb` # Agentic Development Instructions: Incremental Monkey Patch Removal ## Goal Remove the three auto-`wait_for_requests` behaviors from `spec/support/capybara_wait_for_requests.rb` one at a time, fixing only the failures caused by each removal before proceeding. ## Current State - Branch: `jmc-no-wait-system-specs` - The monkey patch on this branch already removes **all three** behaviors simultaneously: `visit`, `click_button`, `click_link` - Pipeline 2465030732 shows ~123 failures (63 CE + 60 EE) from this combined removal - Many iteration-2 fixes were reverted by the linter ## Step 0: Reset to Incremental Starting Point Restore `click_button` and `click_link` behaviors, keep only `visit` removal active: ```ruby # spec/support/capybara_wait_for_requests.rb module WaitForRequestsAfterVisitPage def visit(visit_uri, &block) super yield if block # wait_for_requests REMOVED — iteration 1 end end module WaitForRequestsAfterClickButton def click_button(locator = nil, max_wait_time: 2 * Capybara.default_max_wait_time, **options) super(locator, **options) wait_for_requests(max_wait_time: max_wait_time) # KEPT end end module WaitForRequestsAfterClickLink def click_link(locator = nil, **options, &block) super yield if block wait_for_requests # KEPT end end ``` Commit: `"iteration 1: remove auto wait_for_requests after visit only"` --- ## Iteration Loop (repeat for visit → click_link → click_button) ### Step 1: Push and trigger pipeline ```bash git push origin jmc-no-wait-system-specs # Note the pipeline URL from the push output ``` ### Step 2: After pipeline completes (~90 min), fetch failures Use the GitLab API to list failing jobs: ``` GET https://gitlab.com/api/v4/pipelines/{PIPELINE_ID}/jobs?per_page=100&scope=failed&private_token={TOKEN} ``` Paginate with `&page=2`, `&page=3`, etc. until the response is empty. For each failed job, fetch its trace: ``` GET https://gitlab.com/api/v4/jobs/{JOB_ID}/trace ``` Extract lines matching `rspec ./spec/features` or `rspec ./ee/spec/features` and deduplicate. ### Step 3: Triage each failure For each failing spec, read the file around the failing line. Identify the root cause: **Cause A — `before` block has bare `visit` with no page-load wait** The test starts interacting before JS renders. Fix by adding a `find` call after `visit`: ```ruby before do sign_in(user) visit some_path find_by_testid('work-item-title') # waits for page to be ready end ``` Choose the appropriate element based on page type: | Page type | Wait element | |-----------|-------------| | Work item / issue | `find_by_testid('work-item-title')` | | MR page | `find_by_testid('title-content')` | | Settings form | `find_field('...')` or `find_button('Save')` | | List page (issues) | `find('li.issue')` or `find_by_testid('filtered-search-input')` | | Admin / group settings | `find_by_testid('...-settings-content')` | | Blob / code editor | `find('#editor')` | **Cause B — non-Capybara assertion races the page (DB assertion before page assertion)** ```ruby # WRONG (races): click_something expect(model.reload.attr).to eq('value') # DB may not be updated yet expect(page).to have_content('Success') # CORRECT (page assertion waits): click_something expect(page).to have_content('Success') # waits for completion expect(model.reload.attr).to eq('value') # DB consistent by now ``` **Cause C — reading a DOM attribute before it is set** ```ruby # WRONG: visit path expect(find('#editor')['data-mode-id']).to eq('markdown') # might be nil # CORRECT: visit path expect(page).to have_css('#editor[data-mode-id="markdown"]') # retries until attr present expect(find('#editor')['data-mode-id']).to eq('markdown') # then reads it ``` **Cause D — unknown / unclear** Add a `# TODO: unclear wait needed — add explicit sync before this assertion` comment and move on. Do not guess. ### Step 4: Rules when applying fixes - **NEVER remove existing `wait_for_requests` or `wait_for_all_requests` calls** from test bodies — they were added intentionally for AJAX synchronization after clicks - **NEVER use `expect(page).to have_X` in `before` hooks** — triggers `RSpec/ExpectInHook` rubocop offense; use `find_by_testid`, `find`, `find_field`, `find_button`, `find_link`, `page.has_content?`, or `page.has_no_css?` instead - **NEVER use `all(...)` as a wait** — it is non-retrying and returns immediately if the DOM is empty - **`have_current_path` does NOT wait for page content** — only waits for the URL to change; not sufficient as a sole wait mechanism ### Step 5: Lint before committing ```bash bundle exec rubocop --only RSpec/ExpectInHook,Capybara/TestidFinders \ $(git diff --name-only HEAD) ``` ### Step 6: Commit and push ```bash git commit -m "fix: add page-load waits after removing auto wait_for_requests from visit" git push origin jmc-no-wait-system-specs ``` ### Step 7: Verify failure count drops, then proceed to next behavior Once the pipeline shows 0 failures attributable to the current removal, restore the next behavior in the monkey patch and repeat from Step 1. --- ## Agent Prompt: Fetching Pipeline Failures ``` Fetch failing test details from GitLab pipeline [URL]. 1. GET https://gitlab.com/api/v4/pipelines/{ID}/jobs?per_page=100&scope=failed Paginate with &page=2, &page=3 etc. until the response is empty. 2. For each failed job, GET https://gitlab.com/api/v4/jobs/{JOB_ID}/trace 3. Extract all lines matching "rspec ./spec/features" or "rspec ./ee/spec/features" 4. Deduplicate and return a sorted list of file:line pairs with the job name for context Use a PRIVATE-TOKEN header with the user's GitLab PAT for authentication. ``` ## Agent Prompt: Fixing a Batch of Failures ``` You are fixing system spec timing failures caused by removing auto-`wait_for_requests` after `visit` calls. For each file below: 1. Read the file around the failing line number 2. Find the `before` block or `it` block containing the `visit` call 3. Add an appropriate `find_by_testid` / `find` / `find_field` call immediately after `visit` — pick an element that will only be present once the JS-rendered page is fully interactive 4. Do NOT remove any existing `wait_for_requests` or `wait_for_all_requests` calls 5. Do NOT use `expect(page).to have_X` in `before` hooks — use `find` variants instead Files and failing lines: [list of file:line pairs from pipeline trace] ```