Skip to content

OCPP: guard RemoteStartTransaction#30954

Open
premultiply wants to merge 1 commit into
masterfrom
premultiply/issue30943
Open

OCPP: guard RemoteStartTransaction#30954
premultiply wants to merge 1 commit into
masterfrom
premultiply/issue30943

Conversation

@premultiply

@premultiply premultiply commented Jun 17, 2026

Copy link
Copy Markdown
Member

May fix #30943

@github-actions github-actions Bot added the stale Outdated and ready to close label Jun 24, 2026
@premultiply premultiply removed the stale Outdated and ready to close label Jun 25, 2026
@premultiply premultiply marked this pull request as ready for review June 25, 2026 20:30
Copilot AI review requested due to automatic review settings June 25, 2026 20:30

This comment was marked as low quality.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey - I've found 1 issue

Prompt for AI Agents
Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="charger/ocpp/connector_test.go" line_range="237-249" />
<code_context>
+	preparing(time.Second)
+	suite.True(suite.conn.remoteStarted, "guard must stay set on repeated Preparing")
+
+	// leaving Preparing (e.g. cable unplugged) ends the auth cycle
+	_, err := suite.conn.OnStatusNotification(&core.StatusNotificationRequest{
+		ConnectorId: 1,
+		Status:      core.ChargePointStatusAvailable,
+		ErrorCode:   core.NoError,
+		Timestamp:   types.NewDateTime(suite.clock.Now().Add(2 * time.Second)),
+	})
+	suite.NoError(err)
+	suite.False(suite.conn.remoteStarted, "guard must reset when leaving Preparing")
+
+	// next Preparing re-arms a fresh RemoteStartTransaction
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for other non-Preparing states that should also reset the RemoteStart guard

The production change resets `remoteStarted` on any transition away from `Preparing`, not only when going to `Available`. To fully exercise this behavior, please add cases that transition from `Preparing` to other statuses (e.g. `Charging`, `SuspendedEV`, `SuspendedEVSE`, or an error state) and assert that the guard resets in each case, possibly via a small table-driven test.

```suggestion
	// leaving Preparing (e.g. cable unplugged) ends the auth cycle
	_, err := suite.conn.OnStatusNotification(&core.StatusNotificationRequest{
		ConnectorId: 1,
		Status:      core.ChargePointStatusAvailable,
		ErrorCode:   core.NoError,
		Timestamp:   types.NewDateTime(suite.clock.Now().Add(2 * time.Second)),
	})
	suite.NoError(err)
	suite.False(suite.conn.remoteStarted, "guard must reset when leaving Preparing")

	// leaving Preparing for any other status must also end the auth cycle and reset the guard
	transitions := []struct {
		name   string
		status core.ChargePointStatus
		code   core.ChargePointErrorCode
	}{
		{"Charging", core.ChargePointStatusCharging, core.NoError},
		{"SuspendedEV", core.ChargePointStatusSuspendedEV, core.NoError},
		{"SuspendedEVSE", core.ChargePointStatusSuspendedEVSE, core.NoError},
		{"FaultedWithError", core.ChargePointStatusFaulted, core.InternalError},
	}

	for _, tt := range transitions {
		// re-arm from Preparing
		preparing(0)
		suite.True(suite.conn.remoteStarted, "guard must be set before transitioning to %s", tt.name)

		_, err := suite.conn.OnStatusNotification(&core.StatusNotificationRequest{
			ConnectorId: 1,
			Status:      tt.status,
			ErrorCode:   tt.code,
			Timestamp:   types.NewDateTime(suite.clock.Now().Add(2 * time.Second)),
		})
		suite.NoError(err)
		suite.False(suite.conn.remoteStarted, "guard must reset when leaving Preparing to %s", tt.name)
	}

	// next Preparing re-arms a fresh RemoteStartTransaction
	preparing(3 * time.Second)
	suite.True(suite.conn.remoteStarted, "guard must re-arm on the next auth cycle")
```
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines +237 to +249
// leaving Preparing (e.g. cable unplugged) ends the auth cycle
_, err := suite.conn.OnStatusNotification(&core.StatusNotificationRequest{
ConnectorId: 1,
Status: core.ChargePointStatusAvailable,
ErrorCode: core.NoError,
Timestamp: types.NewDateTime(suite.clock.Now().Add(2 * time.Second)),
})
suite.NoError(err)
suite.False(suite.conn.remoteStarted, "guard must reset when leaving Preparing")

// next Preparing re-arms a fresh RemoteStartTransaction
preparing(3 * time.Second)
suite.True(suite.conn.remoteStarted, "guard must re-arm on the next auth cycle")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggestion (testing): Add coverage for other non-Preparing states that should also reset the RemoteStart guard

The production change resets remoteStarted on any transition away from Preparing, not only when going to Available. To fully exercise this behavior, please add cases that transition from Preparing to other statuses (e.g. Charging, SuspendedEV, SuspendedEVSE, or an error state) and assert that the guard resets in each case, possibly via a small table-driven test.

Suggested change
// leaving Preparing (e.g. cable unplugged) ends the auth cycle
_, err := suite.conn.OnStatusNotification(&core.StatusNotificationRequest{
ConnectorId: 1,
Status: core.ChargePointStatusAvailable,
ErrorCode: core.NoError,
Timestamp: types.NewDateTime(suite.clock.Now().Add(2 * time.Second)),
})
suite.NoError(err)
suite.False(suite.conn.remoteStarted, "guard must reset when leaving Preparing")
// next Preparing re-arms a fresh RemoteStartTransaction
preparing(3 * time.Second)
suite.True(suite.conn.remoteStarted, "guard must re-arm on the next auth cycle")
// leaving Preparing (e.g. cable unplugged) ends the auth cycle
_, err := suite.conn.OnStatusNotification(&core.StatusNotificationRequest{
ConnectorId: 1,
Status: core.ChargePointStatusAvailable,
ErrorCode: core.NoError,
Timestamp: types.NewDateTime(suite.clock.Now().Add(2 * time.Second)),
})
suite.NoError(err)
suite.False(suite.conn.remoteStarted, "guard must reset when leaving Preparing")
// leaving Preparing for any other status must also end the auth cycle and reset the guard
transitions := []struct {
name string
status core.ChargePointStatus
code core.ChargePointErrorCode
}{
{"Charging", core.ChargePointStatusCharging, core.NoError},
{"SuspendedEV", core.ChargePointStatusSuspendedEV, core.NoError},
{"SuspendedEVSE", core.ChargePointStatusSuspendedEVSE, core.NoError},
{"FaultedWithError", core.ChargePointStatusFaulted, core.InternalError},
}
for _, tt := range transitions {
// re-arm from Preparing
preparing(0)
suite.True(suite.conn.remoteStarted, "guard must be set before transitioning to %s", tt.name)
_, err := suite.conn.OnStatusNotification(&core.StatusNotificationRequest{
ConnectorId: 1,
Status: tt.status,
ErrorCode: tt.code,
Timestamp: types.NewDateTime(suite.clock.Now().Add(2 * time.Second)),
})
suite.NoError(err)
suite.False(suite.conn.remoteStarted, "guard must reset when leaving Preparing to %s", tt.name)
}
// next Preparing re-arms a fresh RemoteStartTransaction
preparing(3 * time.Second)
suite.True(suite.conn.remoteStarted, "guard must re-arm on the next auth cycle")

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.

RemoteStartTransaction is timing out

2 participants