Added a failing test to illustrate bug in SignalProducer.sampleOn(SignalProducer)

[NachoSoto]

The docs read "If sampler fires before a value has been observed on self, nothing happens.".
However, in this case the sampler "fires" synchronously upon subscription. Thus, I don't think this is the expected behavior.

If I change the test slightly:

let sampler = SignalProducer<(), NoError>(value: ())
    .delay(0.1, onScheduler: QueueScheduler())

let result = producer.sampleOn(sampler)

var valueReceived: Int?
result.startWithNext { valueReceived = $0 }

expect(valueReceived).toEventually(equal(1))

Then the test passes. So adding a small delay to introduce a scheduler jump is a workaround.

[neilpa]

The gist of the problem is that sampleOn is implemented with lift which is effectively right-biased over it's operands. This means sampler starts and completes before the sampled producer when it's synchronous.

[NachoSoto]

@neilpa that makes sense.
That reminds me of the commend I added to lift:

/// Note: starting the returned producer will start the receiver of the operator,
/// which may not be adviseable for some operators.

I haven't thought much about it, but there might be a way to fix this by having a slightly different version of lift, or sharing the implementation of sampleOn in a different way?

[neilpa]

I was actually wondering if we should invert the startWithSignal order for the producers in lift. Dual synchronous producers also affect takeUntil and takeUntilReplacement but it's less clear for those what the "right" answer is. But since this is broken for sampleOn I would make an argument for the consistency of being left-biased.

The remaining binary lift implementations are combineLatestWith and zipWith which should behave the same regardless of order.

[neilpa]

I was wrong above. Inverting the subscription order does affect combineLatestWith and breaks this test. I guess we'll need something like liftR and liftL, but hopefully someone has better names.

[NachoSoto]

Inverting the subscription order does affect combineLatestWith and breaks this test

That's good to hear, actually!

I think liftLeft and liftRight sound good!

[NachoSoto]

@neilpa I'm happy to implement that if that sounds good, unless you already have a working implementation of liftLeft?

[neilpa]

Let me cleanup my change and I'll push it shortly

[NachoSoto]

Reopened in #2571.