Skip to content

Conversation

@Sergio-Mira
Copy link

@Sergio-Mira Sergio-Mira commented Jan 19, 2026

Resolves #4496

Description

Fix flaky ItemizableUpdateService test. The issue is caused by the use of Timecop and having order dependence on Event entries in the logic that are ordered by event_time, updated_at.
Looks like that given some specific seeds the order of entries into the DB with the exact same timestamp would then sometimes result in out-of-order (vs entry order) events being returned that would be processed in InventoryAggregate:inventory_for in the wrong order, causing unexpected results as seen in the screenshot.

Recommendation is to not freeze time unless you want the time to be consistent across the test execution, I have instead removed the global time freeze and used it as a block on specific tests to mock Time.now similarly to what was intended before.

Also replaced usage of subject with a method definition as subject is normally and object not a method call, but can revert this change.

Type of change

  • Tests refactor

How Has This Been Tested?

  • Test suite passes
  • Test suite tested with broken seed for thousands of runs

Screenshots

Screenshot 2026-01-19 at 13 03 55

How the issue was found

  • Make sure we have a fast loop dev time (Disable SimpleCov by commenting it).
  • Use a script to run the specific spec for thousands of times until we reach a failure. Note down seed.
  • Having found a seed that reproduces the issue (or similar issue in this case) run with --bisect to try and trim down to the least specs possible. In this case surprisingly it was all tests although manual edits and runs later proved that it could be trimmed down a bit.
bundle exec rspec spec/services/itemizable_update_service_spec.rb --seed 38075 --fail-fast
  • Start digging back from the point of failure (current_quantity being zero) by debugging the test failure using a spec runner in VSCode
  • Add debugging outputs and compare runs with breaking seed and any other seed that made the test pass
  • Reached the method in InventoryAggregate:inventory_for where events were being outputted in a different order
  • Checked logic around ordering an noted down it was order(:event_time, :updated_at) (note no id in it)
  • Read about time precision and Timecop influence on it https://stevenharman.net/on-flaky-tests-time-precision-and-order-dependence
  • Disable Timecop for that test and prove it passes with breaking seed
  • Refactor tests to use Timecop in a different way

It was pretty hard to find, got lucky by getting a seed that would cause a similar issue consistently.

Copy link
Collaborator

@dorner dorner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this doesn't seem to work - it's causing failures on CI right now 😦 https://github.com/rubyforgood/human-essentials/actions/runs/21137813115/job/61316145366?pr=5479

@Sergio-Mira Sergio-Mira force-pushed the 4496-fix-flaky-itemizable-update-service-spec branch 2 times, most recently from 0a03229 to 85603b9 Compare January 25, 2026 14:46
@Sergio-Mira Sergio-Mira requested a review from dorner January 25, 2026 14:46
@Sergio-Mira
Copy link
Author

Thanks @dorner had a look at the failure and there can be small differences when loading the exact same date from the DB, looked at alternative ways to check the same date and applied them to the assert.

… removed the global Timecop freeze to be able to order Event entries.
@Sergio-Mira Sergio-Mira force-pushed the 4496-fix-flaky-itemizable-update-service-spec branch from 85603b9 to 5c8dd91 Compare January 25, 2026 20:07
expect(storage_location.size).to eq(26)
expect(new_storage_location.size).to eq(20)
expect(itemizable.issued_at).to eq(2.days.ago)
expect(itemizable.issued_at.to_fs(:db)).to eq(two_days_ago.to_fs(:db)) # Use to_fs to ignore nanosecond differences
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can use be_within(1.second) alternatively as I see it used in other specs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky Itemizable test

2 participants