Commit e8a14346 authored by Dylan Griffith's avatar Dylan Griffith

Fix spec violations for internal_ids cross-database modifications

At present all the spec violations we found are false positives caused
by the fact that `ensure_project_iid!` is not called on the pipeline
before saving. We can remove many of them by just doing this in the
`after(:build)` for the `ci_pipeline` factory.

There are still a couple in `atomic_internal_id_shared_examples` but
these are specs that are explicitly trying to do things in transactions
and rolling things back so they aren't really representative of real
application code so it seemed easier to just wrap these in `allow_...`
for now and extract an issue if we ever want to actually refactor these
tests. It may be tricky to refactor them because they are used for other
models where these transaction behaviours being tested do indeed matter.
parent 31b5b104
...@@ -18,15 +18,13 @@ FactoryBot.define do ...@@ -18,15 +18,13 @@ FactoryBot.define do
transient { child_of { nil } } transient { child_of { nil } }
transient { upstream_of { nil } } transient { upstream_of { nil } }
before(:create) do |pipeline, evaluator|
pipeline.ensure_project_iid!
end
after(:build) do |pipeline, evaluator| after(:build) do |pipeline, evaluator|
if evaluator.child_of if evaluator.child_of
pipeline.project = evaluator.child_of.project pipeline.project = evaluator.child_of.project
pipeline.source = :parent_pipeline pipeline.source = :parent_pipeline
end end
pipeline.ensure_project_iid!
end end
after(:create) do |pipeline, evaluator| after(:create) do |pipeline, evaluator|
......
...@@ -46,7 +46,6 @@ ...@@ -46,7 +46,6 @@
- "./spec/models/ci/group_variable_spec.rb" - "./spec/models/ci/group_variable_spec.rb"
- "./spec/models/ci/job_artifact_spec.rb" - "./spec/models/ci/job_artifact_spec.rb"
- "./spec/models/ci/job_variable_spec.rb" - "./spec/models/ci/job_variable_spec.rb"
- "./spec/models/ci/pipeline_spec.rb"
- "./spec/models/ci/runner_spec.rb" - "./spec/models/ci/runner_spec.rb"
- "./spec/models/ci/variable_spec.rb" - "./spec/models/ci/variable_spec.rb"
- "./spec/models/clusters/applications/runner_spec.rb" - "./spec/models/clusters/applications/runner_spec.rb"
......
...@@ -80,15 +80,22 @@ RSpec.shared_examples 'AtomicInternalId' do |validate_presence: true| ...@@ -80,15 +80,22 @@ RSpec.shared_examples 'AtomicInternalId' do |validate_presence: true|
it 'calls InternalId.generate_next and sets internal id attribute' do it 'calls InternalId.generate_next and sets internal id attribute' do
iid = rand(1..1000) iid = rand(1..1000)
expect(InternalId).to receive(:generate_next).with(instance, scope_attrs, usage, any_args).and_return(iid) # Need to do this before evaluating instance otherwise it gets set
# already in factory
allow(InternalId).to receive(:generate_next).and_return(iid)
subject subject
expect(read_internal_id).to eq(iid) expect(read_internal_id).to eq(iid)
expect(InternalId).to have_received(:generate_next).with(instance, scope_attrs, usage, any_args)
end end
it 'does not overwrite an existing internal id' do it 'does not overwrite an existing internal id' do
write_internal_id(4711) write_internal_id(4711)
expect { subject }.not_to change { read_internal_id } allow_cross_database_modification_within_transaction(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/347091') do
expect { subject }.not_to change { read_internal_id }
end
end end
context 'when the instance has an internal ID set' do context 'when the instance has an internal ID set' do
...@@ -101,6 +108,7 @@ RSpec.shared_examples 'AtomicInternalId' do |validate_presence: true| ...@@ -101,6 +108,7 @@ RSpec.shared_examples 'AtomicInternalId' do |validate_presence: true|
.to receive(:track_greatest) .to receive(:track_greatest)
.with(instance, scope_attrs, usage, internal_id, any_args) .with(instance, scope_attrs, usage, internal_id, any_args)
.and_return(internal_id) .and_return(internal_id)
subject subject
end end
end end
...@@ -110,7 +118,11 @@ RSpec.shared_examples 'AtomicInternalId' do |validate_presence: true| ...@@ -110,7 +118,11 @@ RSpec.shared_examples 'AtomicInternalId' do |validate_presence: true|
context 'when the internal id has been changed' do context 'when the internal id has been changed' do
context 'when the internal id is automatically set' do context 'when the internal id is automatically set' do
it 'clears it on the instance' do it 'clears it on the instance' do
expect_iid_to_be_set_and_rollback write_internal_id(nil)
allow_cross_database_modification_within_transaction(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/347091') do
expect_iid_to_be_set_and_rollback
end
expect(read_internal_id).to be_nil expect(read_internal_id).to be_nil
end end
...@@ -120,7 +132,9 @@ RSpec.shared_examples 'AtomicInternalId' do |validate_presence: true| ...@@ -120,7 +132,9 @@ RSpec.shared_examples 'AtomicInternalId' do |validate_presence: true|
it 'does not clear it on the instance' do it 'does not clear it on the instance' do
write_internal_id(100) write_internal_id(100)
expect_iid_to_be_set_and_rollback allow_cross_database_modification_within_transaction(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/347091') do
expect_iid_to_be_set_and_rollback
end
expect(read_internal_id).not_to be_nil expect(read_internal_id).not_to be_nil
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