gorumstest: extract the test framework from the root package - #329
Open
meling wants to merge 7 commits into
Open
gorumstest: extract the test framework from the root package#329meling wants to merge 7 commits into
meling wants to merge 7 commits into
Conversation
The servers package holds the low-level machinery for starting test servers and dialing them: bufconn listeners in the default build, real TCP listeners under the integration build tag, and the shared start/stop bookkeeping. It defines its own ServerIface and does not import gorums, so it can be used both by gorums's own in-package tests and by a public test-helper package built on top of gorums without an import cycle.
The test framework lived in the root package, so every gorums user pulled in goleak and, in the default build, the bufconn dialer. It now lives in the gorumstest package, modeled on net/http/httptest, and the root package keeps only what has to be there: ServerIface, which the helpers accept, and TestQuorumCallError, which builds an otherwise unexported error type. The exported names lose their Test prefix, which the package name now carries: gorums.TestConfiguration becomes gorumstest.Config, gorums.TestNode becomes gorumstest.Node, and so on. WithPreConnect and Collect are new; the rest are moved unchanged. Tests in package gorums cannot import gorumstest without an import cycle, so inbound_manager_test.go and callopts_test.go carry small in-package counterparts built directly on internal/testutils/servers.
Contributor
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Go |  |
Aug 12, 2026 12:38p.m. | Review ↗ |
| Shell | |
Aug 12, 2026 12:38p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
meling
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request refactors Gorums’ testing utilities by moving the public test framework out of the main gorums package into a dedicated gorumstest package (httptest-style), while introducing a gorums-independent internal server harness to avoid import cycles for in-package (white-box) tests.
Changes:
- Moved test helpers (configs/nodes/servers/systems, timeouts, goleak, bufconn/TCP dialing) from
gorumsintogithub.com/relab/gorums/gorumstest. - Added
internal/testutils/serversas the low-level, gorums-independent server start/dial layer used by bothgorumstestand in-package tests. - Updated repository tests/docs to consume the new
gorumstestAPIs and removed the old shared testing files fromgorums.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| testing_shared.go | Removed legacy in-package test framework helpers. |
| testing_integration.go | Removed legacy integration-tag server setup and dial options. |
| system_test.go | Switched to gorumstest helpers for systems, contexts, and dial options. |
| server_testing.go | Introduced/kept gorums.ServerIface as the common server interface for test helpers. |
| server_test.go | Updated tests to use gorumstest.Node/Context/Closer/InsecureDialOptions. |
| rpc_test.go | Updated RPC tests to use gorumstest node/context and default server helpers. |
| quorumcall_test.go | Updated quorum-call tests/benchmarks to use gorumstest.Config/Range/SkipGoleak. |
| internal/testutils/servers/servers.go | Added gorums-independent server start/stop implementation. |
| internal/testutils/servers/integration.go | Added integration build-tag TCP-backed Start and dial options. |
| internal/testutils/servers/bufconn.go | Converted bufconn-backed server start/dial to servers package API. |
| internal/tests/unresponsive/unreponsive_test.go | Updated to use gorumstest.Node. |
| internal/tests/tls/tls_test.go | Updated to use gorumstest.Node while preserving custom TLS dial options. |
| internal/tests/ordering/order_test.go | Updated to use gorumstest.Node/Config. |
| internal/tests/oneway/oneway_test.go | Updated to use gorumstest.Config for server setup. |
| internal/tests/metadata/metadata_test.go | Updated to use gorumstest.Node. |
| internal/tests/correctable/correctable_test.go | Updated to use gorumstest.Config/Context. |
| internal/tests/config/config_test.go | Updated to use gorumstest.Config. |
| inbound_manager_test.go | Replaced dependency on old helpers with local helpers built on internal/testutils/servers (avoids gorumstest import cycle). |
| gorumstest/servers.go | Added public default/echo/stream test servers for gorumstest users. |
| gorumstest/options.go | Moved options plumbing into gorumstest and adapted types to gorums options. |
| gorumstest/gorumstest.go | Added the main gorumstest framework: Config/Node/Servers/Systems, dialing, timeouts, and cleanup helpers. |
| errors_testing.go | Kept a testing-oriented helper (TestQuorumCallError) in gorums. |
| correctable_test.go | Updated to use gorumstest config/context/server helpers. |
| config_test.go | Updated to use gorumstest dial options and closer. |
| cmd/protoc-gen-gorums/dev/generated_code_test.go | Updated generated-code tests to use gorumstest.Config/Context. |
| client_interceptor_test.go | Updated to use gorumstest.Config/Context in interceptor tests/benchmarks. |
| callopts_test.go | Added in-package equivalents for gorumstest.Systems/WaitUntil to avoid import cycle. |
| async_test.go | Updated async quorum-call tests/benchmarks to use gorumstest.Config/Context. |
| AGENTS.md | Updated contributor guidance to prefer gorumstest helpers and describe import-cycle constraints. |
Suppressed comments (1)
gorumstest/options.go:16
- The comment claims gorums option types "satisfy this interface" because they are "an alias for any", but gorums.DialOption/ServerOption/NodeListOption are not aliases (e.g., DialOption is a func type). This is misleading documentation for consumers of gorumstest.Option.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
actions/checkout v5 to v7, actions/setup-go v6 to v7, and golangci/golangci-lint-action v8 to v9. The golangci-lint-action bump is the one that matters: v9.0.0 is its node20 to node24 migration, which clears the runner deprecation warning. Its args and only-new-issues inputs are unchanged, so the job behaves the same. arduino/setup-protoc stays at v3.0.0. That is its newest tag and its default branch still declares node20, so its deprecation warning cannot be cleared by a version bump.
CI ran golangci-lint with no config file, so the linter set came from the tool defaults and errcheck was switched off with a command-line flag that make did not pass. Move both decisions into .golangci.yml so a local run and a CI run agree on what is checked. The staticcheck set is the golangci-lint default minus QF1008, which asks for the embedded field to be dropped from a selector. Gorums names the layer it is addressing in such selectors, and eliding it makes the call read as the outer type's own.
start signalled the accept loop's exit by sending on an unbuffered channel, so the goroutine blocked forever whenever Serve returned without a matching stop: an immediate Serve error, or a caller that never invokes the returned stop function. Closing is non-blocking and stop's receive still observes it.
The gorumstest helper that adapts between the two pointed readers at "the doc comment on gorums.ServerIface for why", which said nothing about it. Record the reason here: internal/testutils/servers cannot import gorums without an import cycle for gorums's own tests, so it cannot name this type and restates it instead.
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.
Extracts the test framework from the root package into
gorumstest, modeled onnet/http/httptest.The helpers lived in the root package, so every user of
github.com/relab/gorumspulled ingoleakand, in the default build, the bufconn dialer. They now live in their own package, and the root keeps only what has to be there:ServerIface, which the helpers accept, andTestQuorumCallError, which builds an otherwise unexported error type.The exported names lose their
Testprefix, which the package name now carries:gorums.TestConfigurationbecomesgorumstest.Config,gorums.TestNodebecomesgorumstest.Node.WithPreConnectandCollectare new; the rest are moved unchanged.Below it sits
internal/testutils/servers, which owns listener allocation and the start/stop bookkeeping. It defines its ownServerIfaceand imports no gorums, so both the public helpers and gorums's own in-package tests can use it without an import cycle. Tests inpackage gorumscannot importgorumstestfor that reason, so two of them carry small in-package counterparts.Verification:
go test ./... -count=2,go test -C examples ./...,go vet -tags=integration ./...,gofmt -l.First PR in a nine-part stack that re-cuts 334 commits of unmerged work into reviewable pieces.