From 918e583d0135d07fbd8850ecea28db108f33f32c Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 24 Feb 2023 16:28:58 +1100 Subject: [PATCH 1/4] Account for Rails 7 rounding in time spec Storing a timestamp to the database has less accuracy than a Ruby Time object. So `updated_at` changes after being written and loaded from the database. Rails 7 accounts for that by rounding it in the model already before it's written to the database. That made one spec fail. --- spec/models/spree/product_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/spree/product_spec.rb b/spec/models/spree/product_spec.rb index 65781c515b..ab8f3b7fcc 100644 --- a/spec/models/spree/product_spec.rb +++ b/spec/models/spree/product_spec.rb @@ -309,7 +309,7 @@ module Spree it "defaults available_on to now" do Timecop.freeze do product = Product.new - expect(product.available_on).to eq(Time.zone.now) + expect(product.available_on).to be_within(0.000001).of(Time.zone.now) end end From b6cccc2e1d1c789570e34c5a365609060766d88c Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 24 Feb 2023 17:04:23 +1100 Subject: [PATCH 2/4] Mark broken specs, possible broken caching I found this because Rails 7 converts timestamps to database precision straight away. While we may have some broken logic in the code, most of these cases may just be broken spec code. Watch this space. --- spec/models/enterprise_caching_spec.rb | 60 ++++++++++---------------- 1 file changed, 23 insertions(+), 37 deletions(-) diff --git a/spec/models/enterprise_caching_spec.rb b/spec/models/enterprise_caching_spec.rb index 28d1d951c8..0a83fefc93 100644 --- a/spec/models/enterprise_caching_spec.rb +++ b/spec/models/enterprise_caching_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' describe Enterprise do context "key-based caching invalidation" do describe "is touched when a(n)" do - let(:enterprise) { create(:distributor_enterprise, updated_at: Time.zone.now - 1.week) } + let(:enterprise) { create(:distributor_enterprise, updated_at: 1.week.ago) } let(:taxon) { create(:taxon) } let(:supplier2) { create(:supplier_enterprise) } @@ -20,32 +20,28 @@ describe Enterprise do enterprise.set_producer_property 'Biodynamic', 'ASDF 4321' end - it "touches enterprise when a classification on that product changes" do + pending "touches enterprise when a classification on that product changes" do expect { classification.save! - enterprise.reload - }.to change { enterprise.updated_at } + }.to change { enterprise.reload.updated_at } end - it "touches enterprise when a property on that product changes" do + pending "touches enterprise when a property on that product changes" do expect { property.save! - enterprise.reload - }.to change { enterprise.updated_at } + }.to change { enterprise.reload.updated_at } end - it "touches enterprise when a producer property on that product changes" do + pending "touches enterprise when a producer property on that product changes" do expect { producer_property.save! - enterprise.reload - }.to change { enterprise.updated_at } + }.to change { enterprise.reload.updated_at } end it "touches enterprise when the supplier of a product changes" do expect { product.update!(supplier: supplier2) - enterprise.reload - }.to change { enterprise.updated_at } + }.to change { enterprise.reload.updated_at } end end @@ -68,47 +64,41 @@ describe Enterprise do context "with an order cycle" do before { oc } - it "touches enterprise when a classification on that product changes" do + pending "touches enterprise when a classification on that product changes" do expect { classification.save! - enterprise.reload - }.to change { enterprise.updated_at } + }.to change { enterprise.reload.updated_at } end - it "touches enterprise when a property on that product changes" do + pending "touches enterprise when a property on that product changes" do expect { property.save! - enterprise.reload - }.to change { enterprise.updated_at } + }.to change { enterprise.reload.updated_at } end - it "touches enterprise when a producer property on that product changes" do + pending "touches enterprise when a producer property on that product changes" do expect { producer_property.save! - enterprise.reload - }.to change { enterprise.updated_at } + }.to change { enterprise.reload.updated_at } end it "touches enterprise when the supplier of a product changes" do expect { product.update!(supplier: supplier2) - enterprise.reload - }.to change { enterprise.updated_at } + }.to change { enterprise.reload.updated_at } end it "touches enterprise when a relevant exchange is updated" do expect { oc.exchanges.first.update!(updated_at: Time.zone.now) - enterprise.reload - }.to change { enterprise.updated_at } + }.to change { enterprise.reload.updated_at } end end it "touches enterprise when the product's variant is added to order cycle" do expect { oc - enterprise.reload - }.to change { enterprise.updated_at } + }.to change { enterprise.reload.updated_at } end end @@ -116,11 +106,10 @@ describe Enterprise do let(:child_enterprise) { create(:supplier_enterprise) } let!(:er) { create(:enterprise_relationship, parent: enterprise, child: child_enterprise) } - it "touches enterprise when enterprise relationship is updated" do + pending "touches enterprise when enterprise relationship is updated" do expect { er.save! - enterprise.reload - }.to change { enterprise.updated_at } + }.to change { enterprise.reload.updated_at } end end @@ -131,26 +120,23 @@ describe Enterprise do enterprise.shipping_methods << sm end - it "touches enterprise when distributor_shipping_method is updated" do + pending "touches enterprise when distributor_shipping_method is updated" do expect { enterprise.distributor_shipping_methods.first.save! - enterprise.reload - }.to change { enterprise.updated_at } + }.to change { enterprise.reload.updated_at } end it "touches enterprise when shipping method is updated" do expect { sm.save! - enterprise.reload - }.to change { enterprise.updated_at } + }.to change { enterprise.reload.updated_at } end end it "touches enterprise when address is updated" do expect { enterprise.address.save! - enterprise.reload - }.to change { enterprise.updated_at } + }.to change { enterprise.reload.updated_at } end end end From 103bc50bdca9dfd487d7b7905ff0a5fe506eeaf9 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 3 Mar 2023 12:36:24 +1100 Subject: [PATCH 3/4] Make spec robust on very fast computers I didn't observe it but if the spec code would run within the same millisecond then we wouldn't be able to observe a change to `updated_at`. Time travel solves this potential problem. --- spec/models/enterprise_caching_spec.rb | 34 ++++++++++++++------------ 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/spec/models/enterprise_caching_spec.rb b/spec/models/enterprise_caching_spec.rb index 0a83fefc93..b7d110a1d9 100644 --- a/spec/models/enterprise_caching_spec.rb +++ b/spec/models/enterprise_caching_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' describe Enterprise do context "key-based caching invalidation" do describe "is touched when a(n)" do - let(:enterprise) { create(:distributor_enterprise, updated_at: 1.week.ago) } + let(:enterprise) { create(:distributor_enterprise) } let(:taxon) { create(:taxon) } let(:supplier2) { create(:supplier_enterprise) } @@ -22,25 +22,25 @@ describe Enterprise do pending "touches enterprise when a classification on that product changes" do expect { - classification.save! + later { classification.save! } }.to change { enterprise.reload.updated_at } end pending "touches enterprise when a property on that product changes" do expect { - property.save! + later { property.save! } }.to change { enterprise.reload.updated_at } end pending "touches enterprise when a producer property on that product changes" do expect { - producer_property.save! + later { producer_property.save! } }.to change { enterprise.reload.updated_at } end it "touches enterprise when the supplier of a product changes" do expect { - product.update!(supplier: supplier2) + later { product.update!(supplier: supplier2) } }.to change { enterprise.reload.updated_at } end end @@ -66,38 +66,38 @@ describe Enterprise do pending "touches enterprise when a classification on that product changes" do expect { - classification.save! + later { classification.save! } }.to change { enterprise.reload.updated_at } end pending "touches enterprise when a property on that product changes" do expect { - property.save! + later { property.save! } }.to change { enterprise.reload.updated_at } end pending "touches enterprise when a producer property on that product changes" do expect { - producer_property.save! + later { producer_property.save! } }.to change { enterprise.reload.updated_at } end it "touches enterprise when the supplier of a product changes" do expect { - product.update!(supplier: supplier2) + later { product.update!(supplier: supplier2) } }.to change { enterprise.reload.updated_at } end it "touches enterprise when a relevant exchange is updated" do expect { - oc.exchanges.first.update!(updated_at: Time.zone.now) + later { oc.exchanges.first.update!(updated_at: Time.zone.now) } }.to change { enterprise.reload.updated_at } end end it "touches enterprise when the product's variant is added to order cycle" do expect { - oc + later { oc } }.to change { enterprise.reload.updated_at } end end @@ -108,7 +108,7 @@ describe Enterprise do pending "touches enterprise when enterprise relationship is updated" do expect { - er.save! + later { er.save! } }.to change { enterprise.reload.updated_at } end end @@ -122,22 +122,26 @@ describe Enterprise do pending "touches enterprise when distributor_shipping_method is updated" do expect { - enterprise.distributor_shipping_methods.first.save! + later { enterprise.distributor_shipping_methods.first.save! } }.to change { enterprise.reload.updated_at } end it "touches enterprise when shipping method is updated" do expect { - sm.save! + later { sm.save! } }.to change { enterprise.reload.updated_at } end end it "touches enterprise when address is updated" do expect { - enterprise.address.save! + later { enterprise.address.save! } }.to change { enterprise.reload.updated_at } end end end + + def later(&block) + Timecop.travel(1.day.from_now, &block) + end end From 12906d1e13cf72716267aea7c1fa5575aa3672c2 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 3 Mar 2023 12:43:01 +1100 Subject: [PATCH 4/4] Explicitely touch instead of noop save When calling `save!` without changing any attributes then Rails doesn't always touch other records because nothing changed. So I changed the spec to `touch` explicitely and it turns out that everything passes. Tada, our code seems correct and it was only the spec which seemed broken in Rails 7. --- spec/models/enterprise_caching_spec.rb | 32 +++++++++++++------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/spec/models/enterprise_caching_spec.rb b/spec/models/enterprise_caching_spec.rb index b7d110a1d9..eed464940d 100644 --- a/spec/models/enterprise_caching_spec.rb +++ b/spec/models/enterprise_caching_spec.rb @@ -20,21 +20,21 @@ describe Enterprise do enterprise.set_producer_property 'Biodynamic', 'ASDF 4321' end - pending "touches enterprise when a classification on that product changes" do + it "touches enterprise when a classification on that product changes" do expect { - later { classification.save! } + later { classification.touch } }.to change { enterprise.reload.updated_at } end - pending "touches enterprise when a property on that product changes" do + it "touches enterprise when a property on that product changes" do expect { - later { property.save! } + later { property.touch } }.to change { enterprise.reload.updated_at } end - pending "touches enterprise when a producer property on that product changes" do + it "touches enterprise when a producer property on that product changes" do expect { - later { producer_property.save! } + later { producer_property.touch } }.to change { enterprise.reload.updated_at } end @@ -64,21 +64,21 @@ describe Enterprise do context "with an order cycle" do before { oc } - pending "touches enterprise when a classification on that product changes" do + it "touches enterprise when a classification on that product changes" do expect { - later { classification.save! } + later { classification.touch } }.to change { enterprise.reload.updated_at } end - pending "touches enterprise when a property on that product changes" do + it "touches enterprise when a property on that product changes" do expect { - later { property.save! } + later { property.touch } }.to change { enterprise.reload.updated_at } end - pending "touches enterprise when a producer property on that product changes" do + it "touches enterprise when a producer property on that product changes" do expect { - later { producer_property.save! } + later { producer_property.touch } }.to change { enterprise.reload.updated_at } end @@ -106,9 +106,9 @@ describe Enterprise do let(:child_enterprise) { create(:supplier_enterprise) } let!(:er) { create(:enterprise_relationship, parent: enterprise, child: child_enterprise) } - pending "touches enterprise when enterprise relationship is updated" do + it "touches enterprise when enterprise relationship is updated" do expect { - later { er.save! } + later { er.touch } }.to change { enterprise.reload.updated_at } end end @@ -120,9 +120,9 @@ describe Enterprise do enterprise.shipping_methods << sm end - pending "touches enterprise when distributor_shipping_method is updated" do + it "touches enterprise when distributor_shipping_method is updated" do expect { - later { enterprise.distributor_shipping_methods.first.save! } + later { enterprise.distributor_shipping_methods.first.touch } }.to change { enterprise.reload.updated_at } end