Add unit tests to Go harness package (issue #21386)#38979
Conversation
Adds ~30 test functions covering previously untested code paths: - control.metStoreToString, control.getPlanOrResponse (5 scenarios) - convertSeverity (6 cases), monitoring nil-store path - stateSampler lifecycle (new, start via context cancel / done channel, stop) - ScopedDataManager cached-port paths (OpenWrite, OpenElementChan, OpenTimerWrite) - elementsChan lifecycle (InstructionEnded, Closed, PTransformDone) - ScopedStateReader construction, close, and closed-path guards - workerStatusHandler isAlive/shutdown, memoryUsage, goroutineDump, buildInfo, cacheStats - shortIDCache.getNextShortID Closes apache#21386
Summary of ChangesHello, I'm Gemini Code Assist1! 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
New Features🧠 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 to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization 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 Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 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. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive suite of unit tests for several core components of the Go SDK harness, including the data manager, control plane, logging, monitoring, state manager, sampler, and worker status handler. The feedback suggests improving the tests by directly assigning the inactive field in getPlanOrResponse tests to avoid coupling with internal details, properly asserting standard context.Canceled errors in the sampler tests, and refactoring the state channel manager initialization test to cover actual production code paths instead of mocking its own assertions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if len(test.inactive.buf) > 0 { | ||
| ctrl.inactive = test.inactive | ||
| } |
There was a problem hiding this 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.
ctrl.inactive = test.inactive| // 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) | ||
| } |
There was a problem hiding this 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).
| // 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") | ||
| } |
There was a problem hiding this 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.
|
Assigning reviewers: R: @lostluck for label go. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
1 participant
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,TestGetNextShortIDworker_status_test.go—TestIsAliveAndShutdown,TestMemoryUsage,TestGoroutineDump,TestBuildInfo,TestActiveProcessBundleStates,TestCacheStatsdatamgr_test.go— ScopedDataManager cached-port paths, elementsChan lifecycle, terminateStreamOnError, closeInstructionstatemgr_test.go— ScopedStateReader construction/close/closed-path guardssampler_test.go(new) — stateSampler lifecycle (new, stop, start via context cancel and done channel)Test plan
All tests pass: