[satel] Don't use @InjectMocks in discovery test

[Nadahar]

This is an attempt to fix what seems to be flakiness in the Satel binding's discovery tests.

[Nadahar]

@lsiepel Here is a take - but I can't possibly verify anything, since the test didn't fail locally as it was either. I don't fully understand how @InjectMocks works, that part is clear, but it seems like you have little control over what it actually does, so this is an attempt at avoiding it altogether.

[Nadahar]

@lsiepel Is there any way to run this PR on Jenkins before merging, so that we can actually verify that it will work there?

[lsiepel]

I'll invite @david-pace to join this PR, as i think he dealt with this before.

Regarding the differnt build systems; the best person to comment on this is probably either @holgerfriedrich or @wborn

[Nadahar]

All I know is that the solution I've picked now works locally, and it works with the GitHub CI. But, the same is the case with the current code, which is why it feels relevant to get Jenkins to have a go at it, if possible.

I think this solution should work though, since I've excluded some of the "magic" and it still works. Less "magic" hopefully means fewer surprises.

My assumption is that Jenkins either runs slower, or that it runs with parallel threads or something else that affects the relative speed between threads.

[druciak]

Looks good.
You could leave my version of this test, worked well. I don't have any preference, if you think it is better to go this way, I am okay.

[Nadahar]

Looks good.
You could leave my version of this test, worked well. I don't have any preference, if you think it is better to go this way, I am okay.

The reason I changed it was that I felt that you had to "simplify it" as a result of the problems introduced by the change in the discovery result registration. So, this wasn't because your fix wasn't "good", I just wanted to restore what was previously there. Whether it has much point to do so, is a different matter, I just felt "guilty" for having forced a change.

[Nadahar]

I just discovered that the MyBMW discovery test also seems to be flaky for the same reason as the others: https://ci.openhab.org/job/openHAB-Addons/1948/org.openhab.addons.bundles$org.openhab.binding.mybmw/testReport/junit/org.openhab.binding.mybmw.internal.discovery/VehicleDiscoveryTest/testDiscovery/

I pushed the fix here instead of making yet another PR on this topic.

[Nadahar]

For reference, this PR is related to #19562, #19473 and #19230.

[mherwege]

For mybmw, disabling it completely would be fine as well. The binding does not work anymore and needs recreating for the new API. This is not working anymore anyway.

[Nadahar]

For mybmw, disabling it completely would be fine as well. The binding does not work anymore and needs recreating for the new API. This is not working anymore anyway.

Ok, I think the modification I made will take care of the test failure - but, I've heard of other official bindings that no longer work (the Mill binding comes to mind) because of online API changes. My question is: should there be some routine to disable these when they are known to no longer work? Users probably won't know this, and will struggle and think that they are doing something wrong, causing unnecessary frustration.

[Nadahar]

Argh, now one of the Satel tests failed again, even though all I changed was in the MyBMW binding. So, something is clearly still flaky there.

It takes so long time for me to switch branches locally that I was basically just finished rebuilding after switching away from this branch from when I pushed the MyBMW commit. Now I have to switch back again, rebuild, fix, and then finally switch to the branch I'm working on again - and rebuild. This whole process is painfully slow 😢

[Nadahar]

No wonder why the Satel tests failed, I had forgotten to forward the executor in the "test constructor" I created in the discovery service - so it was still running threaded 😊

https://github.com/openhab/openhab-addons/compare/186bcf178dc681f1c16b32d1f452d5502e8a0e52..2a0aab6b8f79fbaa044673a1d6386f6711bce72d

[david-pace]

In case it helps: I faced problems regarding annotations in tests, and summarized my observations here: #19180 (comment)

This happened after the ECJ (Eclipse Compiler for Java) version was updated. Apparently something changed relating to the processing of annotations. Unfortunately, the resulting effects don't seem to be deterministic.

My changes were:

• [boschshc] Fix unit test setups #19188
• [boschshc] Fix unit test setups (part 2) #19193

[Nadahar]

This happened after the ECJ (Eclipse Compiler for Java) version was updated. Apparently something changed regarding the processing of annotations. Unfortunately, the resulting effects don't seem to be deterministic.

I'm not sure why you concluded that it was a problem with the annotations, but then again, I don't know the nature of the test failures. Was it compile error, or just "random errors" while running? Because, if it's the latter, it almost always means that it's timing-related.

I've always experienced a difference between running tests from Eclipse and Maven if they are timing sensitive (and there are some other slight differences too, Eclipse will sometimes fail things that work from Maven). If it's in fact a timing issue, every little details could potentially make a difference, maybe annotation processing is faster or slower than "the old way", which results in different outcome?

I don't think the problems I'm experiencing here now is due to annotations, my previous solution would probably have worked if I didn't make the blunder in the constructor - but I'm not eager to try that again now, given how long this takes to build.

This build just succeeded now, so I hope this is good to go.

@lsiepel If running it on Jenkins manually is difficult, we could just merge it now and observe what happens to the Jenkins build tonight.

[lsiepel]

Thanks, LGTM

Let's see if this now stabalizes the tests.

[lsiepel]

Regression of #19562

[Nadahar]

The Jenkins build looks like it went fine now.