Same thread executor

[Nadahar]

After #5032 was merged, there has been problems with tests for some add-ons: openhab/openhab-addons#19230 openhab/openhab-addons#19473

Some tests are still disabled as a consequence. The problem is that when discovery result registration became asynchronous, tests' assumptions of when the results have been registered fails. Previously, when the registration was done synchronously, it took much longer before thingDiscovered() returns, so when the test continued, the results were in. Now, the thingDiscovered() call is very quick, because another thread performs the actual registration.

The remedy for many of the tests have been to introduce delays and timeouts, but for some that hasn't been enough. Relying on timeouts and delays is a bad practice anyway, because you make assumptions of the speed of the test runner. If the test runner is slow, like the GitHub runners sometimes are, tests fail if the delay is too short. At the same time, the more you increase the delays, the slower the tests will be to run even on fast runners. There is no "good solution".

To have a better way to deal with situations like this, I've expanded on the approach made by @david-pace here:

https://github.com/openhab/openhab-addons/blob/e585d8e417e20830c017cb8e581f0369729ef1d0/bundles/org.openhab.binding.boschshc/src/test/java/org/openhab/binding/boschshc/internal/devices/bridge/LongPollingTest.java#L131-L198

(I hope that's OK for you @david-pace)

The SameThreadExecutorService is a "fake" executor service that will run everything synchronously in the calling thread. It has some limitations, particularly that it can't schedule executions ahead of time, but it should be a suitable drop-in for "real" executors that can be useful for tests in "asynchronous scenarios". The idea here is to add this to core, so that binding (and other code) can use it, instead of everybody having to reinvent the wheel.

I've also done some modifications to AbstractDiscoveryService to make it more "override friendly", for more testing flexibility. I think that using a lot of private fields in abstract classes is a bad practice in general, they are supposed to be subclassed, but the often extensive use of private can make this very cumbersome and inflexible.

This should be very low-risk, as it's not actually used by anything in core, except for the modified AbstractDiscoveryServiceTest. I've also slightly expanded the tests in AbstractDiscoveryServiceTest since the existing tests failed to fail as a result of #5032, which would have revealed the issue before merge.

[Nadahar]

I'll add that I intend to address the disabled add-on tests once this is merged, using the new SameThreadExecutorService.

[Nadahar]

I was uncertain in which package to place SameThreadExecutorService. Another ExecutorService implementation is placed in org.openhab.core.common, but since this isn't a "real" executor, I opted to put it in org.openhab.core.util. If that's considered wrong, let me know, and I'll move it.

[Nadahar]

@jlaur @lsiepel FYI

[david-pace]

Constructor code is redundant, I would propose to call this(ThreadPoolManager.getScheduledPool(DISCOVERY_THREADPOOL_NAME), ...) here

[Nadahar]

Technically, yes, but I didn't intend to change more than I had to. I added the other constructor specifically for use by tests, and left this one alone.

[Nadahar]

@holgerfriedrich It would be nice if this could be reviewed soonish, because I also need to address the disabled tests in addons once this has been merged. The "risk" from this should be non-existent, it can't really break anything.

[Nadahar]

I'm guessing that the disabled add-on tests isn't much of a concern since this is left hanging. My memory on the issue is slowly fading, so it becomes less and less viable for me to quickly restore the tests to a working condition as time goes by.

[lsiepel]

That’s an assumption and probably not accurate. As you can see, there are several PRs in core awaiting review by @openhab/core-maintainers. It seems they just don’t have much time to provide feedback at the moment. This PR will eventually be looked at, I’m sure. And yes, it might take so long that some details are forgotten—but unfortunately, there’s not much I can do about that.

As mentioned before, your efforts and in-depth analysis are highly valuable, but the limited time of core developers remains the bottleneck.

[Nadahar]

That’s an assumption and probably not accurate. As you can see, there are several PRs in core awaiting review by @openhab/core-maintainers. It seems they just don’t have much time to provide feedback at the moment.

I understand that reviews are demanding and can take a lot of time (I've done reviews in other projects, and I think it's difficult and often "hard work"). I have #4633 that has been waiting since March, and that I've had to modify multiple times to accommodate other changes in core that have happened in the meanwhile. I'm not saying that I think it's ideal, far from it, it can be pretty demotivating and frustrating, but I have some understanding for the situation.

This PR is different as I see it. It's "no risk", because it doesn't impact anything that's already there. All it does is to provide a new class that tests can use, and then loosen up some restrictions in an existing class. It doesn't really matter if the new class is completely broken, it won't have any negative impact on the system because it's currently not in use. Because of this, I think reviewing it should be much easier/quicker, which is why I also expected it to be processed faster.

[holgerfriedrich]

Can I ask you to revert the change from private to protected?
I have read you statement regarding the visibility and also went through problems when I tried to derive from too heavily encapsulated classes. Though, it is not required for the impl. and not for the test. Thus I would try to avoid this change of the encapsulation, esp. as a side change in a PR with a different focus.

Thanks!

Besides that, PMD suppression can be done locally.

Otherwise, LGTM.

[Nadahar]

After changing the @SuppressWarnings configuration, Eclipse demands to rebuild everything - including core and addons. My computer will be busy doing that for some hours, so I've pushed this "blind". The CI builder should reveal any problems though.

Can I ask you to revert the change from private to protected?
I have read you statement regarding the visibility and also went through problems when I tried to derive from too heavily encapsulated classes. Though, it is not required for the impl. and not for the test. Thus I would try to avoid this change of the encapsulation, esp. as a side change in a PR with a different focus.

I have reverted them, although I still think it's pointless. The reason this PR came to be was as a result of the problems with add-on tests in the wake of #5032. The things I did here were the things I considered "necessary" to change in Core to address the problems in the add-ons, which is how these things are related. I no longer remember the exact details of why I came to the conclusion I did, but in my mind, both changes would be helpful to make the tests work as intended.

Nothing is "required" though. It's possible to include both the SameThreadExecutorService and a modified version of AbstractDiscoveryService (or just a modified interface implementation that is a tweaked copy of AbstractDiscoveryService) in the add-ons that need this. I just don't see how it is desirable to fragment things in this way. It means repeating identical or almost identical code in several bundles, and perhaps "more seriously", it means that the tests will run with an implementation that doesn't match what the add-on is actually using, so that if something changes in Core's AbstractDiscoveryService which makes the add-on fail, the test won't reflect that (since its implementation will remain unchanged). To make tests have any value, they should mimic what actually happens as closely as possible.

[holgerfriedrich]

I see this as a useful extension to ease testing, so better to have it in core than spreading over several add-ons.

Thanks for doing the requested changes, though you prefer it the other way.

I want to provide some context on review times.
Our @openhab/core-maintainers team does this work on top of day jobs, so availability can vary. When time is tight, I might handle straightforward PRs (minor fixes, dependency updates, etc.) but need to defer more complex reviews that require deeper focus or familiarity with specific areas of the codebase.
Additionally, some PRs touch on specialized areas where we rely on maintainers with particular expertise, which can sometimes create bottlenecks.
We still appreciate all contributions to openHAB!

[holgerfriedrich]

I have triggered a core build on our CI, so the new class should be available for use in about 30 mins.

[Nadahar]

Our @openhab/core-maintainers team does this work on top of day jobs, so availability can vary. When time is tight, I might handle straightforward PRs (minor fixes, dependency updates, etc.) but need to defer more complex reviews that require deeper focus or familiarity with specific areas of the codebase.
Additionally, some PRs touch on specialized areas where we rely on maintainers with particular expertise, which can sometimes create bottlenecks.

I do understand the general situation, I have experience reviewing myself, and I know how hard it can be (to get familiar enough with the code to be able to properly evaluate what it does).

That said, it looks to me like the Core team is too small. I mostly only see you actually doing reviews. That sounds like an unfortunate situation to me. In addition, I think it would be helpful with some "initial feedback" before a proper review is done, that addresses the idea/approach more generally. When a PR is just left there for a long time, you have no idea if it's because the idea or approach is rejected, or if nobody has had the time to study it deeply enough to do a proper review. It's much "easier" to wait if you know that it's the latter.

When it comes to areas requiring specialized knowledge, I do understand that this can be a challenge. OH reaches over a lot, and it's not human to have an overview of it all. But, if a "suitable" reviewer isn't available, I think one need to take some chances sometimes. Do the best review you can with the knowledge you have, and merge it at a point in time where there's ample time to address things that the review missed. Otherwise, some things will just get stuck "forever".