Fix a flaky merge_requests_controller_spec.rb
What does this MR do and why?
The problem was as follows:
- Private build access level wasn't carried over to the forked project (in ProjectForksHelper).
- That made the spec fail on first attempt since the project member would indeed get pipelines in the JSON response.
- On second attempt, because
@merge_request
is memoized inProjects::MergeRequests::ApplicationController
, the MergeCommitDiff returned byCi::PipelinesForMergeRequestFinder#all_pipelines_for_merge_request
would be empty since it would look for the first attempt's MR diffs. - 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.
-
I have evaluated the MR acceptance checklist for this MR.
Edited by Rémy Coutable