chore: remove deprecated API, simplify test infrastructure#307
Merged
chore: remove deprecated API, simplify test infrastructure#307
Conversation
Contributor
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Go |  |
Mar 25, 2026 2:31p.m. | Review ↗ |
| Shell | |
Mar 25, 2026 2:31p.m. | Review ↗ |
meling
force-pushed
the
feature/299/update-all-callers
branch
from
50924a6 to
8a55f2c
Compare
Changes NewConfig(opts ...Option) to NewConfig(nodes NodeListOption, opts ...DialOption), making the required NodeListOption explicit and aligning with standard Go API conventions (required args first). Additional changes: - Add Configuration.Close() error for closing connections without accessing the internal Manager directly. - Add unexported Configuration.mgr() used by Extend and Add internally. - Deprecate Configuration.Manager() in favor of Configuration.Close(). - Deprecate NewConfiguration; callers should use NewConfig instead. - Remove Manager, NewManager, NewConfiguration from generated code template; update NewConfig wrapper to the new signature. - Remove Manager from the reservedIdents list. NOTE: Existing generated files still call the old gorums.NewConfig signature and will fail to compile until regenerated in commit 6. Closes #298. Related to #294.
Update non-generated callers to use the new NewConfig(nodes, opts...) API: - config.go: close manager on error in NewConfig to prevent resource leak - system.go: use cfg (Configuration) as io.Closer directly instead of cfg.Manager() - system_test.go: use cfg.Close() instead of cfg.Manager().Close() - testopts.go: rename existingMgr to existingCfg, update WithManager to accept Configuration instead of *Manager; update shouldSkipGoleak and type switch - testing_bufconn.go: use existingCfg.mgr() in getOrCreateManager - testing_integration.go: use existingCfg.mgr() in getOrCreateManager - config_test.go: replace NewManager+NewConfiguration with NewConfig; remove redundant mgr.Size() checks - server_test.go: use NewConfig in TestTCPReconnection - internal/tests/config/config_test.go: use WithManager(t, c1) instead of WithManager(t, c1.Manager()) Update template static code to remove Manager/NewManager/NewConfiguration wrappers and update NewConfig signature in generated code: - cmd/protoc-gen-gorums/dev/aliases.go: remove Manager type alias, NewManager and NewConfiguration functions; update NewConfig to take (nodes NodeListOption, opts ...DialOption) - cmd/protoc-gen-gorums/gengorums/template_static.go: regenerate from aliases.go
This removes the Manager alias.
Add TestDialOptions(t testing.TB) DialOption with two build-tag implementations: bufconn mode (testing_bufconn.go) creates an indirect in-memory dialer via globalBufconnRegistry; integration mode (testing_integration.go) delegates to InsecureDialOptions. Move getOrCreateManager from both build-tag files into testing_shared.go as a single implementation that calls TestDialOptions(t); removes the duplicated setup logic. TestManager is updated to reflect the unified approach. Replace NewConfiguration(mgr, ...) in TestConfiguration with the internal newConfig(mgr) call, removing the last test helper use of the deprecated NewConfiguration function.
Convert all TestManager + NewConfiguration pairs in inbound_manager_test.go to use NewConfig(WithNodeList(addrs), TestDialOptions(t), ...) directly. Change connectAsPeer and connectAsPeerClient return types from *outboundManager to Configuration; callers that previously called mgr.Close() now call cfg.Close(). Cleanup is registered via t.Cleanup in the helpers, consistent with the rest of the test suite.
Replace all withRequestHandler calls in inbound_manager_test.go with the public WithServer API. In TestKnownPeerServerCallsClient, the mockRequestHandler type is removed and replaced with a real *Server with RegisterHandler calls, which is the idiomatic way to set up a back-channel handler. Update doc comments in connectAsPeerClient to reference WithServer instead of withRequestHandler.
Replace the stale gorums.NewManager example in the TestServers doc comment with the current pattern using gorums.NewConfig together with gorums.TestDialOptions. Rename mgrOption to dialOption and mgrNodes to cfgNodes.
Replace all NewManager + NewConfiguration pairs with gorums.NewConfig. Update prose in the "Implementing the StorageClient" section to no longer describe the Manager type, since it is unexported; the entry point is now NewConfig. Update "Working with Configurations" example to use the current Configuration methods: Extend, Union, Difference, Remove, and WithoutErrors as direct method calls (returns Configuration, no error) instead of the old NewConfiguration wrapper pattern. Replace the removed methods And, Except, WithoutNodes, and WithNewNodes with their current equivalents. Fixed nil return for error cases in WriteNestedMulticast.
This avoids the need for WithManager to create new configurations using the same manager; instead we create one large configuration and remove from it to create other configurations using Union and Difference.
Remove WithManager, existingCfg field, and getOrCreateManager from the testing infrastructure. TestConfiguration now calls NewConfig and registers t.Cleanup(Closer(t, cfg)) using the returned configuration, which owns and closes its manager internally.
meling
force-pushed
the
chore/300/regenerate-gorums-proto
branch
from
1f958ec to
d915c9c
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR completes the post-refactor cleanup by removing the remaining deprecated “Manager”-based outbound API and aligning both the test infrastructure and documentation around NewConfig + DialOptions.
Changes:
- Remove deprecated public symbols (
NewManager,NewConfiguration,Manager/ManagerOptionaliases) and related compatibility plumbing. - Simplify test setup by eliminating config/manager reuse helpers and introducing
TestDialOptions(t)(build-tag-aware) for tests constructing configs directly. - Update tests and docs to use
NewConfig,WithServer, and the configuration composition methods (Union,Difference,Remove, etc.).
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
testopts.go |
Removes WithManager/existingCfg test option path and updates option extraction behavior. |
testing_shared.go |
Switches TestConfiguration to build configs via NewConfig + TestDialOptions(t) and registers config cleanup. |
testing_integration.go |
Replaces manager-creation helper with integration-tag TestDialOptions(t) implementation. |
testing_bufconn.go |
Adds non-integration TestDialOptions(t) using bufconn registry lookup at dial time. |
server_test.go |
Updates test call sites to use DialOption naming. |
opts.go |
Removes deprecated ManagerOption alias. |
node.go |
Updates comments around node manager ownership (back-compat wording removed). |
mgr.go |
Removes deprecated Manager alias and NewManager constructor and old manager helper methods. |
internal/tests/config/config_test.go |
Stops relying on manager reuse; composes derived configs from c1 using config methods. |
inbound_manager_test.go |
Migrates peer-connection helpers to return Configuration and replaces request handler injection with WithServer. |
examples/storage/repl.go |
Renames local vars to reflect Configuration usage. |
doc/user-guide.md |
Updates narrative and code examples to use gorums.NewConfig and configuration methods instead of Manager APIs. |
config_test.go |
Renames test TestNewConfiguration → TestNewConfig. |
config.go |
Removes NewConfiguration and Configuration.Manager(); updates docs and error strings; keeps NewConfig as the constructor. |
Comments suppressed due to low confidence (1)
doc/user-guide.md:608
- In the Storage client example,
cfgCtx := config.Context(ctx)references an undefined identifier (config). It looks like this should use thecfgvariable created above (e.g.,cfg.Context(ctx)), otherwise the snippet won’t compile as shown.
defer cfg.Close()
ctx := context.Background()
cfgCtx := config.Context(ctx)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This was referenced Mar 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Final cleanup of the API surface after the stacked refactors in #295–#299.
Removes all remaining deprecated symbols and simplifies the test infrastructure:
NewManager(useNewConfiginstead)NewConfiguration(useNewConfiginstead)Managertype alias (was= outboundManager)WithManagertest option andexistingCfgfield fromtestOptionsgetOrCreateManagerhelper;TestConfigurationnow callsNewConfigdirectly and registerst.Cleanup(Closer(t, cfg))TestDialOptions(t) DialOption(build-tag-aware) for use in tests that callNewConfigdirectlywithRequestHandler(internal) withWithServerin all test filesTestNewConfiguration→TestNewConfigdoc/user-guide.md, and stale backward-compatibility comments innode.go*_gorums.pb.gofiles to removeManagerwrappers from generated templatesRelated
Part of #294
Closes #300