hyperledger / hyperledger/fabric-x-sdk
Test coverage for network/fabric and network/fabricx
- Dominant language
- Go
- Stars
- 3
- Forks
- 5
- Avg merge
- 2d 11h
- Merged PRs (30d)
- 9
Description
## Context
`network/fabric` has no direct test file. `network/fabricx/packager.go`'s pure
proto-transformation functions (`PackageTx`/`CreateTx`/`NewTxPackager`) are also untested.
Separately, `fabrictest` (the in-memory fake network used throughout the integration suite)
doesn't implement the committer's notification service, so `TestFabric`/`TestFabricX` skip their
`Notifications`/`FinalityListener` cases — only a real Fabric-X committer (`make start-x test-x`)
exercises that path today.
These are three independent gaps. Fixing one doesn't require or block the others — check current
coverage before picking one up, since `network/fabricx.Peer`'s test coverage in particular may
have moved since this was written.
## Approach
### 1. `network/fabric/peer_test.go` — new file
Exercise `Peer.BlockHeight`, `Peer.ProcessProposal`, `Peer.SubscribeBlocks`, and
`EndorsementClient.ExecuteTransaction`/`Close`, using `fabrictest.Start(...)` — the same
fake-network pattern `integration/integration_test.go` and `network/synchronizer_test.go` already
use. Cover the error paths only the happy path reaches today: a nil `Response` from a peer
(`ProcessProposal` already special-cases this per its doc comment, but nothing verifies it), and
`ExecuteTransaction` with one of several configured peers failing.
### 2. `network/fabricx/packager_test.go` — new file
Direct, gRPC-free table-driven unit tests for `PackageTx`/`CreateTx`/`NewTxPackager`: build
`sdk.Endorsement` fixtures by hand (a proposal plus 1-3 `peer.ProposalResponse`s) and assert on the
resulting `common.Envelope`'s structure, plus the "zero responses" error case `CreateTx` already
returns explicitly.
### 3. `network/fabricx.Peer.SubscribeBlocks` coverage
Delegates to `network.Peer.SubscribeBlocks` with the channel and signer bound in; check whether it
already has a direct test before adding one. If not, cover it the same way the rest of that
package's `Peer` methods are tested.
### 4. Extend `fabrictest` with a `committerpb.NotifierServer` implementation
So `TestFabric`/`TestFabricX` can exercise `Notifications`/`FinalityListener` against the fast,
in-process fake network instead of skipping them. In `fabrictest/peer.go`, add
`committerpb.UnimplementedNotifierServer` to `testPeer`'s embedded fields (the same pattern already
used for `UnimplementedBlockQueryServiceServer`) and implement `OpenNotificationStream`, backed by
the existing in-memory `ledger` type, which already tracks committed blocks/transactions. Register
it in `fabrictest/network.go` alongside the servers already registered there.
Note for whoever picks up #43 (live MSP configuration from the ledger): the same
`UnimplementedXServer`-embedding pattern applies directly to adding
`committerpb.QueryServiceServer`/`GetConfigTransaction` to `fabrictest`. Not scoped here since
`ConfigTransaction` doesn't exist in the SDK yet — just worth reusing this approach when it does.
### 5. Run everything under `-race`
`make unit-tests` already runs with `-race`. This package group is where a real data race was
previously found (test-setup code racing on gRPC's global logger) and where the orderer's
persistent-stream goroutines and the notification send/receive fan-in live — coverage under
`-race` matters as much as the percentage itself here.
## Out of scope
- Extending `fabrictest` with `QueryServiceServer`/`GetConfigTransaction` — belongs to #43, once
`ConfigTransaction` actually exists in the SDK.
- A numeric coverage threshold enforced in CI. This issue closes the specific gaps identified
above, not a percentage target; a coverage gate is a smaller, separate follow-up.
- `network/tls_test.go` and `network/synchronizer_test.go` — already have solid coverage, not
touched.
## Verification
- `go test ./network/... -race -v -cover` — the methods listed above show non-zero coverage (not
chasing 100%), all passing under `-race`.
- `go test ./... -short -race` — full suite stays green; the new `fabrictest` `NotifierServer`
implementation must not change behavior for any existing test that doesn't use it.
- `make checks`.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start with go test ./network/... -race -v -cover and review the existing patterns in integration/integration_test.go, network/synchronizer_test.go, network/fabricx, and fabrictest/peer.go and network.go. Pick one of the independent gaps, then verify the listed methods or notification paths and run go test ./... -short -race and make checks; done means the targeted coverage passes without changing existing behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, grpc
- Domain
- networking, testing
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 55/100