This work item is a living document, so the sections are organised kinda backwards, with the most frequently updated section first. ## How kivikakk/wait-for-requests-cleanup-harness> is the vibe-coded equivalent to Pedro's pedropombeiro/let-it-be-frozen-cleanup-harness>. It's produced: * https://gitlab.com/gitlab-org/gitlab/-/merge_requests/245555+ as an initial pilot run on "known easy" cases; and, * https://gitlab.com/gitlab-org/gitlab/-/merge_requests/245805+ as a secondary batch on more "known easy" cases. A third is on the way; each batch has also produced small reliability improvements to the harness, so we're ironing out the creases doing this on a single machine before doing anything fancy-scale. ## What Check out https://gitlab.com/gitlab-org/gitlab/-/work_items/525094+ — we used to automatically `wait_for_requests` after every `visit`, `click_button`, and `click_link`. We don't any more. (This issue has extensive direction on how to do non-racey (waiting) asserts the Right Way™.) @engwan cleaned this up with over a dozen MRs, and added a new cop, [`RSpec/AvoidWaitForRequests`](https://gitlab.com/gitlab-org/gitlab/-/blob/7c181b38cd07cbe365f868067bcf76ec0a3b0670/rubocop/cop/rspec/avoid_wait_for_requests.rb), to prevent new uses of `wait_for_requests` in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/234146+. This cop has about 600 file-based exemptions in `.rubocop_todo/rspec/avoid_wait_for_requests.yml`; around 2,100 calls in all. We want to remove _all_ exemptions by removing every such use, replacing them where necessary with purpose-specific waiting assertions. ### Prior art https://gitlab.com/gitlab-org/gitlab/-/work_items/600267+ is a model for "changing defaults across the whole codebase without 50,000 line unmergeable diffs". It similarly targeted a very large number of spec LoCs, rewriting them en-masse with 20 GCP VMs, plus LLM fallback where it wasn't able to manage the rewrite deterministically. An important difference is that that task produced MRs that were **no-ops**: it flipped the default, looked at what broke, and then applied opt-outs of that default (i.e. back to the previous default) progressively until it passed again — those changes made to get back to passing became the MRs. The net outcome was opting out in 16,000 individual call-sites, so that we could then change the default for the codebase as a whole (done in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/238430+). By contrast, this task seeks to change the behaviour of these \~2,100 call-sites, whether by simply removing unnecessary calls (see example below) or adding site-specific waiting assertions. Fortunately we have an order of magnitude fewer call-sites :meow_double_thumbs: ## Why `wait_for_requests` is staple of large Ruby monoliths' RSpec suite supports: we load a page or click a button; now we need to wait for the browser to finish doing its things before we proceed with our next action or check. This isn't actually part of RSpec and so each codebase evolves its own. [Here's some of ours](https://gitlab.com/gitlab-org/gitlab/-/blob/1203bc656a95cde8b3ee9fb005d2685a3428f0e5/spec/support/helpers/wait_for_requests.rb#L41-54): ```ruby # Wait for client-side AJAX requests def wait_for_requests(max_wait_time: 2 * Capybara.default_max_wait_time) wait_for('JS requests complete', max_wait_time: max_wait_time) do finished_all_js_requests? end end # Wait for active Rack requests and client-side AJAX requests def wait_for_all_requests wait_for('pending requests complete') do finished_all_rack_requests? && finished_all_js_requests? end end ``` (We also have a `wait_for_action_cable_requests`, but we'll call it out-of-scope for now.) This turns out to be a leading cause of unreliable tests! Why? We're not waiting for a certain thing we _need_, just a state where there are no requests (of certain type(s)) in flight. In practice, this means a given `wait_for_requests` call in a spec can end up returning at different points in time, depending on how the browser and the test machine are going that run. `wait_for` (also our code) runs its block with a default of a 10ms sleep between checks, returning with the result once it returns a truthy value, and raising an error after a default of 10 seconds have passed without that happening (20 seconds for `wait_for_requests`; both tripled (!) in CI — see [(a)](https://gitlab.com/gitlab-org/gitlab/-/blob/1203bc656a95cde8b3ee9fb005d2685a3428f0e5/spec/support/capybara.rb#L11) and [(b)](https://gitlab.com/gitlab-org/gitlab/-/blob/1203bc656a95cde8b3ee9fb005d2685a3428f0e5/spec/support/capybara.rb#L114)). A client-side request returning will often cause another client-side request to be fired off as a result; if `wait_for` happens to evaluate its block in the in-between processing time, no requests are outstanding, and `wait_for_requests` returns! Depending on all kinds of factors including the phase of the moon, you might ask for a button click, and then `wait_for_requests` runs and returns _before_ the button has done its job and caused a request to be fired off. There are all kinds of possibilities, because our condition is so non-specific. Thankfully, the solution is simple: instead of waiting for requests, wait for what you care about. Quite often the code writes (or unwrites) itself. Here's an example from my domain: ```ruby context 'in an issue' do let(:issue) { create(:issue, project: project, description: markdown) } it 'includes iframe embed correctly' do visit project_issue_path(project, issue) wait_for_requests expect(page).to have_css(expected_selector) end end ``` The solution? Just delete `wait_for_requests`! `have_css` is already a waiting selector: it'll retry on its own. Other cases will be more complicated and require a new find/assertion that's specific to what's being tested. ## See also * https://gitlab.com/gitlab-org/gitlab/-/work_items/603519+ (staff-only)