TomasMikula / libretto

Declarative concurrency and stream processing library for Scala
Mozilla Public License 2.0
196 stars 6 forks source link

Flaky Tests #82

Open MateuszKowalewski opened 1 year ago

MateuszKowalewski commented 1 year ago

I've experienced some flakynes in some tests. There seems something wrong with the current runtime.

Here's a zip with screenshots: Screenshot_2023062.zip

A heuristic of my test failures would look something like this:

delayed injectL     |||||
blocking        |||||||||||||||||||||||||||
mergeAll        ||||||||||||||||
LList.sortBySignal  ||
merge           |||||
latestValue         
splitResourceAsync  |

Test failures were triggered quite reliably by calling clean ; compile ; test in SBT, especially when there was additional load on the machine, or when called in short sequence.

Maybe just the timeouts are too short. But they are actually quite long for what is computed. So this should just not happen, I guess.

But I didn't investigate further until now. So can't say much besides that it happens at the moment.

TomasMikula commented 1 year ago

Thanks for reporting! I have definitely encountered the first 3, don't remember about the others. delayed injectL is in my estimate the one that I hit overwhelmingly most often on my machine.

Most of these tests are "broken by design": they rely on some scheduling guarantees which should not be assumed.

blocking, I think, also suffers from the "non-monotonicity" of time measurements across threads:

  1. Get current time t0 from one thread.
  2. sleep for 10ms (as per some built-in scheduler)
  3. Get current time t1 from some other thread.
  4. t1 - t0 may be less than 10ms.

Flaky tests are definitely something that needs to be fixed. Though maybe not all at once.

MateuszKowalewski commented 1 year ago

Improving and hardening the runtime should fix this I guess.

It's not like the test as such would have issues. It's how they are executed.

Testing async runtimes is trick anyway in itself…

TomasMikula commented 1 year ago

I have rewritten delayed injectL test in a deterministic fashion. It should now never fail. If it does, please, re-report.

MateuszKowalewski commented 1 year ago

Thanks a lot!

But does it make sense to heal symptoms of an underlying problem?

If the current runtime has issues with timeouts that should not go unnoticed. Just "tuning" tests so these issues don't show up defeats the idea behind tests, imho. That's like: "I've made all test pass by deleting them…" :wink:

I would in this case just document the behavior so it's not unexpected, and look into the root cause.

If looking into the root cause is currently out of scope, that's also fine. Just create a ticket and look into that when time has come.

Never mind, just my 2ct.

TomasMikula commented 1 year ago

The delayed injectL test was actually broken:

It was doing

clock.advanceBy(31.millis)

but if that adjustment of time happened before the tested program did

delay(30.millis)

the test would inevitably timeout, because the clock will not be advanced by another 30 millis for this delay to finish.