Skip to content

test(ocpp): stop test chargers after the server (#1712) - #1717

Merged
frahlg merged 1 commit into
masterfrom
1712-flaky-tests
Oct 17, 2026
Merged

frahlg merged 1 commit into
masterfrom
1712-flaky-tests

Conversation

@frahlg

@frahlg frahlg commented Oct 17, 2026

Copy link
Copy Markdown
Member

Part of #1712.

Problem

go/internal/ocpp panicked once in CI with send on closed channel (run 39045946853, on #1703, which does not touch OCPP). The stack ends in the test charge point: ocpp-go@v0.19.0/ws.(*Client).Write ← ocppj.(*Client).SendError ← chargePoint.notSupportedError ← readPump. The test running was TestPerChargerCredentialOverTheWire. Its last log line before the panic is Core's capability probe getting NotSupported from garage-impostor.

Core probes each charger it accepts: GetConfiguration, then a TriggerMessage identity probe. Several tests stop their charge point before the server, because the server's cleanup or defer was registered first and runs last. A probe can then reach the charger while it closes. ws.Client.Write checks IsConnected() and then sends on outQueue without its lock, while cleanup() closes outQueue. An answer that passes the check just before the close panics.

The ocpp-go client runs only in tests and cmd/sim-ocpp; Core uses its server. There is no newer ocpp-go release than v0.19.0.

Change

stopAfterServer(t, srv, stop) (in server_test.go) stops the server first at cleanup, then the charger. Server.Stop is idempotent (stopOnce) and stops the identity probes before it closes connections. It replaces the charger cleanup where the charger used to stop first:

  • auth_test.go (TestPerChargerCredentialOverTheWire)
  • identity_test.go (TestBootNotificationTakesEitherSerialField)
  • server_test.go (three tests that used defer cp.Stop() after defer srv.Stop())
  • control_test.go connectCharger, control_v201_test.go connectStationV201, charging_needs_test.go connectStationForNeeds

Tests that stop the server with defer already stopped it before their t.Cleanup chargers, so capabilities_test.go and most of control_test.go were safe and do not change.

Not covered

Evidence

  • The panic did not reproduce locally in 2,000 runs of TestPerChargerCredentialOverTheWire with -race (four parallel runs of 500, GOMAXPROCS=2); the window is a few instructions wide. The fix is by construction: no probe can be sent once the server has stopped.
  • go test -race -count=20 ./internal/ocpp/: ok (57.8 s).
  • go vet ./internal/ocpp/: ok.
  • make verify was not run locally; CI runs the full suite.

Test-only change, so it carries the no-changeset label.

🤖 Generated with Claude Code

Core probes each charger it accepts (GetConfiguration, then a
TriggerMessage identity probe). Several tests stopped their charge point
before the server, so a probe could arrive while the charger closed.
ocpp-go v0.19.0's ws.Client.Write checks the connection and then sends
without its lock, so the answer could send on the closed queue and panic
("send on closed channel", seen in run 39045946853).

stopAfterServer stops the server first at cleanup; Server.Stop is
idempotent. Only test code changes; Core uses the ocpp-go server, and
the client runs only in tests and cmd/sim-ocpp.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
@frahlg frahlg added the no-changeset PR intentionally exempt from the changeset requirement (dev tooling / non-shipping) label Oct 17, 2026
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 17, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-17T18:35:12.478666Z 194362c PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@frahlg
frahlg merged commit 2dde246 into master Oct 17, 2026
22 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changeset PR intentionally exempt from the changeset requirement (dev tooling / non-shipping)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant