Potential improvements to mock_system.

[mszulcz-mitre]

Affected Branch

trunk

Basic Diagnostics

• I've pulled the latest changes on the affected branch and the issue is still present.

• The issue is reproducible in docker

Description

The mock_system class “Establishes dummy listeners for each enabled system component” such as a watchtower, atomizer, or sentinel. It’s used in many of the integration tests. I think it’s an extremely useful class, but when using it for the first few times, I got confused about some things. I think the code could modified a little to make the class a little clearer.

Components or modules?

Sometimes the things that are mocked are called components; sometimes they’re called modules. This might be confusing to a new user. For example, when looking at the signature of the expect method, the variables for_module and component_id appear, and it’s not obvious that component_id is an ID of the specified for_module:

opencbdc-tx/tests/integration/mock_system.hpp

Lines 79 to 82 in 4d82040

	template<typename T>
	auto expect(mock_system_module for_module,
	std::optional<cbdc::buffer>&& reply_with = std::nullopt,
	uint64_t component_id = 0) -> std::future<T> {

I can't see any value in using both terms, so it seems reasonable to just use the word “module” since it’s already used more frequently in variable names in the class.

When’s a module disabled?

It’s not always obvious what modules will be mocked. By default, the class mocks all modules in the m_modules set and requires the user to specify what modules not to mock by passing them to the constructor in the disabled_modules set:

opencbdc-tx/tests/integration/mock_system.hpp

Lines 50 to 52 in 4d82040

	mock_system(
	const std::unordered_set<mock_system_module>& disabled_modules,
	config::options opts);

However, even if a module is enabled, it still won’t get mocked if its endpoints aren’t specified in the options struct. This is understandable, but makes code difficult to debug because it happens without warning. For example, according to the disabled modules set, the coordinator module should be mocked in the following tests, but isn’t because endpoints aren’t given: watchtower_integration_test, sentinel_integration_test , replicated_atomizer_integration_tests , replicated_shard_integration_tests , watchtower_integration_test , atomizer_raft_integration_test . In the sentinel_2pc_integration_test, the sentinel module itself is the only module in the disabled_modules set, but 4 of the remaining 5 possible modules do not get mocked because their endpoints aren’t given.

What does the return value of start_servers mean?

The method start_servers attempts to start a listening server for a given module and returns a boolean:

opencbdc-tx/tests/integration/mock_system.hpp

Lines 154 to 156 in 4d82040

	auto start_servers(mock_system_module for_module,
	const std::vector<network::endpoint_t>& endpoints)
	-> bool;

As written, it returns true for both of these conditions:

• A server is started for the given module.
• A server is not started for the given module because it’s in the disabled_modules set or because no endpoints for the module were given.

Having the method return true even if servers aren’t started is a little confusing.

Code of Conduct

• I agree to follow this project's Code of Conduct

[mszulcz-mitre]

I opened draft pull request #184 to address these issues.

[HalosGhost]

via #184.