From 0d162601f4c0106b632297bfa47a8a3a6290bbaf Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Tue, 7 Jun 2016 09:17:09 -0600 Subject: [PATCH 1/4] Fix rake task specs - only run rake tasks once Our specs were running each rake task multiple times, for a couple different reasons. While this is great to ensure that rake tasks are safe to run multiple times, that probably wasn't the intention. :) First problem: Loading the rake tasks in a `before` block meant that we call `Rails.application.load_tasks` multiple times. And that meant that a single task would get run multiple times. e.g.: - spec example 1 - runs its task one time. - spec example 2 - runs its task two times. - spec example 3 - runs its task three times. - ... Second problem: `Rails.application.load_tasks` doesn't seem to work correctly in specs. For some reason it causes rake tasks to run exactly twice. So even if we avoid the previous problem mentioned above, we'd still run each task two times. This commit adds a "rake" shared_context that fixes these problems. It also adds a dummy_task_spec that tests the above issues when using the shared context. --- core/spec/lib/tasks/dummy_task.rake | 10 +++++++ core/spec/lib/tasks/dummy_task_spec.rb | 28 +++++++++++++++++ core/spec/lib/tasks/exchanges_spec.rb | 13 ++++---- .../copy_shipped_shipments_to_cartons_spec.rb | 13 ++++---- core/spec/lib/tasks/order_capturing_spec.rb | 13 ++++---- core/spec/support/shared_contexts/rake.rb | 30 +++++++++++++++++++ 6 files changed, 83 insertions(+), 24 deletions(-) create mode 100644 core/spec/lib/tasks/dummy_task.rake create mode 100644 core/spec/lib/tasks/dummy_task_spec.rb create mode 100644 core/spec/support/shared_contexts/rake.rb diff --git a/core/spec/lib/tasks/dummy_task.rake b/core/spec/lib/tasks/dummy_task.rake new file mode 100644 index 00000000000..563655239d6 --- /dev/null +++ b/core/spec/lib/tasks/dummy_task.rake @@ -0,0 +1,10 @@ +# This is a dummy task used for generic rake task testing + +task dummy_task: :environment do + DummyTaskRunner.run +end + +class DummyTaskRunner + def self.run + end +end diff --git a/core/spec/lib/tasks/dummy_task_spec.rb b/core/spec/lib/tasks/dummy_task_spec.rb new file mode 100644 index 00000000000..1a71b268b9e --- /dev/null +++ b/core/spec/lib/tasks/dummy_task_spec.rb @@ -0,0 +1,28 @@ +require 'spec_helper' + +describe 'dummy_task' do + include_context( + 'rake', + task_name: 'dummy_task', + task_path: Spree::Core::Engine.root.join('spec/lib/tasks/dummy_task.rake'), + ) + + it 'calls the dummy task exactly once' do + expect(DummyTaskRunner).to receive(:run).once + task.invoke + end + + # This tests: + # 1) that tasks get reenabled between examples + # 2) that tasks aren't loaded in the wrong way, causing them to execute + # an extra time for every example that's defined. + # We need at least two specs to trigger the error conditions and spec order is + # random so we just create the same spec twice. We could probably combine this + # with the generic spec above but this seems clearer. + 2.times do |i| + it "still calls the dummy task exactly once when more than one example is defined - #{i}" do + expect(DummyTaskRunner).to receive(:run).once + task.invoke + end + end +end diff --git a/core/spec/lib/tasks/exchanges_spec.rb b/core/spec/lib/tasks/exchanges_spec.rb index b8d471f6933..01d02398f9a 100644 --- a/core/spec/lib/tasks/exchanges_spec.rb +++ b/core/spec/lib/tasks/exchanges_spec.rb @@ -1,14 +1,11 @@ require 'spec_helper' describe "exchanges:charge_unreturned_items" do - let(:task) do - Rake::Task['exchanges:charge_unreturned_items'] - end - - before do - Rails.application.load_tasks - task.reenable - end + include_context( + 'rake', + task_name: 'exchanges:charge_unreturned_items', + task_path: Spree::Core::Engine.root.join('lib/tasks/exchanges.rake'), + ) subject { task } diff --git a/core/spec/lib/tasks/migrations/copy_shipped_shipments_to_cartons_spec.rb b/core/spec/lib/tasks/migrations/copy_shipped_shipments_to_cartons_spec.rb index 21dd99e902b..c69a131877d 100644 --- a/core/spec/lib/tasks/migrations/copy_shipped_shipments_to_cartons_spec.rb +++ b/core/spec/lib/tasks/migrations/copy_shipped_shipments_to_cartons_spec.rb @@ -1,16 +1,13 @@ require 'spec_helper' describe 'spree:migrations:copy_shipped_shipments_to_cartons' do - before do - Rails.application.load_tasks - task.reenable - end + include_context( + 'rake', + task_name: 'spree:migrations:copy_shipped_shipments_to_cartons:up', + task_path: Spree::Core::Engine.root.join('lib/tasks/migrations/copy_shipped_shipments_to_cartons.rake'), + ) describe 'up' do - let(:task) do - Rake::Task['spree:migrations:copy_shipped_shipments_to_cartons:up'] - end - # should generate a carton let!(:shipped_shipment) { shipped_order.shipments.first } # should not generate a carton because it's not shipped diff --git a/core/spec/lib/tasks/order_capturing_spec.rb b/core/spec/lib/tasks/order_capturing_spec.rb index bdd0eb7e15f..5642d6cc3f0 100644 --- a/core/spec/lib/tasks/order_capturing_spec.rb +++ b/core/spec/lib/tasks/order_capturing_spec.rb @@ -1,14 +1,11 @@ require 'spec_helper' describe "order_capturing:capture_payments" do - let(:task) do - Rake::Task['order_capturing:capture_payments'] - end - - before do - Rails.application.load_tasks - task.reenable - end + include_context( + 'rake', + task_name: 'order_capturing:capture_payments', + task_path: Spree::Core::Engine.root.join('lib/tasks/order_capturing.rake'), + ) subject { task } diff --git a/core/spec/support/shared_contexts/rake.rb b/core/spec/support/shared_contexts/rake.rb new file mode 100644 index 00000000000..feac9878007 --- /dev/null +++ b/core/spec/support/shared_contexts/rake.rb @@ -0,0 +1,30 @@ +# +# Rake task spec setup. +# +shared_context "rake" do |task_path:, task_name:| + require 'rake' + + let(:task) do + Rake::Task[task_name] + end + + before(:each) do + # we need to reenable the task or else `task.invoke` will only run the task + # for the first example that runs. + task.reenable + end + + before(:all) do + Rake::Task.clear + # Note: Using `Rails.application.load_tasks` doesn't seem to work correctly + # in the specs. The tasks each run twice when invoked instead of once. + load task_path + # Many tasks require the 'environment' task, which isn't needed in specs + # since the environment is already loaded. So generate a fake one. + Rake::Task.define_task(:environment) + end + + after(:all) do + Rake::Task.clear + end +end From 789b6df5058a7bc1226b31cb9e51fd8e90c79a57 Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Tue, 7 Jun 2016 10:53:05 -0600 Subject: [PATCH 2/4] Fix bug in rake task "assure_store_on_orders" Missing the `Spree::` prefix. Also add some specs for the rake task. --- .../migrations/assure_store_on_orders.rake | 2 +- .../migrations/assure_store_on_orders_spec.rb | 47 +++++++++++++++++++ 2 files changed, 48 insertions(+), 1 deletion(-) create mode 100644 core/spec/lib/tasks/migrations/assure_store_on_orders_spec.rb diff --git a/core/lib/tasks/migrations/assure_store_on_orders.rake b/core/lib/tasks/migrations/assure_store_on_orders.rake index f3567915cce..0a92c8a2cc0 100644 --- a/core/lib/tasks/migrations/assure_store_on_orders.rake +++ b/core/lib/tasks/migrations/assure_store_on_orders.rake @@ -25,7 +25,7 @@ namespace :solidus do TEXT end - default_store = Store.where(default: true).first + default_store = Spree::Store.where(default: true).first unless default_store abort "Your store is not marked as default. Please mark your one store as the default store and run this task again." end diff --git a/core/spec/lib/tasks/migrations/assure_store_on_orders_spec.rb b/core/spec/lib/tasks/migrations/assure_store_on_orders_spec.rb new file mode 100644 index 00000000000..6a383da75b8 --- /dev/null +++ b/core/spec/lib/tasks/migrations/assure_store_on_orders_spec.rb @@ -0,0 +1,47 @@ +require 'spec_helper' + +describe 'solidus:migrations:assure_store_on_orders' do + describe 'up' do + include_context( + 'rake', + task_path: Spree::Core::Engine.root.join('lib/tasks/migrations/assure_store_on_orders.rake'), + task_name: 'solidus:migrations:assure_store_on_orders:up', + ) + + context 'with no orders' do + it 'succeeds' do + expect { task.invoke }.to output( + "Everything is good, all orders in your database have a store attached.\n" + ).to_stdout + end + end + + context 'when all orders have store_ids' do + let!(:order) { create(:order, store: create(:store)) } + + it 'succeeds' do + expect { task.invoke }.to output( + "Everything is good, all orders in your database have a store attached.\n" + ).to_stdout + end + end + + context 'when some orders do not have store_ids' do + let!(:order_with_store) { create(:order, store: store) } + let!(:order_without_store) do + # due to a before_validation that adds a store when one is missing, + # we can't simply specify `store: nil` + create(:order, store: store).tap do |o| + o.update_columns(store_id: nil) + end + end + let!(:store) { create(:store) } + + it 'succeeds' do + expect { task.invoke }.to output( + "All orders updated with the default store.\n" + ).to_stdout + end + end + end +end From 58515658991c10a3d8456cd3a8ee0b96188ba837 Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Tue, 7 Jun 2016 11:03:30 -0600 Subject: [PATCH 3/4] Change `abort` to `raise` in "assure_store_on_orders" Otherwise specs don't run properly and any configured exception handlers may not be triggered. And add specs. --- .../migrations/assure_store_on_orders.rake | 6 ++-- .../migrations/assure_store_on_orders_spec.rb | 33 +++++++++++++++++++ 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/core/lib/tasks/migrations/assure_store_on_orders.rake b/core/lib/tasks/migrations/assure_store_on_orders.rake index 0a92c8a2cc0..767aefa4fd2 100644 --- a/core/lib/tasks/migrations/assure_store_on_orders.rake +++ b/core/lib/tasks/migrations/assure_store_on_orders.rake @@ -16,9 +16,9 @@ namespace :solidus do spree_store_count = Spree::Store.count if spree_store_count == 0 - abort "You do not have a store set up. Please create a store instance for your installation." + raise "You do not have a store set up. Please create a store instance for your installation." elsif spree_store_count > 1 - abort(<<-TEXT.squish) + raise(<<-TEXT.squish) You have more than one store set up. We can not be sure which store to attach your orders to. Please attach store ids to all your orders, and run this task again when you're finished. @@ -27,7 +27,7 @@ namespace :solidus do default_store = Spree::Store.where(default: true).first unless default_store - abort "Your store is not marked as default. Please mark your one store as the default store and run this task again." + raise "Your store is not marked as default. Please mark your one store as the default store and run this task again." end Spree::Order.where(store_id: nil).update_all(store_id: Spree::Store.default.id) diff --git a/core/spec/lib/tasks/migrations/assure_store_on_orders_spec.rb b/core/spec/lib/tasks/migrations/assure_store_on_orders_spec.rb index 6a383da75b8..d639b2a05f3 100644 --- a/core/spec/lib/tasks/migrations/assure_store_on_orders_spec.rb +++ b/core/spec/lib/tasks/migrations/assure_store_on_orders_spec.rb @@ -42,6 +42,39 @@ "All orders updated with the default store.\n" ).to_stdout end + + context 'when there are no stores' do + before do + order_with_store.update_columns(store_id: nil) + # due to a before_validation that adds a store when one is missing, + # we can't simply specify `store: nil` + store.destroy + end + + it 'raises' do + expect { task.invoke }.to raise_error(/You do not have a store set up/) + end + end + + context 'when there are multiple stores' do + let!(:extra_store) { create(:store) } + + it 'raises' do + expect { task.invoke }.to raise_error(/You have more than one store set up/) + end + end + + context 'when there is no default store' do + before do + # The before_save 'ensure_default_exists_and_is_unique' means that we + # need to use update_columns to set up this scenario. + store.update_columns(default: false) + end + + it 'raises' do + expect { task.invoke }.to raise_error(/Your store is not marked as default/) + end + end end end end From 480e580d55940a687f009fca416c450a342fb286 Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Tue, 7 Jun 2016 12:09:00 -0600 Subject: [PATCH 4/4] Rename assure -> ensure for ensure_store_on_orders task --- ...ure_store_on_orders.rake => ensure_store_on_orders.rake} | 2 +- core/lib/tasks/upgrade.rake | 2 +- ...ore_on_orders_spec.rb => ensure_store_on_orders_spec.rb} | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) rename core/lib/tasks/migrations/{assure_store_on_orders.rake => ensure_store_on_orders.rake} (97%) rename core/spec/lib/tasks/migrations/{assure_store_on_orders_spec.rb => ensure_store_on_orders_spec.rb} (93%) diff --git a/core/lib/tasks/migrations/assure_store_on_orders.rake b/core/lib/tasks/migrations/ensure_store_on_orders.rake similarity index 97% rename from core/lib/tasks/migrations/assure_store_on_orders.rake rename to core/lib/tasks/migrations/ensure_store_on_orders.rake index 767aefa4fd2..8ec7cd95158 100644 --- a/core/lib/tasks/migrations/assure_store_on_orders.rake +++ b/core/lib/tasks/migrations/ensure_store_on_orders.rake @@ -1,6 +1,6 @@ namespace :solidus do namespace :migrations do - namespace :assure_store_on_orders do + namespace :ensure_store_on_orders do desc "Makes sure every order in the system has a store attached" task up: :environment do diff --git a/core/lib/tasks/upgrade.rake b/core/lib/tasks/upgrade.rake index c3f68a43502..dbffab7624b 100644 --- a/core/lib/tasks/upgrade.rake +++ b/core/lib/tasks/upgrade.rake @@ -2,7 +2,7 @@ namespace :solidus do namespace :upgrade do desc "Upgrade Solidus to version 1.3" task one_point_three: [ - 'solidus:migrations:assure_store_on_orders:up', + 'solidus:migrations:ensure_store_on_orders:up', 'solidus:migrations:migrate_shipping_rate_taxes:up', 'solidus:migrations:create_vat_prices' ] do diff --git a/core/spec/lib/tasks/migrations/assure_store_on_orders_spec.rb b/core/spec/lib/tasks/migrations/ensure_store_on_orders_spec.rb similarity index 93% rename from core/spec/lib/tasks/migrations/assure_store_on_orders_spec.rb rename to core/spec/lib/tasks/migrations/ensure_store_on_orders_spec.rb index d639b2a05f3..b93f2412119 100644 --- a/core/spec/lib/tasks/migrations/assure_store_on_orders_spec.rb +++ b/core/spec/lib/tasks/migrations/ensure_store_on_orders_spec.rb @@ -1,11 +1,11 @@ require 'spec_helper' -describe 'solidus:migrations:assure_store_on_orders' do +describe 'solidus:migrations:ensure_store_on_orders' do describe 'up' do include_context( 'rake', - task_path: Spree::Core::Engine.root.join('lib/tasks/migrations/assure_store_on_orders.rake'), - task_name: 'solidus:migrations:assure_store_on_orders:up', + task_path: Spree::Core::Engine.root.join('lib/tasks/migrations/ensure_store_on_orders.rake'), + task_name: 'solidus:migrations:ensure_store_on_orders:up', ) context 'with no orders' do