Repository navigation
Tests: Add additional test coverage for LimboHSM::active_state_changed - #470
Conversation
bennbollay
changed the title
| active = p_active; | ||
| is_initiating_update = p_active; | ||
| if (p_active) { | ||
| active = p_active; | ||
| is_initiating_update = p_active; | ||
|
|
||
| if (active) { | ||
| _enter(); | ||
| } else { | ||
| _exit(); | ||
|
|
||
| active = p_active; | ||
| is_initiating_update = p_active; | ||
| } |
There was a problem hiding this comment.
You've got duplicated code in the two branches.
Also, LimboState::_enter() sets active to true, same but false for LimboState::_exit(), so no need to set manually active = p_active afterwards.
| void LimboHSM::change_active_state(LimboState *p_state) { | ||
| ERR_FAIL_NULL(p_state); | ||
| ERR_FAIL_COND_MSG(!is_active(), "LimboHSM: Unable to change active state when HSM is not active."); | ||
| ERR_FAIL_COND_MSG(p_state->get_parent() != this, "LimboHSM: Unable to perform transition to a state that is not a child of this HSM."); | ||
| ERR_FAIL_COND_MSG(p_state && p_state->get_parent() != this, "LimboHSM: Unable to perform transition to a state that is not a child of this HSM."); | ||
|
|
||
| if (active_state) { | ||
| active_state->_exit(); | ||
| previous_active = active_state; | ||
| } | ||
|
|
||
| previous_active = active_state; | ||
| active_state = p_state; | ||
| active_state->_enter(); | ||
|
|
||
| if (active_state) { | ||
| active_state->_enter(); | ||
| } | ||
|
|
||
| emit_signal(LW_NAME(active_state_changed), active_state, previous_active); | ||
|
|
||
| active_state->_clear_cargo(); | ||
| if (active_state) { | ||
| active_state->_clear_cargo(); | ||
| } | ||
| } |
There was a problem hiding this comment.
I would take the helper class I created in #169 instead. It's necessary to be able to call _change_active_state(nullptr) in case of changes in set_active().
Using an helper class was suggested to me by @limbonaut.
There was a problem hiding this comment.
The ActiveStateUpdatedReturnedValues could be used in lieu of using SIGNAL_WATCH but SIGNAL_WATCH is part of the standard test tooling offered in the engine. It was a new discovery for both @limbonaut and myself fairly recently!
limbonaut
left a comment
limbonaut
left a comment
There was a problem hiding this comment.
Thanks for submitting, @bennbollay! Since we almost got #469 to the finish line, let’s go with that one. And then we can consider testing improvements too, if you’d like to contribute those.
|
@adamscott Are there additional behavioral changes that are expected that aren't covered by the test cases implemented? |
|
Added additional tests and support for |
bennbollay
changed the title
|
Alright merged; most everything here now is just test changes, with one behavior change around supporting change_active_state. If that's not desired, it can easily be reverted back to the current behavior where that is forbidden. |
There was a problem hiding this comment.
Good stuff! Thanks for submitting this and improving the test coverage. It still needs a bit of work before I can merge it.
If that's not desired, it can easily be reverted back to the current behavior where that is forbidden.
The main concern is that I don't think changing the state to null should be permitted as that would violate our invariant (see code comments).
bennbollay
changed the title
|
Good review, thank you! Changes applied and PR title updated. |
Docs: Note LimboHSM.EVENT_FINISHED as a way of marking a tree inactive