From 00ab9b938ef8e951b5433ecc0db9f22393c94bb7 Mon Sep 17 00:00:00 2001
From: Alexandru Croitor <acroitor@gitlab.com>
Date: Fri, 13 Dec 2024 22:03:38 +0200
Subject: [PATCH] Improve move and clone work item specs performance
Instead of running the move or clone service for each widget
we can run it once with all widgets and check that the output
is the expected one for each widget data. This reduces the time
spent on running the spec as we run it once vs 8 times now and
more times as we keep adding widgets.
This also prevents issues where the order in which some widgets
are run is important.
---
...eable_and_moveable_data_stared_examples.rb | 30 +++++
...eable_and_moveable_data_stared_examples.rb | 109 +++++++-----------
2 files changed, 74 insertions(+), 65 deletions(-)
create mode 100644 ee/spec/support/shared_examples/services/ee/work_items/data_sync/cloneable_and_moveable_data_stared_examples.rb
diff --git a/ee/spec/support/shared_examples/services/ee/work_items/data_sync/cloneable_and_moveable_data_stared_examples.rb b/ee/spec/support/shared_examples/services/ee/work_items/data_sync/cloneable_and_moveable_data_stared_examples.rb
new file mode 100644
index 000000000000..adc184c5e54c
--- /dev/null
+++ b/ee/spec/support/shared_examples/services/ee/work_items/data_sync/cloneable_and_moveable_data_stared_examples.rb
@@ -0,0 +1,30 @@
+# frozen_string_literal: true
+
+RSpec.shared_examples 'cloneable and moveable for ee widget data' do
+ def work_item_weights_source(work_item)
+ work_item.reload.weights_source&.slice(:rolled_up_weight, :rolled_up_completed_weight)
+ end
+
+ let_it_be(:weights_source) do
+ weights_source = create(:work_item_weights_source, work_item: original_work_item, rolled_up_weight: 20,
+ rolled_up_completed_weight: 50)
+ weights_source&.slice(:rolled_up_weight, :rolled_up_completed_weight)
+ end
+
+ let_it_be(:move) { WorkItems::DataSync::MoveService }
+ let_it_be(:clone) { WorkItems::DataSync::CloneService }
+
+ # rubocop: disable Layout/LineLength -- improved readability with one line per widget
+ let_it_be(:widgets) do
+ [
+ { widget_name: :weights_source, eval_value: :work_item_weights_source, expected_data: weights_source, operations: [move, clone] }
+ ]
+ end
+ # rubocop: enable Layout/LineLength
+
+ with_them do
+ context "with widget" do
+ it_behaves_like 'for clone and move services'
+ end
+ end
+end
diff --git a/spec/support/shared_examples/services/work_items/data_sync/cloneable_and_moveable_data_stared_examples.rb b/spec/support/shared_examples/services/work_items/data_sync/cloneable_and_moveable_data_stared_examples.rb
index 2121269e288c..7deb9140ec11 100644
--- a/spec/support/shared_examples/services/work_items/data_sync/cloneable_and_moveable_data_stared_examples.rb
+++ b/spec/support/shared_examples/services/work_items/data_sync/cloneable_and_moveable_data_stared_examples.rb
@@ -62,8 +62,6 @@
end
RSpec.shared_examples 'cloneable and moveable widget data' do
- using RSpec::Parameterized::TableSyntax
-
def work_item_assignees(work_item)
work_item.reload.assignees
end
@@ -175,79 +173,60 @@ def work_item_crm_contacts(work_item)
timelogs.pluck(:user_id, :time_spent)
end
- where(:widget_name, :eval_value, :expected_data, :operations) do
- :assignees | :work_item_assignees | ref(:assignees) | [ref(:move), ref(:clone)]
- :award_emoji | :work_item_award_emoji | ref(:award_emojis) | [ref(:move)]
- :email_participants | :work_item_emails | ref(:emails) | [ref(:move)]
- :milestone | :work_item_milestone | ref(:milestone) | [ref(:move), ref(:clone)]
- :subscriptions | :work_item_subscriptions | ref(:subscriptions) | [ref(:move)]
- :sent_notifications | :work_item_sent_notifications | ref(:notifications) | [ref(:move)]
- :timelogs | :work_item_timelogs | ref(:timelogs) | [ref(:move)]
- :customer_relations_contacts | :work_item_crm_contacts | ref(:crm_contacts) | [ref(:move), ref(:clone)]
- end
-
- with_them do
- context "with widget" do
- before do
- allow(original_work_item).to receive(:from_service_desk?).and_return(true)
- allow(WorkItems::CopyTimelogsWorker).to receive(:perform_async) do |*args|
- WorkItems::CopyTimelogsWorker.perform_inline(*args)
- end
+ let_it_be(:move) { WorkItems::DataSync::MoveService }
+ let_it_be(:clone) { WorkItems::DataSync::CloneService }
+
+ # rubocop: disable Layout/LineLength -- improved readability with one line per widget
+ let_it_be(:widgets) do
+ [
+ { widget_name: :assignees, eval_value: :work_item_assignees, expected_data: assignees, operations: [move, clone] },
+ { widget_name: :award_emoji, eval_value: :work_item_award_emoji, expected_data: award_emojis, operations: [move] },
+ { widget_name: :email_participants, eval_value: :work_item_emails, expected_data: emails, operations: [move] },
+ { widget_name: :milestone, eval_value: :work_item_milestone, expected_data: milestone, operations: [move, clone] },
+ { widget_name: :subscriptions, eval_value: :work_item_subscriptions, expected_data: subscriptions, operations: [move] },
+ { widget_name: :sent_notifications, eval_value: :work_item_sent_notifications, expected_data: notifications, operations: [move] },
+ { widget_name: :timelogs, eval_value: :work_item_timelogs, expected_data: timelogs, operations: [move] },
+ { widget_name: :customer_relations_contacts, eval_value: :work_item_crm_contacts, expected_data: crm_contacts, operations: [move, clone] }
+ ]
+ end
+ # rubocop: enable Layout/LineLength
+
+ context "with widget" do
+ before do
+ allow(original_work_item).to receive(:from_service_desk?).and_return(true)
+ allow(WorkItems::CopyTimelogsWorker).to receive(:perform_async) do |*args|
+ WorkItems::CopyTimelogsWorker.perform_inline(*args)
end
-
- it_behaves_like 'for clone and move services'
- end
- end
-
- RSpec.shared_examples 'cloneable and moveable for ee widget data' do
- using RSpec::Parameterized::TableSyntax
-
- def work_item_weights_source(work_item)
- work_item.reload.weights_source&.slice(:rolled_up_weight, :rolled_up_completed_weight)
- end
-
- let_it_be(:weights_source) do
- weights_source = create(:work_item_weights_source, work_item: original_work_item, rolled_up_weight: 20,
- rolled_up_completed_weight: 50)
- weights_source&.slice(:rolled_up_weight, :rolled_up_completed_weight)
- end
-
- where(:widget_name, :eval_value, :expected_data, :operations) do
- :weights_source | :work_item_weights_source | ref(:weights_source) | [ref(:move), ref(:clone)]
end
- with_them do
- context "with widget" do
- it_behaves_like 'for clone and move services'
- end
- end
+ it_behaves_like 'for clone and move services'
end
end
-# this shared context is only to be used for sharing the code beetween the shared examples for cloneable and movable
+# this shared example is only to be used for sharing the code between the shared examples for cloneable and movable
# widget data (and for EE widget data)
-RSpec.shared_context 'for clone and move services' do
- let(:move) { WorkItems::DataSync::MoveService }
- let(:clone) { WorkItems::DataSync::CloneService }
-
- it 'clones and moves the data', :aggregate_failures do
+RSpec.shared_examples 'for clone and move services' do
+ it 'clones and moves the data', :aggregate_failures, :sidekiq_inline do
new_work_item = service.execute[:work_item]
- widget_value = send(eval_value, new_work_item)
- if operations.include?(described_class)
- expect(widget_value).not_to be_blank
- # trick to compare single values and arrays with a single statement
- expect([widget_value].flatten).to match_array([expected_data].flatten)
- else
- expect(widget_value).to be_blank
- end
+ widgets.each do |widget|
+ widget_value = send(widget[:eval_value], new_work_item)
+
+ if widget[:operations].include?(described_class)
+ expect(widget_value).not_to be_blank
+ # trick to compare single values and arrays with a single statement
+ expect([widget_value].flatten).to match_array([widget[:expected_data]].flatten)
+ else
+ expect(widget_value).to be_blank
+ end
- cleanup_data = Feature.enabled?(:cleanup_data_source_work_item_data, original_work_item.resource_parent)
- if cleanup_data && described_class == move
- expect(original_work_item.reload.public_send(widget_name)).to be_blank
- elsif widget_name != :sent_notifications
- # sent notifications are moved from original work item to new work item rather than deleted afterwards.
- expect(original_work_item.reload.public_send(widget_name)).not_to be_blank
+ cleanup_data = Feature.enabled?(:cleanup_data_source_work_item_data, original_work_item.resource_parent)
+ if cleanup_data && described_class == move
+ expect(original_work_item.reload.public_send(widget[:widget_name])).to be_blank
+ elsif widget[:widget_name] != :sent_notifications
+ # sent notifications are moved from original work item to new work item rather than deleted afterwards.
+ expect(original_work_item.reload.public_send(widget[:widget_name])).not_to be_blank
+ end
end
end
end
--
GitLab