And just so you know how huge this problem is:
https://infra-reports.apache.org/#ghactions
For all non-members, that don’t have access to that resource:
It shows how much of the GitHub Actions resources each project is using.
Apache IoTDB is currently using 95% of all GitHub Actions resources, leaving
less than 5% for the other 200 projects we have.
Chris
Von: Christofer Dutz <christofer.d...@c-ware.de>
Datum: Donnerstag, 18. Juli 2024 um 10:41
An: dev@iotdb.apache.org <dev@iotdb.apache.org>
Betreff: AW: AW: AW: AW: Possible pattern for reducing the negative effects of
using singletons and improving testability
I think for some tests, it might be worth migrating them back from an IT to an
UT.
I’m currently trying to get resource-consumption stats from infra and wanted to
start finding out which are our most “expensive” Its and possibly start a
discussion on converting them to unit-tests. Right now, it feels as for every
bit of functionality we add more Its, which all require spinning up of small
instances of IoTDB.
Also is it really challenging to test the unhappy path in integration-test,
which comes with other problems.
So yeah … I hope by removing the singletons from a component, we can make it
Unit-Testable and then we can gradually reduce the load on the integration-test
suite.
Chris
Von: Tian Jiang <jt2594...@163.com>
Datum: Donnerstag, 18. Juli 2024 um 10:35
An: dev <dev@iotdb.apache.org>
Betreff: Re: AW: AW: AW: Possible pattern for reducing the negative effects of
using singletons and improving testability
Certainly, I am not actually expecting a really perfect solution, just hoping
when someone else sees this discussion, he may provide a better trade-off.
And as I have said, I am happy with both solutions, as long as they can get rid
of the singletons.
Mocking is helpful in tests. But the modifications (when actually removing
singletons from the main source code) in constructors may still be painful.
Of course, it is enough just for testing purpose.
However, for the problem concerning resource consumption, are you going to
remove some current ITs if your changes are applied?
If no, the resource consumption is still there; if yes, it may be challenging
to guarantee the equivalence of the ITs and UTs.
Tian Jiang
| |
Tian Jiang
|
|
jt2594...@163.com
|
---- Replied Message ----
| From | Christofer Dutz<christofer.d...@c-ware.de> |
| Date | 7/18/2024 16:19 |
| To | dev@iotdb.apache.org<dev@iotdb.apache.org> |
| Subject | AW: AW: AW: Possible pattern for reducing the negative effects of
using singletons and improving testability |
Hi Tian,
I think – as usual – there probably is no perfect solution.
I guess it comes down to deciding which things you are willing to do in order
to solve some other problems on the other side.
I personally think our biggest problem right now (At least this is what’s
making my Life a lot harder working on IoTDB) is the inability to run
unit-tests and I see how much resources we are wasting on GitHub Actions.
Constructor-Injection solves that problem.
Of course, could be build some sort of Factory, that helps initialize some
methods, but admittedly I think a simple:
SomeService someService = Mockito.mock(SomeService.class);
Is not too challenging to do.
Chris
Von: Tian Jiang <jt2594...@163.com>
Datum: Donnerstag, 18. Juli 2024 um 09:55
An: dev <dev@iotdb.apache.org>
Betreff: Re: AW: AW: Possible pattern for reducing the negative effects of
using singletons and improving testability
A template (or util) method can provide basic contexts, and the developers just
need to override the parts they need (of course they should know which parts
they need).
In the constructor-way, the developer should know what instances they will use,
create them, and pass them to the constructor;
while in the context-way, the developer should know what fields they will use,
create them, and override them in the context.
I do not think they are essentially different.
The problem is, for high level structures like DataRegion, they will have many
many more fields because their children use many different ones, and the
constructor will be very long for them.
Certainly, you may further migrate the constructor-way to the builder-way,
which may solve the problem. However, it requires much more extra work.
I am not particularly fond of either one and I just want to point out the
problems, hoping someone may give a perfect solution.
If none is given, I am happy to accept either one.
Tian Jiang
---- Replied Message ----
| From | Christofer Dutz<christofer.d...@c-ware.de> |
| Date | 7/18/2024 15:39 |
| To | dev@iotdb.apache.org<dev@iotdb.apache.org> |
| Subject | AW: AW: Possible pattern for reducing the negative effects of using
singletons and improving testability |
However, then you need to construct these contexts with loads of parameters for
unit tests … I personally don’t think passing in explicitly what you need into
a constructor is bad.
So, if you have this service that you want to test: How can you tell the
developer writing the test which services are needed and therefore need to be
mocked in the context?
I would strongly opt for explicit dependency injection in the constructor and
not using some context.
Chris
Von: Tian Jiang <jt2594...@163.com>
Datum: Donnerstag, 18. Juli 2024 um 09:27
An: dev <dev@iotdb.apache.org>
Betreff: Re: AW: Possible pattern for reducing the negative effects of using
singletons and improving testability
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
