You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "weibangpeng (via GitHub)" <gi...@apache.org> on 2026/06/16 06:39:10 UTC
[PR] Add unit tests to Go harness package (issue #21386) [beam]
weibangpeng opened a new pull request, #38979:
URL: https://github.com/apache/beam/pull/38979
## Summary
Increases unit test coverage for the Go SDK harness package from **43% → 50.7%**, addressing issue #21386.
## Changes
Adds ~30 test functions across 7 files (1 new, 6 modified):
- `harness_test.go` — `TestFail`, `TestControl_MetStoreToString`, `TestControl_GetPlanOrResponse` (5 scenarios)
- `logging_test.go` — `TestConvertSeverity` (6 cases)
- `monitoring_test.go` — `TestMonitoring_NilStore`, `TestGetNextShortID`
- `worker_status_test.go` — `TestIsAliveAndShutdown`, `TestMemoryUsage`, `TestGoroutineDump`, `TestBuildInfo`, `TestActiveProcessBundleStates`, `TestCacheStats`
- `datamgr_test.go` — ScopedDataManager cached-port paths, elementsChan lifecycle, terminateStreamOnError, closeInstruction
- `statemgr_test.go` — ScopedStateReader construction/close/closed-path guards
- **`sampler_test.go` (new)** — stateSampler lifecycle (new, stop, start via context cancel and done channel)
## Test plan
All tests pass:
```
ok github.com/apache/beam/sdks/v2/go/pkg/beam/core/runtime/harness coverage: 50.7% of statements
```
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add unit tests to Go harness package (issue #21386) [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38979:
URL: https://github.com/apache/beam/pull/38979#issuecomment-6124993707
This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add unit tests to Go harness package (issue #21386) [beam]
Posted by "jrmccluskey (via GitHub)" <gi...@apache.org>.
jrmccluskey commented on code in PR #38979:
URL: https://github.com/apache/beam/pull/38979#discussion_r3723781857
##########
sdks/go/pkg/beam/core/runtime/harness/sampler_test.go:
##########
@@ -0,0 +1,80 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package harness
+
+import (
+ "context"
+ "testing"
+ "time"
+
+ "github.com/apache/beam/sdks/v2/go/pkg/beam/core/metrics"
+)
+
+func TestNewSampler(t *testing.T) {
+ ctx := metrics.SetBundleID(context.Background(), "test-bundle")
+ store := metrics.GetStore(ctx)
+ if store == nil {
+ t.Fatal("GetStore returned nil")
+ }
+
+ s := newSampler(store, 0)
+ if s == nil {
+ t.Fatal("newSampler returned nil")
+ }
+ if s.done == nil {
+ t.Error("sampler done channel is nil")
+ }
+}
+
+func TestStateSampler_Stop(t *testing.T) {
+ ctx := metrics.SetBundleID(context.Background(), "test-bundle")
+ store := metrics.GetStore(ctx)
+ s := newSampler(store, 0)
+ // stop should not panic when called on a properly initialized sampler.
+ s.stop()
+}
+
+func TestStateSampler_Start_ContextCancel(t *testing.T) {
+ ctx := metrics.SetBundleID(context.Background(), "test-bundle")
+ store := metrics.GetStore(ctx)
+ s := newSampler(store, 0)
+
+ cancelCtx, cancel := context.WithCancel(ctx)
+ cancel() // Cancel immediately
+
+ // start should return nil immediately when context is already canceled.
+ err := s.start(cancelCtx, samplePeriod)
+ if err != nil {
+ t.Errorf("start returned error on canceled context: %v", err)
+ }
Review Comment:
+1
##########
sdks/go/pkg/beam/core/runtime/harness/harness_test.go:
##########
@@ -258,3 +277,112 @@ func TestElementProcessingTimeoutParsing(t *testing.T) {
}
}
}
+
+func TestControl_MetStoreToString(t *testing.T) {
+ ctx := metrics.SetBundleID(context.Background(), "test-bundle")
+ store := metrics.GetStore(ctx)
+ if store == nil {
+ t.Fatal("GetStore returned nil")
+ }
+ ctrl := &control{
+ metStore: map[instructionID]*metrics.Store{
+ "inst1": store,
+ },
+ }
+ b := &strings.Builder{}
+ ctrl.metStoreToString(b)
+ out := b.String()
+ if !strings.Contains(out, "Bundle ID: inst1") {
+ t.Errorf("metStoreToString output missing bundle ID, got: %s", out)
+ }
+}
+
+func TestControl_GetPlanOrResponse(t *testing.T) {
+ tests := []struct {
+ name string
+ active map[instructionID]*exec.Plan
+ awaitFinalize map[instructionID]awaitingFinalization
+ failed map[instructionID]error
+ inactive circleBuffer
+ wantErr bool // response has Error field set (non-nil response)
+ wantNilPlan bool // response is non-nil and plan is nil -> response is returned
+ wantEmpty bool // both plan and response are nil - empty response needed
+ }{
+ {
+ name: "active",
+ active: map[instructionID]*exec.Plan{
+ "ref": {},
+ },
+ },
+ {
+ name: "awaitingFinalization",
+ awaitFinalize: map[instructionID]awaitingFinalization{
+ "ref": {plan: &exec.Plan{}},
+ },
+ },
+ {
+ name: "failed",
+ failed: map[instructionID]error{"ref": fmt.Errorf("test failure")},
+ wantErr: true,
+ },
+ {
+ name: "inactive",
+ inactive: func() circleBuffer {
+ c := newCircleBuffer()
+ c.Add("ref")
+ return c
+ }(),
+ wantEmpty: true,
+ },
+ {
+ name: "notFound",
+ wantErr: true,
+ },
+ }
+
+ for _, test := range tests {
+ t.Run(test.name, func(t *testing.T) {
+ ctrl := &control{
+ active: make(map[instructionID]*exec.Plan),
+ awaitingFinalization: make(map[instructionID]awaitingFinalization),
+ failed: make(map[instructionID]error),
+ inactive: newCircleBuffer(),
+ }
+ if test.active != nil {
+ ctrl.active = test.active
+ }
+ if test.awaitFinalize != nil {
+ ctrl.awaitingFinalization = test.awaitFinalize
+ }
+ if test.failed != nil {
+ ctrl.failed = test.failed
+ }
+ if len(test.inactive.buf) > 0 {
+ ctrl.inactive = test.inactive
+ }
Review Comment:
+1
##########
sdks/go/pkg/beam/core/runtime/harness/harness_test.go:
##########
@@ -166,6 +167,24 @@ func TestControl_getOrCreatePlan(t *testing.T) {
}
+func TestFail(t *testing.T) {
+ ctx := context.Background()
+ resp := fail(ctx, "test-id", "error %s %d", "msg", 42)
+
+ if resp == nil {
+ t.Fatal("fail returned nil")
+ }
+ if got, want := resp.GetInstructionId(), "test-id"; got != want {
Review Comment:
A check like this is somewhat inherently unhelpful since `fail()` is written to always instantiate an `InstructionResponse` struct, it will _never_ be `nil`. Checking the contents for correctness is valid afterwards though
##########
sdks/go/pkg/beam/core/runtime/harness/statemgr_test.go:
##########
@@ -508,3 +510,99 @@ func contains(got, want error) bool {
}
return strings.Contains(got.Error(), want.Error())
}
+
+func TestNewScopedStateReader(t *testing.T) {
+ mgr := &StateChannelManager{}
+ s := NewScopedStateReader(mgr, "inst1")
+ if s == nil {
+ t.Fatal("NewScopedStateReader returned nil")
+ }
+ if s.mgr != mgr {
+ t.Error("mgr field not set")
+ }
+ if s.instID != "inst1" {
+ t.Errorf("instID = %v, want inst1", s.instID)
+ }
+ if s.cache != nil {
+ t.Error("cache should be nil for NewScopedStateReader")
+ }
+}
+
+func TestNewScopedStateReaderWithCache(t *testing.T) {
+ mgr := &StateChannelManager{}
+ cache := &statecache.SideInputCache{}
+ cache.Init(1)
+ s := NewScopedStateReaderWithCache(mgr, "inst2", cache)
+ if s == nil {
+ t.Fatal("NewScopedStateReaderWithCache returned nil")
+ }
+ if s.mgr != mgr {
+ t.Error("mgr field not set")
+ }
+ if s.instID != "inst2" {
+ t.Errorf("instID = %v, want inst2", s.instID)
+ }
+ if s.cache != cache {
+ t.Error("cache field not set correctly")
+ }
+}
+
+func TestScopedStateReader_GetSideInputCache(t *testing.T) {
+ cache := &statecache.SideInputCache{}
+ cache.Init(1)
+ s := NewScopedStateReaderWithCache(&StateChannelManager{}, "inst", cache)
+ got := s.GetSideInputCache()
+ if got != cache {
+ t.Error("GetSideInputCache returned wrong cache")
+ }
+}
+
+func TestScopedStateReader_Close(t *testing.T) {
+ mgr := &StateChannelManager{}
+ s := NewScopedStateReader(mgr, "inst")
+ err := s.Close()
+ if err != nil {
+ t.Errorf("Close returned error: %v", err)
+ }
+ if !s.closed {
+ t.Error("Close did not set closed flag")
+ }
+ // Second close should not error but should no-op.
+ err = s.Close()
+ if err != nil {
+ t.Errorf("Second Close returned error: %v", err)
+ }
+}
+
+func TestStateChannelManager_PortsInit(t *testing.T) {
+ mgr := &StateChannelManager{}
+ if mgr.ports != nil {
+ t.Error("ports should be nil before first Open call")
+ }
+ // Force-initialize ports by attempting Open; even if it fails, the map should be created.
+ // Restore: just set it directly since we don't want to dial.
+ mgr.mu.Lock()
+ mgr.ports = make(map[string]*StateChannel)
+ mgr.mu.Unlock()
+ if mgr.ports == nil {
+ t.Error("ports map should exist after initialization")
+ }
Review Comment:
+1
##########
sdks/go/pkg/beam/core/runtime/harness/worker_status_test.go:
##########
@@ -94,3 +94,74 @@ func TestSendStatusResponse(t *testing.T) {
t.Error(err)
}
}
+
+func TestWorkerStatusHandler_IsAliveAndShutdown(t *testing.T) {
+ w := &workerStatusHandler{shouldShutdown: 0}
+ if !w.isAlive() {
+ t.Error("isAlive: expected true after init")
+ }
+ w.shutdown()
+ if w.isAlive() {
+ t.Error("isAlive: expected false after shutdown")
+ }
+}
+
+func TestMemoryUsage(t *testing.T) {
+ b := &strings.Builder{}
+ memoryUsage(b)
+ out := b.String()
+ required := []string{"heap in-use-spans", "stack in-use-spans", "GC-CPU percentage", "Last GC time", "Next GC"}
+ for _, r := range required {
+ if !strings.Contains(out, r) {
+ t.Errorf("memoryUsage output missing %q, got:\n%s", r, out)
+ }
+ }
+}
+
+func TestGoroutineDump(t *testing.T) {
+ b := &strings.Builder{}
+ goroutineDump(b)
+ out := b.String()
+ if !strings.Contains(out, "goroutine") {
+ t.Errorf("goroutineDump output missing 'goroutine', got:\n%s", out)
+ }
+}
+
+func TestBuildInfo(t *testing.T) {
+ b := &strings.Builder{}
+ buildInfo(b)
+ out := b.String()
+ if !strings.Contains(out, "Build Info") {
+ t.Errorf("buildInfo output missing 'Build Info', got:\n%s", out)
+ }
+}
Review Comment:
These aren't really testing interesting functionality here, just looking for the hard-coded headers from the functions. It doesn't feel particularly useful to have unit tests that will break if the string we write during the dump changes.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add unit tests to Go harness package (issue #21386) [beam]
Posted by "gemini-code-assist[bot] (via GitHub)" <gi...@apache.org>.
gemini-code-assist[bot] commented on code in PR #38979:
URL: https://github.com/apache/beam/pull/38979#discussion_r3418629142
##########
sdks/go/pkg/beam/core/runtime/harness/statemgr_test.go:
##########
@@ -508,3 +510,99 @@ func contains(got, want error) bool {
}
return strings.Contains(got.Error(), want.Error())
}
+
+func TestNewScopedStateReader(t *testing.T) {
+ mgr := &StateChannelManager{}
+ s := NewScopedStateReader(mgr, "inst1")
+ if s == nil {
+ t.Fatal("NewScopedStateReader returned nil")
+ }
+ if s.mgr != mgr {
+ t.Error("mgr field not set")
+ }
+ if s.instID != "inst1" {
+ t.Errorf("instID = %v, want inst1", s.instID)
+ }
+ if s.cache != nil {
+ t.Error("cache should be nil for NewScopedStateReader")
+ }
+}
+
+func TestNewScopedStateReaderWithCache(t *testing.T) {
+ mgr := &StateChannelManager{}
+ cache := &statecache.SideInputCache{}
+ cache.Init(1)
+ s := NewScopedStateReaderWithCache(mgr, "inst2", cache)
+ if s == nil {
+ t.Fatal("NewScopedStateReaderWithCache returned nil")
+ }
+ if s.mgr != mgr {
+ t.Error("mgr field not set")
+ }
+ if s.instID != "inst2" {
+ t.Errorf("instID = %v, want inst2", s.instID)
+ }
+ if s.cache != cache {
+ t.Error("cache field not set correctly")
+ }
+}
+
+func TestScopedStateReader_GetSideInputCache(t *testing.T) {
+ cache := &statecache.SideInputCache{}
+ cache.Init(1)
+ s := NewScopedStateReaderWithCache(&StateChannelManager{}, "inst", cache)
+ got := s.GetSideInputCache()
+ if got != cache {
+ t.Error("GetSideInputCache returned wrong cache")
+ }
+}
+
+func TestScopedStateReader_Close(t *testing.T) {
+ mgr := &StateChannelManager{}
+ s := NewScopedStateReader(mgr, "inst")
+ err := s.Close()
+ if err != nil {
+ t.Errorf("Close returned error: %v", err)
+ }
+ if !s.closed {
+ t.Error("Close did not set closed flag")
+ }
+ // Second close should not error but should no-op.
+ err = s.Close()
+ if err != nil {
+ t.Errorf("Second Close returned error: %v", err)
+ }
+}
+
+func TestStateChannelManager_PortsInit(t *testing.T) {
+ mgr := &StateChannelManager{}
+ if mgr.ports != nil {
+ t.Error("ports should be nil before first Open call")
+ }
+ // Force-initialize ports by attempting Open; even if it fails, the map should be created.
+ // Restore: just set it directly since we don't want to dial.
+ mgr.mu.Lock()
+ mgr.ports = make(map[string]*StateChannel)
+ mgr.mu.Unlock()
+ if mgr.ports == nil {
+ t.Error("ports map should exist after initialization")
+ }
Review Comment:
This test does not actually verify any production code behavior. It manually initializes `mgr.ports` inside the test itself (lines 584-586) and then asserts that `mgr.ports` is not nil. This only tests that Go's `make` function works, rather than testing that `StateChannelManager` initializes its ports map during actual usage (e.g., when `Open` is called). Consider triggering the actual initialization logic (e.g., by calling `Open` with an invalid port and ignoring the dial error) to ensure the production initialization path is covered.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add unit tests to Go harness package (issue #21386) [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38979:
URL: https://github.com/apache/beam/pull/38979#issuecomment-4716377871
Assigning reviewers:
R: @lostluck for label go.
Note: If you would like to opt out of this review, comment `assign to next reviewer`.
Available commands:
- `stop reviewer notifications` - opt out of the automated review tooling
- `remind me after tests pass` - tag the comment author after tests pass
- `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
The PR bot will only process comments in the main thread (not review comments).
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add unit tests to Go harness package (issue #21386) [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38979:
URL: https://github.com/apache/beam/pull/38979#issuecomment-5143027125
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment `assign to next reviewer`:
R: @jrmccluskey for label go.
Available commands:
- `stop reviewer notifications` - opt out of the automated review tooling
- `remind me after tests pass` - tag the comment author after tests pass
- `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add unit tests to Go harness package (issue #21386) [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38979:
URL: https://github.com/apache/beam/pull/38979#issuecomment-5034131416
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment `assign to next reviewer`:
R: @shunping for label go.
Available commands:
- `stop reviewer notifications` - opt out of the automated review tooling
- `remind me after tests pass` - tag the comment author after tests pass
- `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add unit tests to Go harness package (issue #21386) [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38979:
URL: https://github.com/apache/beam/pull/38979#issuecomment-4914911976
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment `assign to next reviewer`:
R: @lostluck for label go.
Available commands:
- `stop reviewer notifications` - opt out of the automated review tooling
- `remind me after tests pass` - tag the comment author after tests pass
- `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add unit tests to Go harness package (issue #21386) [beam]
Posted by "gemini-code-assist[bot] (via GitHub)" <gi...@apache.org>.
gemini-code-assist[bot] commented on PR #38979:
URL: https://github.com/apache/beam/pull/38979#issuecomment-4715695157
## Summary of Changes
Hello, I'm Gemini Code Assist[^1]! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request focuses on enhancing the reliability and maintainability of the Go SDK harness package by adding a substantial number of unit tests. By covering edge cases in data and state management, as well as monitoring and logging utilities, these changes ensure that the harness behaves correctly under various conditions, ultimately leading to higher code quality and safer future development.
### Highlights
* **Increased Test Coverage**: Significantly improved unit test coverage for the Go SDK harness package, raising it from 43% to 50.7%.
* **New Test Suite**: Introduced a new test file, `sampler_test.go`, to validate the lifecycle and state management of the sampler.
* **Expanded Coverage across Core Components**: Added comprehensive unit tests for `datamgr`, `statemgr`, `worker_status`, `logging`, and `monitoring` components to ensure robust error handling and state transitions.
<details>
<summary><b>New Features</b></summary>
🧠 You can now enable **Memory** (public preview) to help **Gemini Code Assist** learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. **Click [here](https://codeassist.google/code-review/login) to enable Memory in your admin console.**
</details>
<details>
<summary><b>Using Gemini Code Assist</b></summary>
<br>
The full guide for Gemini Code Assist can be found on our [documentation page](https://developers.google.com/gemini-code-assist/docs/review-github-code), here are some quick tips.
<b>Invoking Gemini</b>
You can request assistance from Gemini at any point by creating a comment using either `/gemini <command>` or `@gemini-code-assist <command>`. Below is a summary of the supported commands on the current page.
Feature | Command | Description
--- | --- | ---
Code Review | `/gemini review` | Performs a code review for the current pull request in its current state.
Pull Request Summary | `/gemini summary` | Provides a summary of the current pull request in its current state.
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help | `/gemini help` | Displays a list of available commands.
<b>Customization</b>
To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a `.gemini/` folder in the base of the repository. Detailed instructions can be found [here](https://developers.google.com/gemini-code-assist/docs/customize-gemini-behavior-github).
<b>Limitations & Feedback</b>
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with :thumbsup: and :thumbsdown: on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up [here](https://google.qualtrics.com/jfe/form/SV_2cyuGuTWsEw84yG).
</details>
[^1]: Review the [Privacy Notices](https://policies.google.com/privacy), [Generative AI Prohibited Use Policy](https://policies.google.com/terms/generative-ai/use-policy), [Terms of Service](https://policies.google.com/terms), and learn how to configure Gemini Code Assist in GitHub [here](https://developers.google.com/gemini-code-assist/docs/customize-gemini-behavior-github). Gemini can make mistakes, so double check it and [use code with caution](https://support.google.com/legal/answer/13505487).
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add unit tests to Go harness package (issue #21386) [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #38979: Add unit tests to Go harness package (issue #21386)
URL: https://github.com/apache/beam/pull/38979
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add unit tests to Go harness package (issue #21386) [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38979:
URL: https://github.com/apache/beam/pull/38979#issuecomment-5104338304
Reminder, please take a look at this pr: @shunping
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add unit tests to Go harness package (issue #21386) [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38979:
URL: https://github.com/apache/beam/pull/38979#issuecomment-4809803223
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment `assign to next reviewer`:
R: @shunping for label go.
Available commands:
- `stop reviewer notifications` - opt out of the automated review tooling
- `remind me after tests pass` - tag the comment author after tests pass
- `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add unit tests to Go harness package (issue #21386) [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38979:
URL: https://github.com/apache/beam/pull/38979#issuecomment-4882014491
Reminder, please take a look at this pr: @shunping
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add unit tests to Go harness package (issue #21386) [beam]
Posted by "gemini-code-assist[bot] (via GitHub)" <gi...@apache.org>.
gemini-code-assist[bot] commented on code in PR #38979:
URL: https://github.com/apache/beam/pull/38979#discussion_r3418629130
##########
sdks/go/pkg/beam/core/runtime/harness/sampler_test.go:
##########
@@ -0,0 +1,80 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package harness
+
+import (
+ "context"
+ "testing"
+ "time"
+
+ "github.com/apache/beam/sdks/v2/go/pkg/beam/core/metrics"
+)
+
+func TestNewSampler(t *testing.T) {
+ ctx := metrics.SetBundleID(context.Background(), "test-bundle")
+ store := metrics.GetStore(ctx)
+ if store == nil {
+ t.Fatal("GetStore returned nil")
+ }
+
+ s := newSampler(store, 0)
+ if s == nil {
+ t.Fatal("newSampler returned nil")
+ }
+ if s.done == nil {
+ t.Error("sampler done channel is nil")
+ }
+}
+
+func TestStateSampler_Stop(t *testing.T) {
+ ctx := metrics.SetBundleID(context.Background(), "test-bundle")
+ store := metrics.GetStore(ctx)
+ s := newSampler(store, 0)
+ // stop should not panic when called on a properly initialized sampler.
+ s.stop()
+}
+
+func TestStateSampler_Start_ContextCancel(t *testing.T) {
+ ctx := metrics.SetBundleID(context.Background(), "test-bundle")
+ store := metrics.GetStore(ctx)
+ s := newSampler(store, 0)
+
+ cancelCtx, cancel := context.WithCancel(ctx)
+ cancel() // Cancel immediately
+
+ // start should return nil immediately when context is already canceled.
+ err := s.start(cancelCtx, samplePeriod)
+ if err != nil {
+ t.Errorf("start returned error on canceled context: %v", err)
+ }
Review Comment:
In Go, functions that block on context cancellation typically return `ctx.Err()` (which is `context.Canceled` in this case). If `s.start` follows this standard pattern, then `err` will be non-nil, causing this test to fail. Please verify if `s.start` returns `context.Canceled` on cancellation, and if so, update the assertion to allow or explicitly assert `context.Canceled` using `errors.Is(err, context.Canceled)`.
##########
sdks/go/pkg/beam/core/runtime/harness/harness_test.go:
##########
@@ -258,3 +277,112 @@ func TestElementProcessingTimeoutParsing(t *testing.T) {
}
}
}
+
+func TestControl_MetStoreToString(t *testing.T) {
+ ctx := metrics.SetBundleID(context.Background(), "test-bundle")
+ store := metrics.GetStore(ctx)
+ if store == nil {
+ t.Fatal("GetStore returned nil")
+ }
+ ctrl := &control{
+ metStore: map[instructionID]*metrics.Store{
+ "inst1": store,
+ },
+ }
+ b := &strings.Builder{}
+ ctrl.metStoreToString(b)
+ out := b.String()
+ if !strings.Contains(out, "Bundle ID: inst1") {
+ t.Errorf("metStoreToString output missing bundle ID, got: %s", out)
+ }
+}
+
+func TestControl_GetPlanOrResponse(t *testing.T) {
+ tests := []struct {
+ name string
+ active map[instructionID]*exec.Plan
+ awaitFinalize map[instructionID]awaitingFinalization
+ failed map[instructionID]error
+ inactive circleBuffer
+ wantErr bool // response has Error field set (non-nil response)
+ wantNilPlan bool // response is non-nil and plan is nil -> response is returned
+ wantEmpty bool // both plan and response are nil - empty response needed
+ }{
+ {
+ name: "active",
+ active: map[instructionID]*exec.Plan{
+ "ref": {},
+ },
+ },
+ {
+ name: "awaitingFinalization",
+ awaitFinalize: map[instructionID]awaitingFinalization{
+ "ref": {plan: &exec.Plan{}},
+ },
+ },
+ {
+ name: "failed",
+ failed: map[instructionID]error{"ref": fmt.Errorf("test failure")},
+ wantErr: true,
+ },
+ {
+ name: "inactive",
+ inactive: func() circleBuffer {
+ c := newCircleBuffer()
+ c.Add("ref")
+ return c
+ }(),
+ wantEmpty: true,
+ },
+ {
+ name: "notFound",
+ wantErr: true,
+ },
+ }
+
+ for _, test := range tests {
+ t.Run(test.name, func(t *testing.T) {
+ ctrl := &control{
+ active: make(map[instructionID]*exec.Plan),
+ awaitingFinalization: make(map[instructionID]awaitingFinalization),
+ failed: make(map[instructionID]error),
+ inactive: newCircleBuffer(),
+ }
+ if test.active != nil {
+ ctrl.active = test.active
+ }
+ if test.awaitFinalize != nil {
+ ctrl.awaitingFinalization = test.awaitFinalize
+ }
+ if test.failed != nil {
+ ctrl.failed = test.failed
+ }
+ if len(test.inactive.buf) > 0 {
+ ctrl.inactive = test.inactive
+ }
Review Comment:
Instead of checking the length of the unexported `buf` field of `circleBuffer` to decide whether to assign it, you can directly assign `ctrl.inactive = test.inactive` unconditionally. The zero-value of `circleBuffer` is safe to assign and this avoids coupling the test runner to the internal implementation details of `circleBuffer`.
```go
ctrl.inactive = test.inactive
```
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add unit tests to Go harness package (issue #21386) [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38979:
URL: https://github.com/apache/beam/pull/38979#issuecomment-4779590097
Reminder, please take a look at this pr: @lostluck
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add unit tests to Go harness package (issue #21386) [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38979:
URL: https://github.com/apache/beam/pull/38979#issuecomment-5994851462
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@beam.apache.org list. Thank you for your contributions.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add unit tests to Go harness package (issue #21386) [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38979:
URL: https://github.com/apache/beam/pull/38979#issuecomment-4991994563
Reminder, please take a look at this pr: @lostluck
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org