Prepare caching spec for Rails 7 #10487

mkllnk · 2023-02-28T05:10:35Z

What? Why?

I found some specs which seemed to pass even though the tested code failed. So I tried to make the specs fail correctly and marked them as pending. Then I found that the spec code was faulty and improved it. Now we have a spec that, I think, is correct and also passes with Rails 7.

What should we test?

Specs only.

Release notes

Changelog Category: Technical changes

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

filipefurtad0 · 2023-03-01T19:11:00Z

spec/models/enterprise_caching_spec.rb


          it "touches enterprise when a classification on that product changes" do
+            pending "product touches distributor on change"
+
            expect {
              classification.save!
              enterprise.reload


Ok, so if we don't reload on line 73 then the spec passes because the OC touches the enterprise. Do I understand the issue correctly?

At the moment, classification.save! does not touch the distributor. That's why the spec is pending.

The OC may touch the enterprise but, more importantly, the enterprise is created when we create the enterprise. So similar to before, we need to reload after setup and before executing the code that's supposed to touch.

filipefurtad0 · 2023-03-01T19:12:09Z

spec/models/enterprise_caching_spec.rb

@@ -5,7 +5,8 @@
 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) }
+      # Reloading changes timestamps to the precision of the database.
+      let(:enterprise) { create(:distributor_enterprise, updated_at: 1.week.ago).reload }


Do we need to reload the enterprise upon creation, and before any action "touched" it?

Yes, otherwise our reload after the expected touch changes the timestamp regardless, touched or not.

filipefurtad0

Ok, we expect that changing the:

classification
product property
producer property
enterprise relationship
changes enterprise.updated_at (aka. touching).

Thanks for pinging. Surely worth testing manually. It should be even possible to verify these changes by logging into the server.

mkllnk · 2023-03-02T23:57:09Z

spec/models/enterprise_caching_spec.rb

@@ -5,7 +5,8 @@
 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) }
+      # Reloading changes timestamps to the precision of the database.
+      let(:enterprise) { create(:distributor_enterprise, updated_at: 1.week.ago).reload }


Yes, otherwise our reload after the expected touch changes the timestamp regardless, touched or not.

mkllnk · 2023-03-02T23:59:22Z

spec/models/enterprise_caching_spec.rb


          it "touches enterprise when a classification on that product changes" do
+            pending "product touches distributor on change"
+
            expect {
              classification.save!
              enterprise.reload


At the moment, classification.save! does not touch the distributor. That's why the spec is pending.

The OC may touch the enterprise but, more importantly, the enterprise is created when we create the enterprise. So similar to before, we need to reload after setup and before executing the code that's supposed to touch.

mkllnk · 2023-03-03T00:02:27Z

spec/models/enterprise_caching_spec.rb


          it "touches enterprise when a classification on that product changes" do
+            pending "product touches distributor on change"
+
            expect {
              classification.save!
              enterprise.reload
            }.to change { enterprise.updated_at }


Alternatively, we could reload the enterprise before and after:

Suggested change

}.to change { enterprise.updated_at }

}.to change { enterprise.reload.updated_at }

I think that this is clearer. That block is executed before and after and then compared. Then it doesn't matter which setup code needed reloading.

My current solution came from the rails7 branch where we don't actually need to reload after create because Rails adjusts the precision of timestamps on creation already. But we still have the potential problem of other setup code touching the enterprise.

So big thank you for your feedback here. I will revise my pull request with this code instead. It will be much easier to understand.

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.

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.

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.

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.

jibees

Congrats! 🎉
Are we ready to upgrade to Rails7 ? ;) 👏

mkllnk · 2023-03-05T23:45:06Z

Are we ready to upgrade to Rails7 ?

No, we still have some failing specs:

Upgrade to Rails 7.0 #10440

dacook

Good fix!

dacook · 2023-03-08T00:24:47Z

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)


I'm just wondering, would be_within(1) be good enough?

Hm, I'm using the full precision of the timestamp but you are probably right, from a user perspective, "now" just needs the accuracy to the second.

filipefurtad0 · 2023-03-09T14:29:54Z

Hey @mkllnk ,

I've tested the scenarios:

changing the product category (which refers to taxon/classification, right?) -> it indeed touches the enterprise

producer property

enterprise relationship

I'm not entirely sure about the difference between classification vs. product category taxon. Other than that, I can confirm that making these changes indeed touches the respective enterprise 👍

mkllnk · 2023-03-09T23:02:55Z

Thank you for thorough testing @filipefurtad0!

mkllnk self-assigned this Feb 28, 2023

mkllnk requested a review from filipefurtad0 February 28, 2023 05:10

filipefurtad0 force-pushed the time-spec branch from 057efcf to 467bc32 Compare March 1, 2023 18:38

filipefurtad0 reviewed Mar 1, 2023

View reviewed changes

mkllnk commented Mar 3, 2023

View reviewed changes

mkllnk added 2 commits March 3, 2023 12:34

Mark broken specs, possible broken caching

b6cccc2

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.

mkllnk force-pushed the time-spec branch from 467bc32 to dd655fa Compare March 3, 2023 01:47

mkllnk marked this pull request as ready for review March 3, 2023 01:47

mkllnk changed the title ~~Spec broken caching~~ Prepare caching spec for Rails 7 Mar 3, 2023

mkllnk added 2 commits March 3, 2023 12:51

Make spec robust on very fast computers

103bc50

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.

mkllnk force-pushed the time-spec branch from dd655fa to 12906d1 Compare March 3, 2023 01:51

jibees approved these changes Mar 3, 2023

View reviewed changes

dacook approved these changes Mar 8, 2023

View reviewed changes

mkllnk merged commit df82836 into openfoodfoundation:master Mar 8, 2023

mkllnk deleted the time-spec branch March 8, 2023 00:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prepare caching spec for Rails 7 #10487

Prepare caching spec for Rails 7 #10487

mkllnk commented Feb 28, 2023 •

edited

Loading

filipefurtad0 Mar 1, 2023

mkllnk Mar 2, 2023

filipefurtad0 Mar 1, 2023 •

edited

Loading

mkllnk Mar 2, 2023

filipefurtad0 left a comment

mkllnk Mar 2, 2023

mkllnk Mar 2, 2023

mkllnk Mar 3, 2023

jibees left a comment

mkllnk commented Mar 5, 2023

dacook left a comment

dacook Mar 8, 2023

mkllnk Mar 8, 2023

filipefurtad0 commented Mar 9, 2023

mkllnk commented Mar 9, 2023

	}.to change { enterprise.updated_at }
	}.to change { enterprise.reload.updated_at }

Prepare caching spec for Rails 7 #10487

Prepare caching spec for Rails 7 #10487

Conversation

mkllnk commented Feb 28, 2023 • edited Loading

What? Why?

What should we test?

Release notes

Dependencies

Documentation updates

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipefurtad0 Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipefurtad0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jibees left a comment

Choose a reason for hiding this comment

mkllnk commented Mar 5, 2023

dacook left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipefurtad0 commented Mar 9, 2023

mkllnk commented Mar 9, 2023

mkllnk commented Feb 28, 2023 •

edited

Loading

filipefurtad0 Mar 1, 2023 •

edited

Loading