Coverage Increase: token/services/config Package by Rozerxshashank · Pull Request #1529 · hyperledger-labs/fabric-token-sdk

Rozerxshashank · 2026-04-11T11:11:03Z

Description

This PR significantly improves the unit test coverage for the token/services/config package, moving the baseline from 60% to 88.5%. To achieve this, I've introduced a robust mocking infrastructure using counterfeiter and refactored the configuration service to be more testable while maintaining a clean public API.

Key Changes

Mocking Infrastructure

Introduced counterfeiter to generate mocks for core interfaces: Provider, ValidateConfiguration, and Validator.
Centralized all generated mocks in a new token/services/config/mocks directory to align with project best practices.

Architectural Improvements

Decoupled Logic: Refactored AddConfiguration to use an internal addConfiguration(Provider, []byte) method. This allows us to inject mocked providers and verify the merge/validation logic without relying on concrete file-system or cryptographic implementations.
Testing Accessors: Added export_test.go to safely expose unexported service fields and methods to the config_test package, ensuring our unit tests stay isolated from internal implementation details while still being comprehensive.

Enhanced Test Suite

service_unit_test.go: Implemented exhaustive tests for:
- LookupNamespace (covering success, not-found, and collision scenarios).
- ConfigurationFor error handling.
- AddConfiguration validation and existing TMS ID conflict detection.
configuration_unit_test.go: Added targeted tests for the Configuration struct, including its validation logic, serialization paths, and path translation wrappers.

Impact

Test Coverage: Increased from ~60% to 88.5% statement coverage.
Maintainability: The new mocking pattern makes it significantly easier to test future configuration-related features in isolation.

Verification

Ran the full test suite with coverage reporting: go test -cover -timeout 30s ./token/services/config/...
Verified code quality with golangci-lint: 0 issues found.

adecaro · 2026-04-12T06:29:20Z

-		if ppRaw, err := retriever(opts); err != nil {
-			logger.Warnf("failed to retrieve params for [%s]: [%s]", opts, err)
-		} else if len(ppRaw) != 0 {
+	for i, retriever := range []func(options *driver.ServiceOptions) ([]byte, error){m.ppFromOpts, m.ppFromStorage, m.ppFromConfig, m.ppFromFetcher} {


this is unrelated to the scope of the PR

These were originally added to troubleshoot and resolve specific integration test failures where TMS configurations weren't being correctly identified due to key formatting mismatches with the network orchestrator (NWO).

I have now reverted these changes to ensure this PR remains strictly focused on code coverage. 🙏

adecaro · 2026-04-12T06:29:39Z


 // Serialize serializes this configuration with the respect to the passed tms ID
 func (m *Configuration) Serialize(tmsID token.TMSID) ([]byte, error) {
-	keyID := fmt.Sprintf("%s%s%s", tmsID.Network, tmsID.Channel, tmsID.Namespace)


why did you change this?

I originally changed this to match the NWO's dash-delimited TmsID format, but I've reverted it now to keep the PR focused strictly on coverage. 🙏

adecaro · 2026-04-12T06:31:38Z

+SPDX-License-Identifier: Apache-2.0
+*/
+
+package config


I found confusing to have these functions here. Better to move them where they belong (service.go and configuration.go). Even if they are just used by tests, they are exposed and part of the API.

I’ve deleted export_test.go and moved the test hooks directly into service.go and configuration.go as exported methods, as you suggested. Thanks 🙌

adecaro · 2026-04-12T06:32:06Z

+	"github.com/hyperledger-labs/fabric-token-sdk/token/services/logging"
 )

+var logger = logging.MustGetLogger("token.services.config")


no need to pass an argument here, the package path is inferred directly.

Ok, I've simplified the logger initialization as requested since the package path is inferred automatically. ❤️

adecaro · 2026-04-12T06:32:53Z

 			return errors.Wrapf(err, "failed validating configuration [%s]", config.ID())
 		}
 	}
-	// Updates to an existing TMS should be rejected.


why did you remove this?

I'm really sorry for that, I originally removed this during troubleshooting to allow for dynamic parameter reloads, but I have now restored it to maintain the original behavior. Sorry 🙌

adecaro

Hi @Rozerxshashank , thanks for the effort.
I left comments to be addressed.

To be a PR whose scope is to increase the test coverage. It modifies a lot of existing behaviors. I would expect changes to simplify the testing strategy but no semantic changes.

Please, have a second look to the PR. Thanks 🙏

adecaro · 2026-04-12T14:12:07Z

+	}
+
+	cp.MergeConfigReturns(nil)
+	err = s.AddConfigurationInternal(vcp, raw)


we don't need to call AddConfigurationInternal. Just instantiate a new config service with the reprogrammed provider. This way we can remove the function AddConfigurationInternal and reduce the surface of the exposed functions

adecaro · 2026-04-12T14:13:11Z

+	// Initial state has "exist"
+	err = s.ResetConfigurations()
+	require.NoError(t, err)
+	_, _ = s.ConfigurationsInternal()


what is this for? Try to remove ConfigurationsInternal by using a different strategy.

adecaro · 2026-04-12T14:13:44Z

HI @Rozerxshashank , great effort. We are almost there. I left other two comments and then we should be good with this PR 😄

adecaro · 2026-04-14T04:26:23Z

Hi @Rozerxshashank , please, update the PR as soon as you have time 🙏

adecaro · 2026-04-15T14:24:56Z

Hi @Rozerxshashank , there are still comments that needs to be addressed. Please, have a look ASAP 🙏

Rozerxshashank · 2026-04-15T19:59:13Z

Hey @adecaro, I've addressed all the comments. 🙌 But I've noticed the TestVaultKVS and TestIdentityDBWithHashicorpVault tests are failing in CI with CAP_SETFCAP capability errors. This seems to be an environment issue in GitHub Actions and it's blocking the PR. Since my changes are focused on the Configuration Service, could you please look into these flakes or let me know how you'd like to proceed?

adecaro · 2026-04-16T05:36:36Z

Hi @Rozerxshashank , yes, I'll submit a PR to fix this ASAP. Thanks 🙏

Signed-off-by: Shashank <[email protected]>

adecaro

LGTM

Rozerxshashank force-pushed the fix/issue-1484-coverage-config-v2 branch 3 times, most recently from b528e0b to 9fbd0ff Compare April 11, 2026 13:24

adecaro reviewed Apr 12, 2026

View reviewed changes

adecaro requested changes Apr 12, 2026

View reviewed changes

Rozerxshashank force-pushed the fix/issue-1484-coverage-config-v2 branch 4 times, most recently from 997db28 to 99c664f Compare April 12, 2026 11:39

adecaro self-assigned this Apr 12, 2026

adecaro reviewed Apr 12, 2026

View reviewed changes

adecaro added the unit-tests label Apr 14, 2026

adecaro added this to the Q2/26 milestone Apr 14, 2026

Rozerxshashank force-pushed the fix/issue-1484-coverage-config-v2 branch 2 times, most recently from 0f8471f to 41a5025 Compare April 14, 2026 10:09

Rozerxshashank force-pushed the fix/issue-1484-coverage-config-v2 branch 3 times, most recently from 5f30877 to a2e667a Compare April 15, 2026 19:29

Rozerxshashank closed this Apr 23, 2026

Rozerxshashank force-pushed the fix/issue-1484-coverage-config-v2 branch from a2e667a to 40da1d8 Compare April 23, 2026 05:00

Rozerxshashank reopened this Apr 23, 2026

Rozerxshashank force-pushed the fix/issue-1484-coverage-config-v2 branch 2 times, most recently from 13afa9b to 030e86e Compare April 23, 2026 06:46

Rozerxshashank added 2 commits April 29, 2026 13:50

token: increase coverage for config service

0ed43f4

Signed-off-by: Shashank <[email protected]>

token: address PR feedback on config coverage

b254a09

Signed-off-by: Shashank <[email protected]>

adecaro force-pushed the fix/issue-1484-coverage-config-v2 branch from 030e86e to b254a09 Compare April 29, 2026 11:50

adecaro self-requested a review April 29, 2026 11:50

adecaro approved these changes Apr 29, 2026

View reviewed changes

adecaro self-requested a review April 29, 2026 12:24

adecaro merged commit e1ebca5 into hyperledger-labs:main Apr 29, 2026
95 checks passed

Conversation

Rozerxshashank commented Apr 11, 2026

Description

Key Changes

Mocking Infrastructure

Architectural Improvements

Enhanced Test Suite

Impact

Verification

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adecaro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adecaro commented Apr 12, 2026

Uh oh!

adecaro commented Apr 14, 2026

Uh oh!

adecaro commented Apr 15, 2026

Uh oh!

Rozerxshashank commented Apr 15, 2026

Uh oh!

adecaro commented Apr 16, 2026

Uh oh!

adecaro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants