Yes, maybe we could have leveled context like DataNodeContext,
DataRegionContext, TsFileProcessorContext... and put the former singletons in
these contexts.
This may avoid constructors with dozens of parameters.
Tian Jiang
| |
Tian Jiang
|
|
jt2594...@163.com
|
---- Replied Message ----
| From | Christofer Dutz<christofer.d...@c-ware.de> |
| Date | 7/18/2024 15:22 |
| To | dev@iotdb.apache.org<dev@iotdb.apache.org> |
| Subject | AW: Possible pattern for reducing the negative effects of using
singletons and improving testability |
Ideally,
we could inspect the usages of the constructors using the singletons and apply
the same pattern there. Once all usages have been migrated that way, we could
even eliminate the Singleton-using constructor. In the end getting a system for
which we can finally write sensible unit-tests again and don’t need to rely on
Integration tests.
Integration tests that have problems running in paralell, while unit-tests can
run perfectly in paralell.
Chris
Von: Christofer Dutz <christofer.d...@c-ware.de>
Datum: Donnerstag, 18. Juli 2024 um 09:03
An: dev@iotdb.apache.org <dev@iotdb.apache.org>
Betreff: AW: Possible pattern for reducing the negative effects of using
singletons and improving testability
Ok …
So, as there was no objection here, I’ll trat that as consensus and I’ll just
do this for every component that I touch and hereby need to write tests for.
Chris
Von: Christofer Dutz <christofer.d...@c-ware.de>
Datum: Montag, 15. Juli 2024 um 11:49
An: dev@iotdb.apache.org <dev@iotdb.apache.org>
Betreff: Possible pattern for reducing the negative effects of using singletons
and improving testability
Hi all,
I am currently working on some functions and when writing tests for these, I
had problems sensibly testing things, as the singletons were preventing that.
As I mentioned before … I consider the usage of singletons the probably worst
design decision done in IoTDBs core. Even if it simplifies “composition”, it
really makes everything else more complicated.
Knowing how deeply rooted in IoTDB the use of singletons is, I always thought
this would be a never-ending story.
But I just recently had an idea of how we could probably find a way to get the
testability that we need without having to give up on singletons.
Possibly this path could also provide a path to getting rid of singletons over
time.
As I was just working on DataNodeServerCommandLine I did an experiment with
that, and it worked nicely.
So, I searched for all singletons used in the component and defined private
final member variables for each of them.
In the no-args constructor I initialize these via the singletons, but I also
add a second constructor, that allows passing in explicit versions.
private final ConfigNodeInfo configNodeInfo;
private final IClientManager<ConfigRegionId, ConfigNodeClient>
configNodeClientManager;
private final DataNode dataNode;
/** Default constructor using the singletons for initializing the relationship.
*/
public DataNodeServerCommandLine() {
configNodeInfo = ConfigNodeInfo.getInstance();
configNodeClientManager = ConfigNodeClientManager.getInstance();
dataNode = DataNode.getInstance();
}
/**
* Additional constructor allowing injection of custom instances (mainly for
testing)
*
* @param configNodeInfo config node info
* @param configNodeClientManager config node client manager
* @param dataNode data node
*/
public DataNodeServerCommandLine(
ConfigNodeInfo configNodeInfo,
IClientManager<ConfigRegionId, ConfigNodeClient> configNodeClientManager,
DataNode dataNode) {
this.configNodeInfo = configNodeInfo;
this.configNodeClientManager = configNodeClientManager;
this.dataNode = dataNode;
}
With this was I now able to write real unit-tests for the component, without
having to require writing an Integration-test which costs a lot more resources
and which makes implementing unhappy-path tests really challenging.
With this approach, I think we can really improve testability in IoTDB …
considering a class test coverage of only 24% and a method coverage of just 19%
in the data node, we really need this.
What do you all think?
Chris
