Commit adf003a9 authored by Shinya Maeda's avatar Shinya Maeda

Fix inconsistent `branch?` behavior

between Ci::Pipeline and Ci::Build

Add spec

Add more tests

ok
parent c994484d
...@@ -417,10 +417,6 @@ module Ci ...@@ -417,10 +417,6 @@ module Ci
@commit ||= Commit.lazy(project, sha) @commit ||= Commit.lazy(project, sha)
end end
def branch?
super && !merge_request?
end
def stuck? def stuck?
pending_builds.any?(&:stuck?) pending_builds.any?(&:stuck?)
end end
......
...@@ -4,7 +4,7 @@ module HasRef ...@@ -4,7 +4,7 @@ module HasRef
extend ActiveSupport::Concern extend ActiveSupport::Concern
def branch? def branch?
!tag? !tag? && !merge_request?
end end
def git_ref def git_ref
......
...@@ -101,6 +101,15 @@ FactoryBot.define do ...@@ -101,6 +101,15 @@ FactoryBot.define do
end end
end end
trait :with_merge_request_pipeline do
after(:build) do |merge_request|
merge_request.merge_request_pipelines << build(:ci_pipeline,
source: :merge_request,
merge_request: merge_request,
project: merge_request.source_project)
end
end
trait :deployed_review_app do trait :deployed_review_app do
target_branch 'pages-deploy-target' target_branch 'pages-deploy-target'
......
...@@ -2734,6 +2734,122 @@ describe Ci::Build do ...@@ -2734,6 +2734,122 @@ describe Ci::Build do
end end
end end
describe '#secret_group_variables' do
subject { build.secret_group_variables }
let!(:variable) { create(:ci_group_variable, protected: true, group: group) }
context 'when ref is branch' do
let(:build) { create(:ci_build, ref: 'master', tag: false, project: project) }
context 'when ref is protected' do
before do
create(:protected_branch, :developers_can_merge, name: 'master', project: project)
end
it { is_expected.to include(variable) }
end
context 'when ref is not protected' do
it { is_expected.not_to include(variable) }
end
end
context 'when ref is tag' do
let(:build) { create(:ci_build, ref: 'v1.1.0', tag: true, project: project) }
context 'when ref is protected' do
before do
create(:protected_tag, project: project, name: 'v*')
end
it { is_expected.to include(variable) }
end
context 'when ref is not protected' do
it { is_expected.not_to include(variable) }
end
end
context 'when ref is merge request' do
let(:merge_request) { create(:merge_request, :with_merge_request_pipeline) }
let(:pipeline) { merge_request.merge_request_pipelines.first }
let(:build) { create(:ci_build, ref: merge_request.source_branch, tag: false, pipeline: pipeline, project: project) }
context 'when ref is protected' do
before do
create(:protected_branch, :developers_can_merge, name: merge_request.source_branch, project: project)
end
it 'does not return protected variables as it is not supported for merge request pipelines' do
is_expected.not_to include(variable)
end
end
context 'when ref is not protected' do
it { is_expected.not_to include(variable) }
end
end
end
describe '#secret_project_variables' do
subject { build.secret_project_variables }
let!(:variable) { create(:ci_variable, protected: true, project: project) }
context 'when ref is branch' do
let(:build) { create(:ci_build, ref: 'master', tag: false, project: project) }
context 'when ref is protected' do
before do
create(:protected_branch, :developers_can_merge, name: 'master', project: project)
end
it { is_expected.to include(variable) }
end
context 'when ref is not protected' do
it { is_expected.not_to include(variable) }
end
end
context 'when ref is tag' do
let(:build) { create(:ci_build, ref: 'v1.1.0', tag: true, project: project) }
context 'when ref is protected' do
before do
create(:protected_tag, project: project, name: 'v*')
end
it { is_expected.to include(variable) }
end
context 'when ref is not protected' do
it { is_expected.not_to include(variable) }
end
end
context 'when ref is merge request' do
let(:merge_request) { create(:merge_request, :with_merge_request_pipeline) }
let(:pipeline) { merge_request.merge_request_pipelines.first }
let(:build) { create(:ci_build, ref: merge_request.source_branch, tag: false, pipeline: pipeline, project: project) }
context 'when ref is protected' do
before do
create(:protected_branch, :developers_can_merge, name: merge_request.source_branch, project: project)
end
it 'does not return protected variables as it is not supported for merge request pipelines' do
is_expected.not_to include(variable)
end
end
context 'when ref is not protected' do
it { is_expected.not_to include(variable) }
end
end
end
describe '#scoped_variables_hash' do describe '#scoped_variables_hash' do
context 'when overriding CI variables' do context 'when overriding CI variables' do
before do before do
......
...@@ -16,6 +16,16 @@ describe HasRef do ...@@ -16,6 +16,16 @@ describe HasRef do
it 'return true when tag is set to false' do it 'return true when tag is set to false' do
is_expected.to be_truthy is_expected.to be_truthy
end end
context 'when it was triggered by merge request' do
let(:merge_request) { create(:merge_request, :with_merge_request_pipeline) }
let(:pipeline) { merge_request.merge_request_pipelines.first }
let(:build) { create(:ci_build, pipeline: pipeline) }
it 'returns false' do
is_expected.to be_falsy
end
end
end end
context 'is not a tag' do context 'is not a tag' do
...@@ -55,5 +65,15 @@ describe HasRef do ...@@ -55,5 +65,15 @@ describe HasRef do
is_expected.to start_with(Gitlab::Git::BRANCH_REF_PREFIX) is_expected.to start_with(Gitlab::Git::BRANCH_REF_PREFIX)
end end
end end
context 'when it is triggered by a merge request' do
let(:merge_request) { create(:merge_request, :with_merge_request_pipeline) }
let(:pipeline) { merge_request.merge_request_pipelines.first }
let(:build) { create(:ci_build, tag: false, pipeline: pipeline) }
it 'returns nil' do
is_expected.to be_nil
end
end
end end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment