From bd1b8b48e0e6532ba63f0290e4d7f02ab80510a5 Mon Sep 17 00:00:00 2001
From: Priyank Gupta <priyank.gupta@vinsol.com>
Date: Tue, 23 Feb 2016 13:14:55 +0530
Subject: [PATCH] fix intermittent spec failure for order details

use js: true in backend feature specs where required

fix spec for order details spec

fix order details spec, reset config track inventory levels in after block

test commit to fix order details spec

refactor for wait_for_ajax

change targeted product object in order details failure spec

wait for ajax and reload order in order details spec

modify failing test suit of order details spec

refactor prototypes_spec feature

extract wait_for_condition in capybara_ext, change default time of wait_for_ajax to Capybara.default_max_wait_time

change Capybara.max_wait_time_out to 10

hound ci fixes

refactor of refund reason and authorization, change timout to 15

fix return authorisation reasons and refund reasons feature spec, change timeout from 15 to 10

Conflicts:
	backend/spec/features/admin/refund_reasons/refund_reasons_spec.rb
	backend/spec/features/admin/return_authorization_reasons/return_authorization_reasons_spec.rb
---
 .../configuration/general_settings_spec.rb    |   4 +-
 .../admin/configuration/roles_spec.rb         |   2 +-
 .../configuration/stock_locations_spec.rb     |   6 +-
 .../configuration/tax_categories_spec.rb      |   4 +-
 .../admin/configuration/tax_rates_spec.rb     |   2 +-
 .../admin/configuration/zones_spec.rb         |   2 +-
 .../admin/orders/customer_details_spec.rb     |  15 --
 .../admin/orders/order_details_spec.rb        | 141 +++++++++++-------
 .../features/admin/orders/payments_spec.rb    |  10 +-
 .../admin/products/edit/images_spec.rb        |   4 +-
 .../admin/products/properties_spec.rb         |   9 +-
 .../admin/products/prototypes_spec.rb         |   8 +-
 .../admin/products/stock_management_spec.rb   |  14 +-
 .../features/admin/products/variant_spec.rb   |   4 +-
 backend/spec/spec_helper.rb                   |   2 +-
 .../lib/spree/testing_support/capybara_ext.rb |  20 ++-
 16 files changed, 144 insertions(+), 103 deletions(-)

diff --git a/backend/spec/features/admin/configuration/general_settings_spec.rb b/backend/spec/features/admin/configuration/general_settings_spec.rb
index 014b2b22077..b7c2ab11d80 100644
--- a/backend/spec/features/admin/configuration/general_settings_spec.rb
+++ b/backend/spec/features/admin/configuration/general_settings_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe "General Settings", type: :feature, js: true do
+describe "General Settings", type: :feature do
   stub_authorization!
 
   before(:each) do
@@ -30,7 +30,7 @@
     end
   end
 
-  context "clearing the cache" do
+  context "clearing the cache", js: true do
     it "should clear the cache" do
       expect(page).to_not have_content(Spree.t(:clear_cache_ok))
       expect(page).to have_content(Spree.t(:clear_cache_warning))
diff --git a/backend/spec/features/admin/configuration/roles_spec.rb b/backend/spec/features/admin/configuration/roles_spec.rb
index a44f5d65cd8..be208ee615b 100644
--- a/backend/spec/features/admin/configuration/roles_spec.rb
+++ b/backend/spec/features/admin/configuration/roles_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe "Roles", type: :feature, js: true do
+describe "Roles", type: :feature do
   stub_authorization!
 
   before(:each) do
diff --git a/backend/spec/features/admin/configuration/stock_locations_spec.rb b/backend/spec/features/admin/configuration/stock_locations_spec.rb
index 8bff68ee8a9..5b0f59eedd8 100644
--- a/backend/spec/features/admin/configuration/stock_locations_spec.rb
+++ b/backend/spec/features/admin/configuration/stock_locations_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe "Stock Locations", type: :feature, js: true do
+describe "Stock Locations", type: :feature do
   stub_authorization!
 
   before(:each) do
@@ -18,7 +18,7 @@
     expect(page).to have_content("London")
   end
 
-  it "can delete an existing stock location" do
+  it "can delete an existing stock location", js: true do
     location = create(:stock_location)
     visit current_path
 
@@ -32,7 +32,7 @@
     expect(page).to have_content("No Stock Locations found")
   end
 
-  it "can update an existing stock location" do
+  it "can update an existing stock location", js: true do
     create(:stock_location)
     visit current_path
 
diff --git a/backend/spec/features/admin/configuration/tax_categories_spec.rb b/backend/spec/features/admin/configuration/tax_categories_spec.rb
index 871cd608037..a7c8d0eccb5 100644
--- a/backend/spec/features/admin/configuration/tax_categories_spec.rb
+++ b/backend/spec/features/admin/configuration/tax_categories_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe "Tax Categories", type: :feature, js: true do
+describe "Tax Categories", type: :feature do
   stub_authorization!
 
   before(:each) do
@@ -43,7 +43,7 @@
   end
 
   context "admin editing a tax category" do
-    it "should be able to update an existing tax category" do
+    it "should be able to update an existing tax category", js: true do
       create(:tax_category)
       click_link "Tax Categories"
       within_row(1) { click_icon :edit }
diff --git a/backend/spec/features/admin/configuration/tax_rates_spec.rb b/backend/spec/features/admin/configuration/tax_rates_spec.rb
index 6398363e0e3..665e7d06ca2 100644
--- a/backend/spec/features/admin/configuration/tax_rates_spec.rb
+++ b/backend/spec/features/admin/configuration/tax_rates_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe "Tax Rates", type: :feature, js: true do
+describe "Tax Rates", type: :feature do
   stub_authorization!
 
   let!(:tax_rate) { create(:tax_rate, calculator: stub_model(Spree::Calculator)) }
diff --git a/backend/spec/features/admin/configuration/zones_spec.rb b/backend/spec/features/admin/configuration/zones_spec.rb
index 9eb61aa91f5..836fb6adf7a 100644
--- a/backend/spec/features/admin/configuration/zones_spec.rb
+++ b/backend/spec/features/admin/configuration/zones_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe "Zones", type: :feature, js: true do
+describe "Zones", type: :feature do
   stub_authorization!
 
   before(:each) do
diff --git a/backend/spec/features/admin/orders/customer_details_spec.rb b/backend/spec/features/admin/orders/customer_details_spec.rb
index 4f5013c209d..1b9ea82a8a2 100644
--- a/backend/spec/features/admin/orders/customer_details_spec.rb
+++ b/backend/spec/features/admin/orders/customer_details_spec.rb
@@ -15,21 +15,6 @@
 
   let!(:user) { create(:user, email: 'foobar@example.com', ship_address: ship_address, bill_address: bill_address) }
 
-  # "Intelligiently" wait on condition
-  #
-  # Much better than a random sleep "here and there"
-  # it will not cause any delay in case the condition is fullfilled on first cycle.
-  def wait_for_condition
-    time = Capybara.default_max_wait_time
-    step = 0.1
-    while time > 0
-      return if yield
-      sleep(step)
-      time -= 0.1
-    end
-    fail "Could not achieve condition within #{Capybara.default_max_wait_time} seconds."
-  end
-
   # Value attribute is dynamically set via JS, so not observable via a CSS/XPath selector
   # As the browser might take time to make the values visible in the dom we need to
   # "intelligiently" wait for that event o prevent a race.
diff --git a/backend/spec/features/admin/orders/order_details_spec.rb b/backend/spec/features/admin/orders/order_details_spec.rb
index f5074774f2b..55e8430888c 100644
--- a/backend/spec/features/admin/orders/order_details_spec.rb
+++ b/backend/spec/features/admin/orders/order_details_spec.rb
@@ -4,13 +4,12 @@
 describe "Order Details", type: :feature, js: true do
   let!(:stock_location) { create(:stock_location_with_items) }
   let!(:product) { create(:product, name: 'spree t-shirt', price: 20.00) }
-  let!(:tote) { create(:product, name: "Tote", price: 15.00) }
   let(:order) { create(:order, state: 'complete', completed_at: "2011-02-01 12:36:15", number: "R100") }
   let(:state) { create(:state) }
   let!(:shipping_method) { create(:shipping_method, name: "Default") }
 
   before do
-    order.shipments.create(stock_location_id: stock_location.id)
+    order.shipments.create!(stock_location_id: stock_location.id)
     order.contents.add(product.master, 2)
   end
 
@@ -120,7 +119,7 @@
         expect(page).to have_content("Backdoor")
       end
 
-      it "will show the variant sku" do
+      it "will show the variant sku", js: false do
         order = create(:completed_order_with_totals)
         visit spree.edit_admin_order_path(order)
         sku = order.line_items.first.variant.sku
@@ -128,51 +127,64 @@
       end
 
       context "with special_instructions present" do
-        let(:order) { create(:order, state: 'complete', completed_at: "2011-02-01 12:36:15", number: "R100", special_instructions: "Very special instructions here") }
-        it "will show the special_instructions" do
+        before(:each) do
+          order.update_column(:special_instructions, "Very special instructions here")
+        end
+
+        it "will show the special_instructions", js: false do
           visit spree.edit_admin_order_path(order)
           expect(page).to have_content("Very special instructions here")
         end
       end
 
-      context "variant doesn't track inventory" do
-        before do
-          tote.master.update_column :track_inventory, false
-          # make sure there's no stock level for any item
-          tote.master.stock_items.update_all count_on_hand: 0, backorderable: false
-        end
+      context 'when not tracking inventory' do
+        let(:tote) { create(:product, name: "Tote", price: 15.00) }
 
-        it "adds variant to order just fine" do
-          select2_search tote.name, from: Spree.t(:name_or_sku)
-          within("table.stock-levels") do
-            fill_in "variant_quantity", with: 1
-            click_icon :add
+        context "variant doesn't track inventory" do
+          before do
+            tote.master.update_column :track_inventory, false
+            # make sure there's no stock level for any item
+            tote.master.stock_items.update_all count_on_hand: 0, backorderable: false
           end
 
-          within(".line-items") do
-            expect(page).to have_content(tote.name)
-          end
-        end
-      end
+          it "adds variant to order just fine" do
+            select2_search tote.name, from: Spree.t(:name_or_sku)
+            within("table.stock-levels") do
+              fill_in "variant_quantity", with: 1
+              click_icon :add
+            end
 
-      context "site doesn't track inventory" do
-        before do
-          Spree::Config.set track_inventory_levels: false
-          product.master.update_column(:track_inventory, true)
-          product.master.stock_items.first.update_column(:backorderable, true)
-          product.master.stock_items.first.update_column(:count_on_hand, 0)
+            wait_for_ajax
+
+            within(".line-items") do
+              expect(page).to have_content(tote.name)
+            end
+          end
         end
 
-        it "adds variant to order just fine" do
-          select2_search product.name, from: Spree.t(:name_or_sku)
-          within("table.stock-levels") do
-            fill_in "variant_quantity", with: 1
-            click_icon :add
+        context "site doesn't track inventory" do
+          before do
+            Spree::Config[:track_inventory_levels] = false
+            tote.master.update_column(:track_inventory, true)
+            # make sure there's no stock level for any item
+            tote.master.stock_items.update_all count_on_hand: 0, backorderable: true
           end
 
-          within(".line-items") do
-            expect(page).to have_content(product.name)
+          it "adds variant to order just fine" do
+            select2_search tote.name, from: Spree.t(:name_or_sku)
+            within("table.stock-levels") do
+              fill_in "variant_quantity", with: 1
+              click_icon :add
+            end
+
+            wait_for_ajax
+
+            within(".line-items") do
+              expect(page).to have_content(tote.name)
+            end
           end
+
+          after { Spree::Config[:track_inventory_levels] = true }
         end
       end
 
@@ -301,7 +313,7 @@
 
           context 'A shipment has shipped' do
 
-            it 'should not show or let me back to the cart page, nor show the shipment edit buttons' do
+            it 'should not show or let me back to the cart page, nor show the shipment edit buttons', js: false do
               order = create(:order, state: 'payment')
               order.shipments.create!(stock_location_id: stock_location.id, state: 'shipped')
 
@@ -376,27 +388,54 @@
           end
         end
 
-        context "site doesn't track inventory" do
-          before do
-            Spree::Config.set track_inventory_levels: false
-            product.master.update_column(:track_inventory, true)
-            product.master.stock_items.first.update_column(:backorderable, true)
-            product.master.stock_items.first.update_column(:count_on_hand, 0)
+        context 'when not tracking inventory' do
+          let(:tote) { create(:product, name: "Tote", price: 15.00) }
+
+          context "variant doesn't track inventory" do
+            before do
+              tote.master.update_column :track_inventory, false
+              # make sure there's no stock level for any item
+              tote.master.stock_items.update_all count_on_hand: 0, backorderable: false
+            end
+
+            it "adds variant to order just fine" do
+              select2_search tote.name, from: Spree.t(:name_or_sku)
+              within("table.stock-levels") do
+                fill_in "stock_item_quantity", with: 1
+                click_icon :add
+              end
+
+              wait_for_ajax
+
+              within("[data-hook=admin_order_form_fields]") do
+                expect(page).to have_content(tote.name)
+              end
+            end
           end
 
-          it "adds variant to order just fine" do
-            select2_search product.name, from: Spree.t(:name_or_sku)
-            within("table.stock-levels") do
-              fill_in "stock_item_quantity", with: 1
-              click_icon :add
+          context "site doesn't track inventory" do
+            before do
+              Spree::Config[:track_inventory_levels] = false
+              tote.master.update_column(:track_inventory, true)
+              # make sure there's no stock level for any item
+              tote.master.stock_items.update_all count_on_hand: 0, backorderable: true
             end
 
-            wait_for_ajax
-            order.reload
+            it "adds variant to order just fine" do
+              select2_search tote.name, from: Spree.t(:name_or_sku)
+              within("table.stock-levels") do
+                fill_in "stock_item_quantity", with: 1
+                click_icon :add
+              end
 
-            within(".stock-contents") do
-              expect(page).to have_content(product.name)
+              wait_for_ajax
+
+              within("[data-hook=admin_order_form_fields]") do
+                expect(page).to have_content(tote.name)
+              end
             end
+
+            after { Spree::Config[:track_inventory_levels] = true }
           end
         end
 
@@ -571,7 +610,7 @@
                                    and_return(Spree.user_class.new)
     end
 
-    it 'should not display order tabs or edit buttons without ability' do
+    it 'should not display order tabs or edit buttons without ability', js: false do
       visit spree.edit_admin_order_path(order)
 
       # Order Form
diff --git a/backend/spec/features/admin/orders/payments_spec.rb b/backend/spec/features/admin/orders/payments_spec.rb
index bedfeb2f5df..9d7dda3863d 100644
--- a/backend/spec/features/admin/orders/payments_spec.rb
+++ b/backend/spec/features/admin/orders/payments_spec.rb
@@ -62,7 +62,7 @@ def refresh_page
       end
     end
 
-    it 'lists and create payments for an order', js: true do
+    it 'lists and create payments for an order' do
       within_row(1) do
         expect(column_text(3)).to eq('$150.00')
         expect(column_text(4)).to eq('Credit Card')
@@ -91,7 +91,7 @@ def refresh_page
     end
 
     # Regression test for #1269
-    it 'cannot create a payment for an order with no payment methods' do
+    it 'cannot create a payment for an order with no payment methods', js: false do
       Spree::PaymentMethod.delete_all
       order.payments.delete_all
 
@@ -101,7 +101,7 @@ def refresh_page
     end
 
     %w[checkout pending].each do |state|
-      context "payment is #{state.inspect}", js: true do
+      context "payment is #{state.inspect}" do
         let(:state) { state }
 
         it 'allows the amount to be edited by clicking on the edit button then saving' do
@@ -152,7 +152,7 @@ def refresh_page
       end
     end
 
-    context 'payment is completed' do
+    context 'payment is completed', js: false do
       let(:state) { 'completed' }
 
       it 'does not allow the amount to be edited' do
@@ -204,7 +204,7 @@ def refresh_page
 
       before { visit spree.admin_order_payments_path(order) }
 
-      it "is able to reuse customer payment source" do
+      it "is able to reuse customer payment source", js: false do
         expect(find("#card_#{cc.id}")).to be_checked
         click_button "Continue"
         expect(page).to have_content("Payment has been successfully created!")
diff --git a/backend/spec/features/admin/products/edit/images_spec.rb b/backend/spec/features/admin/products/edit/images_spec.rb
index 799c9a04b00..6807ddf0f07 100644
--- a/backend/spec/features/admin/products/edit/images_spec.rb
+++ b/backend/spec/features/admin/products/edit/images_spec.rb
@@ -40,7 +40,7 @@
   end
 
   # Regression test for #2228
-  it "should see variant images" do
+  it "should see variant images", js: false do
     variant = create(:variant)
     variant.images.create!(attachment: File.open(file_path))
     visit spree.admin_product_images_path(variant.product)
@@ -64,7 +64,7 @@
     end
   end
 
-  it "should not see variant column when product has no variants" do
+  it "should not see variant column when product has no variants", js: false do
     product = create(:product)
     product.images.create!(attachment: File.open(file_path))
     visit spree.admin_product_images_path(product)
diff --git a/backend/spec/features/admin/products/properties_spec.rb b/backend/spec/features/admin/products/properties_spec.rb
index e1403891645..1c33f5cc5bb 100644
--- a/backend/spec/features/admin/products/properties_spec.rb
+++ b/backend/spec/features/admin/products/properties_spec.rb
@@ -42,7 +42,7 @@
   end
 
   context "creating a property" do
-    it "should allow an admin to create a new product property", js: true do
+    it "should allow an admin to create a new product property" do
       click_link "Products"
       click_link "Properties"
       click_link "new_property_link"
@@ -126,9 +126,10 @@ def fill_in_property
     end
 
     def delete_product_property
-      page.evaluate_script('window.confirm = function() { return true; }')
-      click_icon :delete
-      wait_for_ajax # delete action must finish before reloading
+      accept_alert do
+        click_icon :delete
+        wait_for_ajax # delete action must finish before reloading
+      end
     end
 
     def check_property_row_count(expected_row_count)
diff --git a/backend/spec/features/admin/products/prototypes_spec.rb b/backend/spec/features/admin/products/prototypes_spec.rb
index f08b8cb964e..914d7c24248 100644
--- a/backend/spec/features/admin/products/prototypes_spec.rb
+++ b/backend/spec/features/admin/products/prototypes_spec.rb
@@ -93,7 +93,7 @@
     end
   end
 
-  it 'should be deletable', js: true do
+  it 'should be deletable' do
     shirt_prototype = create(:prototype, name: "Shirt", properties: [])
     shirt_prototype.taxons << create(:taxon)
 
@@ -105,8 +105,8 @@
       page.find('.delete-resource').click
     end
 
-    page.evaluate_script('window.confirm = function() { return true; }')
-
-    expect(page).to have_content("Prototype \"#{shirt_prototype.name}\" has been successfully removed!")
+    accept_alert do
+      expect(page).to have_content("Prototype \"#{shirt_prototype.name}\" has been successfully removed!")
+    end
   end
 end
diff --git a/backend/spec/features/admin/products/stock_management_spec.rb b/backend/spec/features/admin/products/stock_management_spec.rb
index 4cf4caaaa81..db74358a898 100644
--- a/backend/spec/features/admin/products/stock_management_spec.rb
+++ b/backend/spec/features/admin/products/stock_management_spec.rb
@@ -22,7 +22,7 @@
         wait_for_ajax
       end
 
-      it "persists the value when page reload", js: true do
+      it "persists the value when page reload" do
         visit current_path
         expect(backorderable).not_to be_checked
       end
@@ -37,7 +37,7 @@
         wait_for_ajax
       end
 
-      it "persists the value when page reloaded", js: true do
+      it "persists the value when page reloaded" do
         visit current_path
         expect(track_inventory).not_to be_checked
       end
@@ -47,7 +47,7 @@
     # The regression was that unchecking the last checkbox caused a redirect
     # to happen. By ensuring that we're still on an /admin/products URL, we
     # assert that the redirect is *not* happening.
-    it "can toggle backorderable for the second variant stock item", js: true do
+    it "can toggle backorderable for the second variant stock item" do
       new_location = create(:stock_location, name: "Another Location")
       visit current_url
 
@@ -58,7 +58,7 @@
       expect(page.current_url).to include("/admin/products")
     end
 
-    it "can create a new stock movement", js: true do
+    it "can create a new stock movement" do
       fill_in "stock_movement_quantity", with: 5
       select2 "default", from: "Stock Location"
       click_button "Add Stock"
@@ -70,7 +70,7 @@
       end
     end
 
-    it "can create a new negative stock movement", js: true do
+    it "can create a new negative stock movement" do
       fill_in "stock_movement_quantity", with: -5
       select2 "default", from: "Stock Location"
       click_button "Add Stock"
@@ -89,7 +89,7 @@
         visit current_url
       end
 
-      it "can create a new stock movement for the specified variant", js: true do
+      it "can create a new stock movement for the specified variant" do
         fill_in "stock_movement_quantity", with: 10
         select2 "SPREEC", from: "Variant"
         click_button "Add Stock"
@@ -114,7 +114,7 @@
         visit spree.stock_admin_product_path(@product)
       end
 
-      it "redirects to stock locations page" do
+      it "redirects to stock locations page", js: false do
         expect(page).to have_content(Spree.t(:stock_management_requires_a_stock_location))
         expect(page.current_url).to include("admin/stock_locations")
       end
diff --git a/backend/spec/features/admin/products/variant_spec.rb b/backend/spec/features/admin/products/variant_spec.rb
index 845a305d952..988aaf8511e 100644
--- a/backend/spec/features/admin/products/variant_spec.rb
+++ b/backend/spec/features/admin/products/variant_spec.rb
@@ -1,13 +1,13 @@
 # encoding: utf-8
 require 'spec_helper'
 
-describe "Variants", type: :feature, js: true do
+describe "Variants", type: :feature do
   stub_authorization!
 
   let(:product) { create(:product_with_option_types, price: "1.99", cost_price: "1.00", weight: "2.5", height: "3.0", width: "1.0", depth: "1.5") }
 
   context "creating a new variant" do
-    it "should allow an admin to create a new variant" do
+    it "should allow an admin to create a new variant", js: true do
       product.options.each do |option|
         create(:option_value, option_type: option.option_type)
       end
diff --git a/backend/spec/spec_helper.rb b/backend/spec/spec_helper.rb
index 07d00c013a7..ba3f5c386c2 100644
--- a/backend/spec/spec_helper.rb
+++ b/backend/spec/spec_helper.rb
@@ -50,7 +50,7 @@
 Capybara.javascript_driver = :poltergeist
 
 # Set timeout to something high enough to allow CI to pass
-Capybara.default_max_wait_time = 20
+Capybara.default_max_wait_time = 10
 
 RSpec.configure do |config|
   config.color = true
diff --git a/core/lib/spree/testing_support/capybara_ext.rb b/core/lib/spree/testing_support/capybara_ext.rb
index 606db19dd26..8ad415480f3 100644
--- a/core/lib/spree/testing_support/capybara_ext.rb
+++ b/core/lib/spree/testing_support/capybara_ext.rb
@@ -101,12 +101,28 @@ def find_label(text)
     first(:xpath, "//label[text()[contains(.,'#{text}')]]")
   end
 
-  def wait_for_ajax
+  # arg delay in seconds
+  def wait_for_ajax(delay = Capybara.default_max_wait_time)
     counter = 0
+    delay_threshold = delay * 10
     while page.evaluate_script("typeof($) === 'undefined' || $.active > 0")
       counter += 1
       sleep(0.1)
-      raise "AJAX request took longer than 5 seconds." if counter >= 50
+      raise "AJAX request took longer than #{delay} seconds." if counter >= delay_threshold
+    end
+  end
+
+  # "Intelligiently" wait on condition
+  #
+  # Much better than a random sleep "here and there"
+  # it will not cause any delay in case the condition is fullfilled on first cycle.
+  def wait_for_condition(delay = Capybara.default_max_wait_time)
+    counter = 0
+    delay_threshold = delay * 10
+    while !yield
+      counter += 1
+      sleep(0.1)
+      raise "Could not achieve condition within #{delay} seconds." if counter >= delay_threshold
     end
   end