search

mit-dci / opencbdc-tx

A transaction processor for a hypothetical, general-purpose, central bank digital currency
Other
896 stars 198 forks source link

Potential improvements to `mock_system`. #183

Closed mszulcz-mitre closed 1 year ago

mszulcz-mitre commented 1 year ago

Affected Branch

trunk

Basic Diagnostics

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: https://github.com/mit-dci/opencbdc-tx/blob/4d82040c58142aec263a6e293b2664ec140141f6/tests/integration/mock_system.hpp#L79-L82

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: https://github.com/mit-dci/opencbdc-tx/blob/4d82040c58142aec263a6e293b2664ec140141f6/tests/integration/mock_system.hpp#L50-L52

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: https://github.com/mit-dci/opencbdc-tx/blob/4d82040c58142aec263a6e293b2664ec140141f6/tests/integration/mock_system.hpp#L154-L156 As written, it returns true for both of these conditions:

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

Code of Conduct

mszulcz-mitre commented 1 year ago

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

HalosGhost commented 1 year ago

via #184.