hyperledger / hyperledger/fabric-x-sdk

Test coverage for network/fabric and network/fabricx

Open
#45 0 comments 0 reactions 0 assignees View on GitHub
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.