gocardless / statesman

A statesmanlike state machine library.
https://gocardless.com/blog/statesman/
MIT License
1.78k stars 163 forks source link

Incorrect current_state when using Timecop in tests? #425

Closed NicoSa closed 3 years ago

NicoSa commented 3 years ago

Hi there,

I have the following scenario. I want to change the state of my items from ready to failed if their storage days are post a certain time. Now, this seems to be working. However in my test scenario, the current_state of my state machine is wrong. Despite adding the expected transition to failed after the ready transition. Here's the test, I think the issue is that for some reason last_transition remains ready. No idea why, if I just test whether the current_state changes without Timecop, there are no issues and the current_state changes to failed as expected.

should 'set expired item to state failed' do
    time = Time.parse('2020-01-02 09:00:00 UTC') 
    four_days_later = Time.parse('2020-01-06 09:00:00 UTC')

    Timecop.freeze(time) do
        @item = create(:ready_item, storage_days: 2)
        @another_item = create(:ready_item, storage_days: 10)           

        assert_equal 'ready', @item.state
        assert_equal 'ready', @another_item.state

        Timecop.travel(four_days_later)

        Cron::SetToFailedWhereDue.perform

        assert_equal 1, @item.transitions.where(to_state: 'failed').count
        assert_equal 'failed', @item.state #this is where it fails! The state remains in ready for some reason.
        assert_equal 0, @another_item.transitions.where(to_state: 'failed').count
        assert_equal 'ready', @another_item.state
    end
end
thom-oman commented 3 years ago

Hey @NicoSa is this an ActiveRecord model? If so you may need to call reload on @item as this forces the in-memory object to be refreshed from the database.

NicoSa commented 3 years ago

@thom-oman it is, generally a good call, however I have tried this and it did not change the outcome of current_state despite the transition being there as proven by the test. #state in this case is just a short for @item.state_machine.current_state.

thom-oman commented 3 years ago

The only thing I can think of is if state is defined something like

def state
  @state ||= @item.state_machine.current_state
end

and without any more information its hard for me to diagnose further. Is there anything else you can provide that may help?

NicoSa commented 3 years ago

Now that you mention your example, could it be the state_machine method somehow, see item model attached.

# frozen_string_literal: true

class Item < ActiveRecord::Base
  include Statesman::Adapters::ActiveRecordQueries

  STATES = [
    ST_NEW = 'new',
    ST_READY = 'ready',
    ST_RECEIVED = 'received',
    ST_RETURNED = 'returned',
    ST_FAILED = 'failed',
  ]

  has_many :transitions, foreign_key: 'item_id', class_name: 'ItemTransition', autosave: false, dependent: :delete_all

  scope :with_transitions, -> { joins(:transitions).where(item_transitions: { most_recent: true }) }

  def self.transition_class
    ItemTransition
  end

  def self.initial_state
    ST_NEW
  end

  def state_machine
    @state_machine ||= ItemStateMachine.new(self, transition_class: ItemTransition, association_name: :transitions)
  end

  def state
    state_machine.current_state
  end

  def ready?
    state_machine.in_state? ST_READY
  end

  def received?
    state_machine.in_state? ST_RECEIVED
  end

  def failed?
    state_machine.in_state? ST_FAILED
  end

  def returned?
    state_machine.in_state? ST_RETURNED
  end

  def self.valid_state?(state)
    self::STATES.include?(state)
  end
end
danwakefield commented 3 years ago

Yes, You should probably reload the current state if you don't handle stale reads manually.

state_machine.current_state(force_reload: true)

NicoSa commented 3 years ago

This was the solution! Thank you @danwakefield ! Works like a charm! :)