Copilot commented on code in PR #64650:
URL: https://github.com/apache/airflow/pull/64650#discussion_r3066474331
##########
go-sdk/pkg/worker/runner_test.go:
##########
@@ -189,9 +188,8 @@ func (s *WorkerSuite)
TestTaskReturnErrorReportsFailedState() {
}
func (s *WorkerSuite) TestTaskHeartbeatsWhileRunning() {
- s.T().Parallel()
id := uuid.New().String()
- callCount := 0
+ var callCount atomic.Int32
Review Comment:
`atomic.Int32` requires a sufficiently new Go version (introduced after the
classic `atomic.AddInt32/LoadInt32` APIs). If this repository’s supported Go
version can be older, this will break builds. Consider using `var callCount
int32` with `atomic.AddInt32(&callCount, 1)` / `atomic.LoadInt32(&callCount)`
for broader compatibility, or confirm/align the module’s minimum Go version
accordingly.
##########
go-sdk/pkg/worker/runner_test.go:
##########
@@ -94,7 +95,7 @@ func (s *WorkerSuite) SetupSuite() {
s.worker = s.worker.WithHeartbeatInterval(100 *
time.Millisecond).WithClient(s.client)
}
-func (s *WorkerSuite) TearDownSuite() {
+func (s *WorkerSuite) TearDownTest() {
Review Comment:
The PR title/description focus on removing parallelism, but this also
changes the suite lifecycle semantics from suite-level to per-test. That can
materially change runtime and test behavior (fresh mocks/state each test,
expectations asserted per-test instead of once). Either (a) keep
`SetupSuite/TearDownSuite` and only remove `t.Parallel()`, or (b) update the PR
description/title to explicitly call out the additional isolation change and
why it’s needed to address the flake.
##########
go-sdk/pkg/worker/runner_test.go:
##########
@@ -21,6 +21,7 @@ import (
"context"
"fmt"
"log/slog"
+ "sync/atomic"
Review Comment:
`atomic.Int32` requires a sufficiently new Go version (introduced after the
classic `atomic.AddInt32/LoadInt32` APIs). If this repository’s supported Go
version can be older, this will break builds. Consider using `var callCount
int32` with `atomic.AddInt32(&callCount, 1)` / `atomic.LoadInt32(&callCount)`
for broader compatibility, or confirm/align the module’s minimum Go version
accordingly.
##########
go-sdk/pkg/worker/runner_test.go:
##########
@@ -215,8 +213,9 @@ func (s *WorkerSuite) TestTaskHeartbeatsWhileRunning() {
// Since we heartbeat every 100ms and run for 1 second, we should
expect 10 heartbeat calls. But allow +/-
// 1 due to timing imprecision
+ count := callCount.Load()
s.Assert().
- True(callCount <= 11 && callCount >= 9, fmt.Sprintf("Call count
of %d was not within the margin of error of 10+/-1", callCount))
+ True(count <= 11 && count >= 9, fmt.Sprintf("Call count of %d
was not within the margin of error of 10+/-1", count))
Review Comment:
Testify provides formatted assertion variants (e.g., `Truef`) so you don’t
have to eagerly build the error string with `fmt.Sprintf`. Switching to a
formatted assertion avoids unnecessary formatting work on passing runs and is
more idiomatic for Testify assertions.
```suggestion
Truef(count <= 11 && count >= 9, "Call count of %d was not
within the margin of error of 10+/-1", count)
```
##########
go-sdk/pkg/worker/runner_test.go:
##########
@@ -81,7 +82,7 @@ func TestWorkerSuite(t *testing.T) {
suite.Run(t, &WorkerSuite{})
}
-func (s *WorkerSuite) SetupSuite() {
+func (s *WorkerSuite) SetupTest() {
Review Comment:
The PR title/description focus on removing parallelism, but this also
changes the suite lifecycle semantics from suite-level to per-test. That can
materially change runtime and test behavior (fresh mocks/state each test,
expectations asserted per-test instead of once). Either (a) keep
`SetupSuite/TearDownSuite` and only remove `t.Parallel()`, or (b) update the PR
description/title to explicitly call out the additional isolation change and
why it’s needed to address the flake.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
