Fix dropped errors in tests #246
Conversation
Also, make test-alt a bit more reliable and make the exception messages distinct so that a dropped error can easily be matched up with the deferred.
test/manifold/deferred_test.clj
Outdated
| (deftest test-alt | ||
| (is (#{1 2 3} @(d/alt 1 2 3))) | ||
| (is (= 2 @(d/alt (d/future (Thread/sleep 10) 1) 2))) | ||
| (is (= 2 @(d/alt (d/future (Thread/sleep 100) 1) 2))) |
There was a problem hiding this comment.
why is the 10 making this test flaky?
There was a problem hiding this comment.
Hmm. Probably because there's a non-zero chance of the following sequence:
- deftest thread starts the future thread.
- deftest thread gets swapped out
- future thread sleeps, runs, and returns
- deftest thread resumes, and starts the alt logic checking its possible values in random order
- deftest thread checks the future results before "2", finds it's finished, and returns
It's unlikely, but not impossible. The longer the future thread sleeps, the less likely the alt logic is to be delayed.
Honestly, this test is not great. Using unit tests on probabilistic behaviors was always a recipe for flakiness.
One quick way to improve consistency is to set a seed during testing, so the random order used by alt/alt' is always the same.
It won't fix the underlying issue, though. As the sleep time approaches the slice time given threads, the odds of the "sleeping" thread finishing before alt checks any of its values, goes way up.
One possibility is to run it repeatedly, and set a threshold for the expected proportion that select 1 vs 2. Of course, that proportion will still be affected by hardware, so...
There was a problem hiding this comment.
Right, @KingMob's hypothesis is also what I arrived at after having seen it fail like that occasionally. This happened while I was working on the patch. I repeatedly ran the test from the REPL so there was probably a bit more thread scheduling going on compared to running the test suite once with lein test.
I agree that this test isn't great. However, I just realized that we can actually make it fully deterministic - see ec01b4a!
Involving `d/future` makes these tests needlessly flaky. By decoupling the `d/alt` invocation from dereferencing its result, we can deterministically control the order of events.
Also, make test-alt a bit more reliable and make the exception messages distinct so that a dropped error can easily be matched up with the deferred.
Note this only fixes cases which are due to the test code itself. All other failing tests require changes in Manifold itself.
Based on #245.