search

openhab / openhab-addons

Add-ons for openHAB
https://www.openhab.org/
Eclipse Public License 2.0
1.86k stars 3.58k forks source link

[homematic] Unit tests unstable #10753

Open fwolter opened 3 years ago

fwolter commented 3 years ago

See https://ci.openhab.org/job/PR-openHAB-Addons/org.openhab.addons.bundles$org.openhab.binding.homematic/4245/testReport/org.openhab.binding.homematic.internal.communicator.virtual/ButtonDatapointTest/testDoublePress/

java.lang.AssertionError: 

Expected: is "DOUBLE_PRESSED"
     but: was null
    at org.openhab.binding.homematic.internal.communicator.virtual.ButtonDatapointTest.testDoublePress(ButtonDatapointTest.java:106)

@FStolte @gerrieg @mdicke2s Can you take a look?

fwolter commented 3 years ago

You could use waitForAssertion(() -> ...) provided from the JavaTest class instead of the sleeps, which are error prone and depend on the load of the build machine.

MHerbst commented 3 years ago

Thanks for the hint. I will have a closer look at it next weekend.

MHerbst commented 3 years ago

@fwolter I had a look into the implementation of waitForAssertion(), but I am not sure that it can be used to replace the sleep calls completely.

In the first part of the test, a double press is simulated and the sleep() is used to have a small interval between two short press events. If I replaced the sleep() with waitForAssert there would be no sleep before the first test.

The second sleep() is necessary because a short press should be detected as a single press and not a double press. Therefore it needs a longer pause, and this won't work with waitForAssert().

But since the problem occurs in the first part of the test, we could try to see if the problem can be solved this way.

fwolter commented 3 years ago

After looking into this test for a few minutes, I don't understand what happens there really. In any case there's something fishy about the sleep in the "mock". Seems to me some state is reset after some time. This could be done manually in the test as it seems the sleep in the mock is the only concurrent operation in the unit test. So, if you replace the sleep in the mock by an external manual reset, the sleep in the tests might be removed, too.

fwolter commented 3 years ago

I'd consider waiting for assertions in unit tests a hack, tough. Often that means you also test some scheduler along with your code, which shouldn't be your intention when writing unit tests.

MHerbst commented 3 years ago

I agree with you and I have the same problem to understand why it is implemented this way. In my opinion the whole test does not make sense.