chore: remove deprecated API, simplify test infrastructure#307
Open
meling wants to merge 18 commits intofeature/299/update-all-callersfrom
Open
chore: remove deprecated API, simplify test infrastructure#307meling wants to merge 18 commits intofeature/299/update-all-callersfrom
meling wants to merge 18 commits intofeature/299/update-all-callersfrom
Conversation
This removes from generated code: - Manager type - NewManager function - NewConfiguration function And updates the NewConfig signature to enforce better type safety regarding which options are allowed and in which order.
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.
Contributor
 |
|
Overall Grade |
Security Reliability  Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Go |  |
Mar 25, 2026 7:56a.m. | Review ↗ |
| Shell |  |
Mar 25, 2026 7:56a.m. | Review ↗ |
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
1 participant
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