human-essentials icon indicating copy to clipboard operation
human-essentials copied to clipboard

Flaky Itemizable test

Open dorner opened this issue 1 year ago • 10 comments

Summary

The following test only fails sometimes: https://github.com/rubyforgood/human-essentials/actions/runs/9780748669/job/27003147071?pr=4488

 1) ItemizableUpdateService events with distributions should send an itemizable event if it already exists
     Failure/Error:
       raise InventoryError.new("Could not reduce quantity by #{quantity} - current quantity is #{current_quantity}",
         item_id,
         id)

     InventoryError:
       Could not reduce quantity by 5 - current quantity is 0 for My Item 1 in Smithsonian Conservation Center
     # ./app/events/event_types/event_storage_location.rb:35:in `reduce_inventory'
     # ./app/events/event_types/inventory.rb:45:in `move_item'
     # ./app/events/inventory_aggregate.rb:71:in `block in handle_inventory_event'
     # ./app/events/inventory_aggregate.rb:65:in `each'
     # ./app/events/inventory_aggregate.rb:65:in `handle_inventory_event'
     # ./app/events/inventory_aggregate.rb:114:in `block in <module:InventoryAggregate>'
     # ./app/events/inventory_aggregate.rb:57:in `handle'
     # ./app/events/inventory_aggregate.rb:42:in `block in inventory_for'
     # ./app/events/inventory_aggregate.rb:33:in `each'
     # ./app/events/inventory_aggregate.rb:33:in `inventory_for'
     # ./app/models/event.rb:84:in `validate_inventory'
     # ./app/events/distribution_event.rb:4:in `publish'
     # ./spec/services/itemizable_update_service_spec.rb:234:in `block (4 levels) in <top (required)>'
     # ./spec/services/itemizable_update_service_spec.rb:23:in `block (3 levels) in <top (required)>'
     # ./spec/services/itemizable_update_service_spec.rb:22:in `block (2 levels) in <top (required)>'
     
     Randomized with seed 18466

Things to consider

No response

Criteria for Completion

No response

dorner avatar Jul 04 '24 00:07 dorner

I took a crack at figuring this out but was unsuccessful. Just commenting in case it helps someone else in the future.

I ran the test suite with --bisect and the given seed on the commit that this occurred on and wasn't able to reproduce the failure.

I also looked through the code a bit. At first I thought it might be flaky because there are two rspec before blocks during setup that create inventory (TestInventory#create_inventory) for the same storage location and that method has a comment saying it "Blows away any current inventory for the storage locations." But it turns out to only wipe current inventory for the given items (in a storage location) passed in. So I'm not sure where the issue is.

Best of luck future contributor.

coalest avatar Dec 18 '24 10:12 coalest

Hi, I'd be interested in working on this issue.

I was able to locally reproduce the failure by running the individual spec 400 times. The flake appears to occur when the from_location has no items associated with it, and reduce_inventory method sets current_quantity to equal 0.

I haven't yet figured out why the from_location has no items, but I look forward to investigating!

McEileen avatar Jan 05 '25 18:01 McEileen

It's yours -- Good hunting!

cielf avatar Jan 05 '25 19:01 cielf

This issue is marked as stale due to no activity within 30 days. If no further activity is detected within 7 days, it will be unassigned.

github-actions[bot] avatar Feb 05 '25 00:02 github-actions[bot]

Hi, I am having a tough time making progress. As previously noted, I was able to reproduce the error and note that it occurs in the event_types/inventory’s move_item method. When the event storage_locations[from_location] doesn’t have any items associated with it, the reduce_inventory method sets item current_quantity to 0. and the error seen in the flaky test occurs.

I can’t figure out why storage_locations[from_location] doesn’t have items associated with it. I couldn’t find any bug in test inventory setup. When I added puts statements to see the items in the test inventory, there was no discrepancy between the tests that passed and the tests that flaked. I still tried to combine the two before blocks into one, which can be seen on my branch here. The test continued to flake with this change.

I did notice that the itemizable_update_service method update_storage_location deletes all line items. But it then updates the distribution with the line items attributes in the params. So, I can’t think of how that would contribute to this flake.

I’d appreciate input from anyone who’s more familiar with the code or has an idea what might be causing this flake. If I don’t make progress by this upcoming Monday, I’ll remove myself from the issue.

McEileen avatar Feb 05 '25 13:02 McEileen

As an aside -- Not a code familiarity insight per se. Storage locations are initially created with no items.. but you should never get a "from" storage location with no items at all in it in the wild, because you can't select a storage location with no items when entering a distribution.

cielf avatar Feb 06 '25 00:02 cielf

@dorner -- this is definitely in your bailiwick.

cielf avatar Feb 06 '25 01:02 cielf

Nothing jumps out at me. Sometimes there needs to be reload statements in judicious places to ensure the data is correct.

dorner avatar Feb 07 '25 20:02 dorner

I haven't made progress since posting and I am going to step aside. Could someone please unassign me from this issue? Thanks!

McEileen avatar Feb 15 '25 21:02 McEileen

Thanks for the attempt!

cielf avatar Feb 16 '25 00:02 cielf