celery / jumpstarter

MIT License
7 stars 3 forks source link

Test coverage missing for starting the actor when it is in stopped state #11

Closed thedrow closed 3 years ago

thedrow commented 3 years ago

Currently, it is possible to transition from the stopped state back to the starting state using the start trigger. However, there is currently no test that verifies that such a transition is possible.

The test should start() the actor, stop() it and then start() it again.

ahopkins commented 3 years ago

Would there be a use case to add a .restart()?

ahopkins commented 3 years ago

I am not so sure this makes sense:

await fake_actor.start()
assert fake_actor.state == ActorState.started

await fake_actor.stop()
assert fake_actor.state == ActorState.stopped

await fake_actor.start()
assert fake_actor.state == ActorState.starting

This is not intuitive that it would be in starting and not mirror the first transition. What am I missing?

thedrow commented 3 years ago

Would there be a use case to add a .restart()?

Yes there is but there's no issue about it yet other than a TODO task in #3.

thedrow commented 3 years ago

I am not so sure this makes sense:

await fake_actor.start()
assert fake_actor.state == ActorState.started

await fake_actor.stop()
assert fake_actor.state == ActorState.stopped

await fake_actor.start()
assert fake_actor.state == ActorState.starting

This is not intuitive that it would be in starting and not mirror the first transition. What am I missing?

See the description of how the transitions work in #3. Essentially, after each transition, we call start() or stop() again to progress to the next state.

ahopkins commented 3 years ago

Right, but my point is this:

fake_actor = FakeActor()

assert fake_actor.state == ActorState.initializing

await fake_actor.start()
assert fake_actor.state == ActorState.started

await fake_actor.stop()
assert fake_actor.state == ActorState.stopped

await fake_actor.start()
assert fake_actor.state == ActorState.starting

await fake_actor.start()
assert fake_actor.state == ActorState.started

There is not a consistent progression of states from -ing to -ed

thedrow commented 3 years ago

The progression is internal. You will be eventually able to hook callbacks on those states (and transitions occasionally) to perform custom startup/shutdown activities.

ahopkins commented 3 years ago

A more intuitive API would be this:

fake_actor = FakeActor()

assert fake_actor.state == ActorState.initialized

await fake_actor.start()
assert fake_actor.state == ActorState.started

await fake_actor.stop()
assert fake_actor.state == ActorState.stopped

await fake_actor.start()
assert fake_actor.state == ActorState.started

With the initializing state hidden inside internals. And, the same for the second .start() >> starting.

Have you considered something like this:


fake_actor = FakeActor()

assert fake_actor.state == ActorState.initializing

await fake_actor.init()
assert fake_actor.state == ActorState.initialized

async with fake_actor.start():
    assert fake_actor.state == ActorState.starting

assert fake_actor.state == ActorState.started

async with fake_actor.stop():
    assert fake_actor.state == ActorState.stopping

assert fake_actor.state == ActorState.stopped

await fake_actor.start()
assert fake_actor.state == ActorState.started

await fake_actor.stop()
thedrow commented 3 years ago

We can't do that since the transitions library doesn't allow us to do so. The start() and stop() triggers are automatically attached to the Actor when we cls._state_machine.add_model(self) and they are defined by the state machine itself.

In any case, this is not needed and incorrect. The assertion would fail. This is the correct assertion with your API:

async with fake_actor.start():
    assert fake_actor.state == ActorSartingState.tasks_started

assert fake_actor.state == ActorState.started

Only the initial transition from initialized to starting sets the state to starting. Afterward, we transition into a sub-state of starting. See diagram at #3 or refer to the table with the description of states.