Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Non-deterministic test failure: schedulers #1654

Closed
benjchristensen opened this issue Sep 1, 2014 · 5 comments
Closed

Non-deterministic test failure: schedulers #1654

benjchristensen opened this issue Sep 1, 2014 · 5 comments

Comments

@benjchristensen
Copy link
Member

rx.observables.BlockingObservableTest > testSingle STARTED

rx.schedulers.NewThreadSchedulerTest > testUnSubscribeForScheduler STARTED
rx.schedulers.NewThreadSchedulerTest > testUnSubscribeForScheduler FAILED
    java.lang.AssertionError: expected:<2> but was:<8>

rx.schedulers.NewThreadSchedulerTest > testRecursion STARTED
Test Result (2 failures / +2)
rx.schedulers.ExecutorSchedulerTest.testUnSubscribeForScheduler
@benjchristensen
Copy link
Member Author

I was watching this test run and the CloudBees instance was very slow.

@benjchristensen benjchristensen added this to the 1.x milestone Oct 7, 2014
@dmgd
Copy link
Contributor

dmgd commented Oct 18, 2014

this test can be made deterministic by using the test scheduler for interval(..) instead of the computation scheduler - dmgd@e95d4a4

@benjchristensen
Copy link
Member Author

This is a test for the actual Scheduler implementation in ExecutorSchedulerTest so the intent is to be testing real concurrency.

@dmgd
Copy link
Contributor

dmgd commented Oct 18, 2014

Sorry, I should have been more clear in my earlier comment. I'm not talking about using the test scheduler in place of the scheduler under test. Currently the call to interval(..) is Observable.interval(50, TimeUnit.MILLISECONDS) which uses the computation scheduler. I'm suggesting that instead of that you use the test scheduler, i.e Observable.interval(50, TimeUnit.MILLISECONDS, testScheduler).

The scheduler under test will still observe all emissions, and the original issue #431 that when the observing thread requests unsubscription then the original emitting thread never sees isUnsubscribed() == true is still tested (because if it's not then the second call to advanceTime() will result in further emissions!)

With the current test there is always going to be a chance that the computation thread will get some time to generate more emissions -- we don't have control over the way that the underlying system schedules threads, so we can't control that!!

There are other ways to accomplish determinism here. E.g. You could put a countdown latch inside the map--

.map(new Func1<Long, Long>() {
    @Override
    public Long call(Long aLong) {
      return aLong;
      if (1 == aLong) {
        latch.await();
      }
    }
})

--however this really just does exactly the same thing as using the test scheduler: it ensures that once the second interval(..) emission has been emitted, that the thread on which it is emitted does not get a chance to run until the thread on which the emission is observed has had a chance to react to the emission. Either way works fine and results in the test being deterministic!

@akarnokd
Copy link
Member

akarnokd commented Feb 5, 2015

Haven't seen this fail since we moved to travis so closing.

@akarnokd akarnokd closed this as completed Feb 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants