Sample operator?

[staltz]

I don't know how often people use or need the sample operator, but the primary reason I've been avoiding it is because I think it's semantics are "broken" in RxJS 2.x. Check this marble diagram:

I expect each event B, C and D to sample 2 on the resulting Observable, but only the first sampling event does so.

Any other opinions on this?

[mattpodwysocki]

@staltz yes, it did have a problem but has since been fixed as of Issue #727 Reactive-Extensions/RxJS#727 . Please check the issues from the existing RxJS to ensure we're not making extra efforts for ourselves.

[staltz]

@mattpodwysocki is that fix in v2.5.3? RxMarbles is running on v2.5.3 and sample is still broken http://rxmarbles.com/#sample

[benlesh]

@staltz ... FWIW, I thought this was how sample was supposed to work? It doesn't emit a new sample if the source hasn't emitted a new one?

[mattpodwysocki]

@Blesh if all else fails, check the docs and the tests https://github.com/Reactive-Extensions/RxJS/blob/master/doc/api/core/operators/sample.md 😄

[staltz]

@Blesh Apparently, there is a test for the case I'm talking about, meaning... sample is working according to its semantics.

test('Sample Sampler Simple3', function () {
    var scheduler = new TestScheduler();

    var xs = scheduler.createHotObservable(
      onNext(150, 1),
      onNext(220, 2),
      onNext(240, 3),
      onNext(290, 4),
      onCompleted(300)
     );

    var ys = scheduler.createHotObservable(
      onNext(150, ""),
      onNext(210, "bar"),
      onNext(250, "foo"),
      onNext(260, "qux"),
      onNext(320, "baz"),
      onCompleted(500)
    );

    var res = scheduler.startWithCreate(function () {
      return xs.sample(ys);
    });

    res.messages.assertEqual(
      onNext(250, 3),
      onNext(320, 4),
      onCompleted(320 /* on sampling boundaries only */)
    );

    xs.subscriptions.assertEqual(
      subscribe(200, 300)
    );

    ys.subscriptions.assertEqual(
      subscribe(200, 320)
    );
  });

Notice the sampler Observable samples at times 250 and 260, but the resulting messages are

onNext(250, 3),
onNext(320, 4),
onCompleted(320 /* on sampling boundaries only */)

What I'm proposing is that sample would emit the same value as previously emitted. If distinctness is an issue, then one can just use distinctUntilChanged.

[mattpodwysocki]

@staltz @Blesh this is why before we go too much further we need virtual time and a test scheduler to ensure the correctness if we're introducing timing operations.

[headinthebox]

this is why before we go too much further we need virtual time and a test scheduler

Absolutely!

[benlesh]

this is why before we go too much further we need virtual time and a test scheduler to ensure the correctness if we're introducing timing operations.

I don't disagree, however, I think the "magic" in the tests in RxJS 2 prohibited contribution. Tests should be first, and foremost readable. And, while I can read them now, it still takes a little mental math at times. We need a better method for doing this. Something clear and easy to follow, I don't even care how verbose it is, as long as it's readable.

I also think that we should only use such tests when the operators we're testing are temporal in nature. Using them for any non-temporal purpose is overkill.

[ktrott]

Do we have an issue for readable unit tests? We should move these comments over there. I know @trxcllnt was hoping to reuse a lot of the RxJS unit tests but was waiting to see if they were going to be converted from QUnit.

[headinthebox]

@Blesh can you elaborate a bit about why the test are not "readable"? They are simply marble diagrams in text format.

[staltz]

How about generating onNext(time, event) boilerplate automatically from ASCII marble diagrams?

var xsdiagram =  '--1---2---3---4---|->';
var ysdiagram =  '--0-----0----|---->';
var resdiagram = '--1-----2----3---->';

var xs = scheduler.createHotObservableFromDiagram(xsdiagram);
var ys = scheduler.createHotObservableFromDiagram(ysdiagram);

var res = scheduler.startWithCreate(function () {
  return xs.sample(ys);
});

res.messages.assertEqualDiagram(resdiagram);

PS: I could do this.

[trxcllnt]

@staltz I love this idea

[benlesh]

@staltz @trxcllnt This is a decent idea...

@headinthebox what I mean by not "readable" is that there is a fair bit of magic occurring in some of the tests. The observables are magically subscribed to at 200 or something like that, the author of the tests is left to know how to add in their selected time values, plus the magic start value, plus any additional modifiers that might happen in their operator. The tests don't read explicitly enough to convey to a newcomer what they're testing and why. I think this is bad because that means there are fewer eyes that can read the tests and check them for sanity. It also may turn away contributors with good ideas that have a hard time figuring out the testing harness.

[benlesh]

The more I think about it, the more I like @staltz's proposal, particularly if we convert over to ES6 for the tests.

I think we could create some jasmine-esque helpers that will do things like subscribed to the scheduled observables and flush the virtual time to assert expectations:

describe('some test', _ => {
  it('should do something', done => {
    let test = new TestScheduler(),
        cold = test.createColdObservable;

    let a = 1, b = 2, c = 3;

    let xs =        cold('----a---b-----|', { a, b });
    let ys =        cold('------c-------|', { c });
    let expected =  cold('----a-c-b-----|', { a, b, c });

    let result = xs.merge(ys);

    expectObservable(result).toEqual(expected);
  });
});

Tricker would be the hot observables, because you want to have a little gap in time before subscription, perhaps we could delinate subscription time with another character, such as a $:

describe('some test', _ => {
  it('should do something', done => {
    let test = new TestScheduler(),
        hot = test.createHotObservable;

    let a = 1, b = 2, c = 3, d = 4;

    let xs = hot('----a--$----b---c-----|', { a, b });
    let ys =        cold('------d-------|', { d });
    let expected =  cold('----b-d-c-----|', { b, c, d });

    let result = xs.merge(ys);

    expectObservable(result).toEqual(expected);
  });
});

[staltz]

Before I move to #151 to continue the testing helpers discussion, I'll just remind that my intuition would expect sample to behave as:

---1-----2-----3-----4----->
----0------0-0-----0------->
           sample
----1------2-2-----3------->

[benlesh]

closing this in favor of #178. This issue has a lot of side-topics in it that are important, but I don't want to confuse people who wish to put in a PR.