Skip to content

Fix a flaky merge_requests_controller_spec.rb

What does this MR do and why?

The problem was as follows:

  1. Private build access level wasn't carried over to the forked project (in ProjectForksHelper).
  2. That made the spec fail on first attempt since the project member would indeed get pipelines in the JSON response.
  3. On second attempt, because @merge_request is memoized in Projects::MergeRequests::ApplicationController, the MergeCommitDiff returned by Ci::PipelinesForMergeRequestFinder#all_pipelines_for_merge_request would be empty since it would look for the first attempt's MR diffs.
  4. This made the test pass because there would be no pipelines found, but it wasn't actually testing that private builds would prevent the pipelines from being returned, it was only passing due to a combination of rspec-retry and memoization of a controller's instance variable.

Due to this reason, I think we should disable retrying controller tests.

Note that this is the most flaky spec we have at the moment, but it's because it's not real flakiness as it's deterministic: always fail on first attempt; always pass on second attempt.

How to set up and validate locally

Prior to this change:

  • The spec would always fail without retrying locally: bin/rspec 'spec/controllers/projects/merge_requests_controller_spec.rb[1:8:2:3:1:1:1]'
  • The spec would always pass on second attempt locally: RETRIES=1 bin/rspec 'spec/controllers/projects/merge_requests_controller_spec.rb[1:8:2:3:1:1:1]'

With this change, the spec always passes on first attempt.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Rémy Coutable

Merge request reports