openfoodfoundation / openfoodnetwork

Connect suppliers, distributors and consumers to trade local produce.
https://www.openfoodnetwork.org
GNU Affero General Public License v3.0
1.12k stars 724 forks source link

[Flaky] ./spec/system/admin/order_cycles/edit_spec.rb:65 #12786

Open filipefurtad0 opened 3 months ago

filipefurtad0 commented 3 months ago

What we should change and why (this is tech debt)

./spec/system/admin/order_cycles/edit_spec.rb:65


  1) 
    As an administrator
    I want to edit a specific order cycle
 simple order cycle with no attached order does not show warning modal
     Failure/Error: expect(page).to have_content('Your order cycle has been updated.')
       expected to find text "Your order cycle has been updated." in "Logged in as : debera@boylegleason.biz Account Logout Store Dashboard Products Order cycles Orders Reports Configuration Enterprises Customers Groups Users Edit Order Cycle Advanced Settings Notify producers You have unsaved changes Name * Orders open at Coordinator Enterprise 185 Orders close Schedules Ready for * Customer instructions Products Select all Tags Fees"

     [Screenshot Image]: /home/runner/work/openfoodnetwork/openfoodnetwork/tmp/capybara/screenshots/failures_r_spec_example_groups_as_an_administrator_i_want_to_edit_a_specific_order_cycle_simple_order_cycle_with_no_attached_order_does_not_show_warning_modal_821.png

Context

https://github.com/openfoodfoundation/openfoodnetwork/actions/runs/10426143065/job/28921607320?pr=12785

Impact and timeline

filipefurtad0 commented 3 months ago

Re-occuring here: https://github.com/openfoodfoundation/openfoodnetwork/actions/runs/10478749105/job/29022833209?pr=12760

And here: https://github.com/openfoodfoundation/openfoodnetwork/actions/runs/10479002344/job/29023660900?pr=12790

filipefurtad0 commented 3 months ago

Still occurring. The only thing I can think of is adding a sleep(2) before the failing assertion, to make sure we bypass the Updating... warning...

Any other ideas?

Edit: sleeping this does not fix it either. The failing scenario looks like this:

image

1) The change is made 2) The Save button is clicked (it displays a slightly different color) 3) The message does not change to Updating... nor to Your order cycle has been updated.

I've tried to reproduce this locally and on staging, but was not successful. Chrome throws out some warnings on this regard, but these seem unrelated to me:

image

wandji20 commented 3 months ago

@filipefurtad0 I tried adding sleep(2) just before the save button is clicked and it seems to work fine for me. (Tested 50 plus times consecutively without failure). Not sure if it's worth it but mentioning in case. Here the diff

diff --git a/spec/system/admin/order_cycles/edit_spec.rb b/spec/system/admin/order_cycles/edit_spec.rb
index c3899aa35e..337e9bdf46 100644
--- a/spec/system/admin/order_cycles/edit_spec.rb
+++ b/spec/system/admin/order_cycles/edit_spec.rb
@@ -83,6 +83,7 @@ RSpec.describe '
           find("button", text: "Close").click
         end
         expect(page).to have_content('You have unsaved changes')
+        sleep(2)

         click_button('Save')
         expect(page).to have_field 'order_cycle_orders_close_at', with: '2024-03-30 00:00'
@@ -155,6 +156,7 @@ RSpec.describe '
           find("button", text: "Close").click
         end
         expect(page).to have_content('You have unsaved changes')
+        sleep(2)

         click_button('Save')
         expect(page).to have_content('Your order cycle has been updated.')
filipefurtad0 commented 3 months ago

Thanks so much @wandji20,

Did you push this change to master? The thing is I can't make it fail locally (I've ran several variations, 50x each and no failures on my local env.) but if you observe the test to master on CI, then I guess it could be an alternative to my PR here.

What do you think is best?

wandji20 commented 3 months ago

Did you push this change to master?

No, I was only testing locally. This one can be annoying as it can't be consistently reproduced.

What do you think is best?

How about we try both, may be my suggestion with rerty first

filipefurtad0 commented 3 months ago

How about we try both, may be my suggestion with rerty first

Great idea :raised_hands: I mean, both approaches are a workaround but I'd say we need to get this one green asap. I'll add the two seconds sleep on the PR. Thanks!

filipefurtad0 commented 2 months ago

This has not been occurring since #12800; I've added an entry in https://github.com/openfoodfoundation/openfoodnetwork/issues/12086, so I think we can close here.

mkllnk commented 1 month ago

It just came up again, despite retry: