Code refactoring and Testability
Closed this issue · 15 comments
Objective(s):
- We need to place the codebase under test
- We need to decompose the code into modular & testable pieces
- We need a common test framework that we can use (e.g. gmock) and is OS agnostic
We need to place the codebase under test
We need a common test framework that we can use (e.g. gmock) and is OS agnostic
When it comes to testing. You suggested: https://github.com/google/googletest, which is a great idea.
I think when it comes to the granularity of the types of testing scenarios we need. Boost Unit may not be the most optimal way to ensure functional test cases.
We need to decompose the code into modular & testable pieces
This part. We need to start small. What is a specific part of the codebase we could granularize? Personally, I think separating the source in chainparams.cpp would be a good idea.
Having a chainparams folder. And each chain configuration it's own .hpp file which is extended in chainparams.cpp.
I am more than happy to start a pull request. We can review the formatting. And make sure that it reflects the type of code style that will make all our lives easier.
Thus, the structure of chainparams.cpp would then be:
chainparams.cpp
chainparams.hpp
chainparams/chainparams.main.hpp
chainparams/chainparams.testnet.hpp
chainparams/chainparams.regtest.hppThus, the structure of
chainparams.cppwould then be:chainparams.cpp chainparams.hpp chainparams/chainparams.main.hpp chainparams/chainparams.testnet.hpp chainparams/chainparams.regtest.hpp
Not entirely sure about the splitting or why we're switching to 'hpp'. I like the idea of separating out the implementations in their own files, but I think the concept of "chainparams" is outdated. We should be moving to a more coherent system that depends less on "global" objects like chainparams
@galpHub, do you think that instead of the chain params being hardcoded. The chain params could be extensible by a .json or .yaml configuration file?
The reason why .hpp instead of .cpp is purely just for the portability and flexibility of header files. A header file does not need to be part of the main program and is instead linked as a library instead.
This means that these header files could be ported to other codebases more easily.
The reason why
.hppinstead of.cppis purely just for the portability and flexibility of header files. A header file does not need to be part of the main program and is instead linked as a library instead.This means that these header files could be ported to other codebases more easily.
I don't think we should be placing a premium on bringing our code into other codebases that might not even want it. It's a case of optimizing outside of the leaky bucket lol.
@galpHub, do you think that instead of the chain params being hardcoded. The chain params could be extensible by a
.jsonor.yamlconfiguration file?
Not directly like that, what I mean is that chainparams are parameters for the entire blockchain, but instead of getting transmitted as parameters, they just float "everywhere" instead of being introduced where they're actually relevant. GLOBALS ARE EVIL
It's not necessarily for other people to use it.
But, rather create more flexibility for other potential tools we may create.
Regardless, that semantic is not a big deal. I think understanding how exactly the parameters themselves should be structured is more important.
In this case, we need to create it into one object that is contained in only the specific parts of the code that needs it.
I agree globals can and will cause problems.
It's not necessarily for other people to use it.
But, rather create more flexibility for other potential tools we may create.Regardless, that semantic is not a big deal. I think understanding how exactly the parameters themselves should be structured is more important.
In this case, we need to create it into one object that is contained in only the specific parts of the code that needs it.
I agree globals can and will cause problems.
That's fair. The first thing we need is a test framework, and then slowly move towards refactoring the code and bring it under the framework.
Without getting proper tests going we won't get very far refactoring without potentially introducing problems.
That's true.
Let's get everything dialled down with a full testing suite.
And then we can start introducing refactors.
I'd say that if you're keen on refactoring chainparams, which honestly is going to happen at some point anyway, I'd ask that you get it under test and then look into what's needed to get the actual params object injected as a proper dependency anywhere that it is needed - instead of being requested on the fly from a global object.
I agree with a test-driven refactor. I think the testing framework will be most important here so that we don't end up back where we started. Although, as @ChrisCates mentioned, Boost has proven to be kind of substandard, it remains one of the top unit testing frameworks for C++ out now. I'm open to using a different framework. Google Test looks excellent. What about CTest or Microsoft's variant, which I believe is extensible to some degree. That could be valuable in creating unit tests for specific cases. You guys let me know which direction you're leaning.
As for modularity, I couldn't be more in agreement. As we scale the ecosystem, modularity will become a critical factor in our ability to develop new iterations quickly. It's the reason we are modularizing the APIs for the 2.0 application suite. Let me know if I can help with building out this architecture.
I've mentioned it several times to both you and @galpHub, but when we really carve out the improved module based codebase. I would love to create a BIP for Bitcoin Core. And outline some key decisions that improved the source code.
I've mentioned it several times to both you and @galpHub, but when we really carve out the improved module based codebase. I would love to create a BIP for Bitcoin Core. And outline some key decisions that improved the source code.
If we can help advance other projects (esp BTC) while helping our own I can't see a downside 😄