Skip to content

Failing test spec/models/merge_request_spec.rb#current_patch_id_sha

This test will always fail when the entire file is executed. It will then be re-tried and then it will pass. Which creates flaky test warnings.

Example job: https://gitlab.com/gitlab-org/gitlab/-/jobs/5366849892#L10972

To reproduce run the entire file:

rspec --format documentation --color ./spec/models/merge_request_spec.rb

The following test always fails:

  1) MergeRequest#current_patch_id_sha when related merge_request_diff does not have a patch_id_sha is expected to eq "ghi789"
     Failure/Error: it { is_expected.to eq(patch_id) }

       expected: "ghi789"
            got: nil

Since the file has many tests, here is a condensed version of the file that re-creates the issue:

# frozen_string_literal: true

require 'spec_helper'

RSpec.describe MergeRequest, factory_default: :keep, feature_category: :code_review_workflow do
  include RepoHelpers
  include ProjectForksHelper
  include ReactiveCachingHelpers

  using RSpec::Parameterized::TableSyntax

  let_it_be(:namespace) { create_default(:namespace).freeze }
  let_it_be(:project, refind: true) { create_default(:project, :repository).freeze }

  subject { create(:merge_request) }


  describe 'prepare' do
    it 'calls NewMergeRequestWorker' do
      expect(NewMergeRequestWorker).to receive(:perform_async)
        .with(subject.id, subject.author_id)

      subject.prepare
    end
  end


  describe '#current_patch_id_sha' do
    let(:merge_request) { build_stubbed(:merge_request) }
    let(:merge_request_diff) { build_stubbed(:merge_request_diff) }
    let(:patch_id) { 'ghi789' }

    subject(:current_patch_id_sha) { merge_request.current_patch_id_sha }

    before do
      allow(merge_request).to receive(:merge_request_diff).and_return(merge_request_diff)
      allow(merge_request_diff).to receive(:patch_id_sha).and_return(patch_id)
    end

    it { is_expected.to eq(patch_id) }

    context 'when related merge_request_diff does not have a patch_id_sha' do
      let(:diff_refs) { instance_double(Gitlab::Diff::DiffRefs, base_sha: base_sha, head_sha: head_sha) }
      let(:base_sha) { 'abc123' }
      let(:head_sha) { 'def456' }

      before do
        allow(merge_request_diff).to receive(:patch_id_sha).and_return(nil)
        allow(merge_request).to receive(:diff_refs).and_return(diff_refs)

        allow_next_instance_of(Repository) do |repo|
          allow(repo)
            .to receive(:get_patch_id)
            .with(diff_refs.base_sha, diff_refs.head_sha)
            .and_return(patch_id)
        end
      end

      it { is_expected.to eq(patch_id) }

      context 'when base_sha is nil' do
        let(:base_sha) { nil }

        it { is_expected.to be_nil }
      end

      context 'when head_sha is nil' do
        let(:head_sha) { nil }

        it { is_expected.to be_nil }
      end

      context 'when base_sha and head_sha match' do
        let(:head_sha) { base_sha }

        it { is_expected.to be_nil }
      end
    end
  end

end

Adding subject { build_stubbed(:merge_request) } under describe 'prepare' do, solves the problem for this simple case, but there are other tests in the file that also cause this test to fail.

The following discussion from !134555 (merged) should be addressed:

Edited by Jay McCure