Conversation
There was a problem hiding this comment.
Pull request overview
Adds new Go tests to expand coverage around hierarchical state handling (superstate/substate resolution), guarded transitions, ignored triggers, and internal transitions for the stateless state machine.
Changes:
- Added
transition_test.goto cover superstate vs substate transition selection, guarded fallback to ancestors, internal transitions, and multi-layer substate resolution. - Added
actions_test.goto cover ignored triggers in substates, conditional ignore, and entry/exit action behavior across superstate/substate transitions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| transition_test.go | New tests for transition resolution across super/substates and guarded behavior. |
| actions_test.go | New tests for ignored triggers and verifying superstate exit behavior during substate transitions. |
Comments suppressed due to low confidence (7)
transition_test.go:129
StateMachine.Firereturns anerror, but it’s ignored here. Assert the returned error is nil so the test fails if no guarded transition matches and the trigger becomes unhandled.
sm.Fire(triggerX)
transition_test.go:84
StateMachine.Firereturns anerror, but it’s ignored here. Assert the returned error is nil so the test fails if the internal transition isn’t actually handled.
sm.Fire(1)
actions_test.go:35
StateMachine.Firereturns anerror, but this test ignores it. IfIgnoreis misbehaving andFirereturns an error without changing state, the assertions here may not catch it. Asserterr == nil.
sm.Fire(triggerX)
actions_test.go:53
StateMachine.Firereturns anerror, but this test ignores it. Please assert the returned error is nil so failures aren’t silently missed.
sm.Fire(triggerX)
transition_test.go:41
StateMachine.Firereturns anerror, but it’s ignored here. Assert the returned error is nil so the test fails if the trigger becomes unhandled or an action returns an error.
sm.Fire(triggerX)
transition_test.go:60
StateMachine.Firereturns anerror, but it’s ignored here. Assert the returned error is nil so this test can’t pass whileFireis failing.
sm.Fire(triggerX)
actions_test.go:17
StateMachine.Firereturns anerror, but this test ignores it. For ignored-trigger behavior, the state may remain unchanged even ifFirereturned an error, so the test could pass while behavior is broken. Capture and assert the returned error is nil.
sm.Fire(triggerX)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sm.Fire(triggerX) | ||
| sm.Fire(triggerY) |
There was a problem hiding this comment.
Both sm.Fire calls return an error but the test ignores it. If either trigger becomes unhandled or an action returns an error, the test could miss it depending on the recorded side effects. Assert both errors are nil.
This issue also appears in the following locations of the same file:
- line 35
- line 53
- line 17
| sm.Configure(stateB). // Our super state | ||
| InitialTransition(stateC). | ||
| OnEntry(func(_ context.Context, _ ...any) error { | ||
| record = append(record, "Entered super state B") | ||
| return nil |
There was a problem hiding this comment.
This configuration block is not gofmt-formatted (indentation is inconsistent), which will create noisy diffs and diverge from standard Go formatting used elsewhere in the repo. Please run gofmt on this file.
| sm.Fire(triggerX) | ||
| if got := sm.MustState(); got != stateB { | ||
| t.Errorf("sm.MustState() = %v, want %v", got, stateB) | ||
| } | ||
|
|
There was a problem hiding this comment.
StateMachine.Fire returns an error, but it’s ignored here. Please assert err == nil so the test fails if the trigger is unexpectedly unhandled or an action returns an error (apply similarly to the subsequent Fire call in this test).
This issue also appears in the following locations of the same file:
- line 129
- line 84
- line 41
- line 60
| } | ||
| } | ||
|
|
||
| func TestStateMachine_InternalTransitionIf_ExecutesOnlyFirstMatchingAction(t *testing.T) { |
There was a problem hiding this comment.
Test name refers to InternalTransitionIf, but the API used is InternalTransition with guards. Consider renaming the test to match the public API surface (e.g., TestStateMachine_InternalTransition_...) to make it easier to find and understand.
| func TestStateMachine_InternalTransitionIf_ExecutesOnlyFirstMatchingAction(t *testing.T) { | |
| func TestStateMachine_InternalTransition_ExecutesOnlyFirstMatchingAction(t *testing.T) { |
| executed = append(executed, 2) | ||
| return nil | ||
| }, func(_ context.Context, _ ...any) bool { | ||
| return false | ||
| }) |
There was a problem hiding this comment.
The current setup doesn’t actually verify any “first match wins” behavior: only the first internal transition’s guard can ever match (the second returns false). Also, the implementation panics when multiple trigger behaviours match the same trigger in a state, so configuring both guards to match would make this test crash rather than select the first. Consider either renaming this test to reflect what it checks (single guarded internal transition executes) or changing it to assert that multiple matching behaviours cause a panic / require mutually-exclusive guards.
No description provided.