chore: run tests with race checks #25870

xBlaz3kx · 2025-12-07T11:58:39Z

Since there is a lot of concurrent code with mutexes, channels etc, it would make sense that tests run with -race check.

sourcery-ai

Hey there - I've reviewed your changes - here's some feedback:

The race detector requires cgo, so CGO_ENABLED=0 will prevent -race from working; consider either removing CGO_ENABLED=0 for this target or providing a separate race-enabled test target.
Running all tests with -race by default can significantly slow down the test suite; you might want to add a separate test-race make target instead of changing the default test target.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The race detector requires cgo, so `CGO_ENABLED=0` will prevent `-race` from working; consider either removing `CGO_ENABLED=0` for this target or providing a separate race-enabled test target.
- Running all tests with `-race` by default can significantly slow down the test suite; you might want to add a separate `test-race` make target instead of changing the default `test` target.

## Individual Comments

### Comment 1
<location> `Makefile:83` </location>
<code_context>
 test::
 	@echo "Running testsuite"
-	CGO_ENABLED=0 go test $(BUILD_TAGS) ./...
+	CGO_ENABLED=0 go test -race $(BUILD_TAGS) ./...

 porcelain::
</code_context>

<issue_to_address>
**issue (testing):** Race detector is effectively disabled when CGO is turned off.

Because the Go race detector depends on cgo, `-race` is ignored when `CGO_ENABLED=0`. If you want these tests to run with race detection, you’ll need to run them with `CGO_ENABLED=1` (e.g. via a separate `test-race` target) so the flag actually takes effect.
</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.}

Makefile

andig · 2025-12-07T12:36:13Z

Since there is a lot of code with mutexes, channels etc, it would make sense that tests run with -race check.

...only this won't work on all plattforms that we're supporting :/

xBlaz3kx · 2025-12-07T13:23:24Z

Since there is a lot of code with mutexes, channels etc, it would make sense that tests run with -race check.

...only this won't work on all plattforms that we're supporting :/

Better on some than on none, right? I would guess Linux is the primary target platform

andig · 2025-12-07T15:39:12Z

Makefile

+	@OS=$$(go env GOOS); \
+	if [ "$$OS" = "linux" ] || [ "$$OS" = "darwin" ]; then \
+		echo "Running tests on $$OS"; \
+		CGO_ENABLED=1 go test -race $(BUILD_TAGS) ./...; \


Suggested change

CGO_ENABLED=1 go test -race $(BUILD_TAGS) ./...; \

CGO_ENABLED=0 go test -race $(BUILD_TAGS) ./...; \

Sadly, this is required - #25870 (comment)

Because the bot says so? I can test, that's for sure.

CGO_ENABLED=0 got -race . -timeout 5s -run TestRace ================== WARNING: DATA RACE Read at 0x00c000674a58 by goroutine 31: github.com/evcc-io/evcc.TestRace.func2() /Users/a25058/htdocs/evcc/race_test.go:16 +0x2c Previous write at 0x00c000674a58 by goroutine 30: github.com/evcc-io/evcc.TestRace.func1() /Users/a25058/htdocs/evcc/race_test.go:10 +0x3c Goroutine 31 (running) created at: github.com/evcc-io/evcc.TestRace() /Users/a25058/htdocs/evcc/race_test.go:14 +0xf8 testing.tRunner() /opt/homebrew/Cellar/go/1.25.5/libexec/src/testing/testing.go:1934 +0x164 testing.(*T).Run.gowrap1() /opt/homebrew/Cellar/go/1.25.5/libexec/src/testing/testing.go:1997 +0x3c Goroutine 30 (running) created at: github.com/evcc-io/evcc.TestRace() /Users/a25058/htdocs/evcc/race_test.go:8 +0x98 testing.tRunner() /opt/homebrew/Cellar/go/1.25.5/libexec/src/testing/testing.go:1934 +0x164 testing.(*T).Run.gowrap1() /opt/homebrew/Cellar/go/1.25.5/libexec/src/testing/testing.go:1997 +0x3c ================== ================== WARNING: DATA RACE Write at 0x00c000674a58 by goroutine 31: github.com/evcc-io/evcc.TestRace.func2() /Users/a25058/htdocs/evcc/race_test.go:16 +0x3c Previous write at 0x00c000674a58 by goroutine 30: github.com/evcc-io/evcc.TestRace.func1() /Users/a25058/htdocs/evcc/race_test.go:10 +0x3c Goroutine 31 (running) created at: github.com/evcc-io/evcc.TestRace() /Users/a25058/htdocs/evcc/race_test.go:14 +0xf8 testing.tRunner() /opt/homebrew/Cellar/go/1.25.5/libexec/src/testing/testing.go:1934 +0x164 testing.(*T).Run.gowrap1() /opt/homebrew/Cellar/go/1.25.5/libexec/src/testing/testing.go:1997 +0x3c Goroutine 30 (running) created at: github.com/evcc-io/evcc.TestRace() /Users/a25058/htdocs/evcc/race_test.go:8 +0x98 testing.tRunner() /opt/homebrew/Cellar/go/1.25.5/libexec/src/testing/testing.go:1934 +0x164 testing.(*T).Run.gowrap1() /opt/homebrew/Cellar/go/1.25.5/libexec/src/testing/testing.go:1997 +0x3c ================== FAIL FAIL github.com/evcc-io/evcc 1.352s FAIL

package main import "testing" func TestRace(t *testing.T) { var i int go func() { for { i += 1 } }() go func() { for { i -= 1 } }() }

Ah: golang/go#9918 (comment)

Yeah we can use a different race detector (in the issue thread) or leave as-is.

Shall we split the cases once more? No CGO would be preferred since it may not always build when you pull in the wrong dependency...

I mean split osx and linux?

Sure, we can split this further

andig · 2025-12-07T15:40:38Z

We can only merge this when issues are fixed. Amongst them lorenzodonini/ocpp-go#309

xBlaz3kx · 2025-12-07T16:57:23Z

We can only merge this when issues are fixed. Amongst them lorenzodonini/ocpp-go#309

Yep, Ill try to find some time to get this resolved

andig · 2025-12-07T18:04:08Z

Bzw, great PRs- did you already join us on Slack?

xBlaz3kx · 2025-12-07T22:02:57Z

Bzw, great PRs- did you already join us on Slack?

Thanks! I just did 😃

andig added enhancement New feature or request infrastructure Basic functionality labels Dec 7, 2025

sourcery-ai bot reviewed Dec 7, 2025

View reviewed changes

Makefile Outdated Show resolved Hide resolved

andig marked this pull request as draft December 7, 2025 12:36

xBlaz3kx force-pushed the chore/test-with-race branch from 9fc54cc to bc03751 Compare December 7, 2025 13:40

andig reviewed Dec 7, 2025

View reviewed changes

andig added backlog Things to do later and removed enhancement New feature or request labels Dec 7, 2025

chore: run tests with race checks only on linux & darwin

6b5fd03

xBlaz3kx force-pushed the chore/test-with-race branch from bc03751 to 6b5fd03 Compare December 7, 2025 22:19

	CGO_ENABLED=1 go test -race $(BUILD_TAGS) ./...; \
	CGO_ENABLED=0 go test -race $(BUILD_TAGS) ./...; \

Uh oh!

chore: run tests with race checks #25870

Are you sure you want to change the base?

chore: run tests with race checks #25870

Uh oh!

Conversation

xBlaz3kx commented Dec 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andig commented Dec 7, 2025

Uh oh!

xBlaz3kx commented Dec 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xBlaz3kx Dec 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andig commented Dec 7, 2025

Uh oh!

xBlaz3kx commented Dec 7, 2025

Uh oh!

andig commented Dec 7, 2025

Uh oh!

xBlaz3kx commented Dec 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

xBlaz3kx commented Dec 7, 2025 •

edited

Loading

xBlaz3kx Dec 7, 2025 •

edited

Loading