Comprehensive marble tests for groupBy

[benlesh]

Should duplicate the current RxJS 3/4 tests

Should adhere to our contribution guidelines and operator authoring docs.

[staltz]

I'm picking this one.

[staltz]

Just a quick report on this issue: writing the marble tests for this operator is not trivial: I am seeing how to improve the TestScheduler so that it can assert also Observables-of-Observables, which is what this operator returns. It's a better solution to, say, applying .mergeAll() after groupBy, or reverting to raw manual tests with the scheduler.

I also fixed a small bug in groupBy, so eventually I'll submit a PR with 3 commits, one for tests, another for groupBy fix, another to TestScheduler improvements.

[staltz]

Example: we will be able to write this

it('should group values with a key comparer', function () {
    var values = {
      a: '  foo',
      b: ' FoO ',
      c: 'baR  ',
      d: 'foO ',
      e: ' Baz   ',
      f: '  qux ',
      g: '   bar',
      h: ' BAR  ',
      i: 'FOO ',
      j: 'baz  ',
      k: ' bAZ ',
      l: '    fOo    '
    };
    var e1 = hot('-1--2--^-a-b-c-d-e-f-g-h-i-j-k-l-|', values);
    var expected =      '--w---x---y-z-------------|';
    var w = cold(         'a-b---d---------i-----l-|', values);
    var x = cold(             'c-------g-h---------|', values);
    var y = cold(                 'e---------j-k---|', values);
    var z = cold(                   'f-------------|', values);
    var expectedValues = { w: w, x: x, y: y, z: z };

    var source = e1
      .groupBy(function (x) { return x.toLowerCase().trim(); });
    expectObservable(source).toBe(expected, expectedValues);
  });

Relevant also for window operators.

[benlesh]

Ah, I see the problem.

I think perhaps the API should remain the same, and we'll just need to analyze expected values to see if they are either ColdObservable or HotObservable, and pull the marbles out of them, convert them to messages, and put them in the outer message value . In contrast, we'll need to subscribe to the resulting observables and do the same thing. The toDeepEqual assertion should handle the rest.

Basically, you're going to create Arrays of messages, that sometimes have values that are Arrays of messages.

[benlesh]

This is one of the reasons I wanted this one to be one of the ones that was tackled first.

[staltz]

I think perhaps the API should remain the same, and we'll just need to analyze expected values to see if they are either ColdObservable or HotObservable, and pull the marbles out of them, convert them to messages, and put them in the outer message value .

Basically, you're going to create Arrays of messages, that sometimes have values that are Arrays of messages.

Yep, I did that. All was good until I noticed that we need to write tests to assert that the outer Observable can be unsubscribed early, but you can still keep subscription to the inner Observables, which will keep emitting, because those are GroupSubjects (hot). For these, I wrote this test:

  it('should allow the outer to be unsubscribed early but inners continue', function () {
   var values = {
     a: '  foo',
     b: ' FoO ',
     c: 'baR  ',
     d: 'foO ',
     e: ' Baz   ',
     f: '  qux ',
     g: '   bar',
     h: ' BAR  ',
     i: 'FOO ',
     j: 'baz  ',
     k: ' bAZ ',
     l: '    fOo    '
   };
   var e1 = hot('-1--2--^-a-b-c-d-e-f-g-h-i-j-k-l-|', values);
   var unsub =         '         !';
   var expected =      '--w---x---';
   var foo =             'a-b---d---------i-----l-|';
   var bar =                 'c-------g-h---------|';
   var expectedInners = { foo: foo, bar: bar };
   var expectedInnersKeys = { w: 'foo', x: 'bar' };

   var source = e1
     .groupBy(function (x) { return x.toLowerCase().trim(); })
     .do(function (groupSubject) {
       expectObservable(groupSubject).toBe(expectedInners[groupSubject.key], values);
     })
     .map(function (g) { return g.key; });
   expectObservable(source, unsub).toBe(expected, expectedInnersKeys);
  });

But it doesn't work in practice, I'm still frying my brain trying to fix the TestScheduler to support this type of test.

[benlesh]

In the example above, shouldn't:

   var foo =             'a-b---d---------i-----l-|';
   var bar =                 'c-------g-h---------|';

be

   var foo =        cold('a-b---d---------i-----l-|');
   var bar =            cold('c-------g-h---------|');

So you can run them and collect the results in the expect values?

[benlesh]

I guess what I'm saying is every time a value is emitted that happens to be an observable, you subscribe to it and create the message array, likewise for expectations, when you're converting the outer expectation to an array of messages, if you notice the value is an observable, you pull the marbles from it and create an inner array of messages and use that as the value for the outer message.

Make sense?

It doesn't seem like unsubscription from the outer should affect that, since you're just building arrays to compare.

[staltz]

In the example above, shouldn't:

   var foo =             'a-b---d---------i-----l-|';
   var bar =                 'c-------g-h---------|';

be

   var foo =        cold('a-b---d---------i-----l-|');
   var bar =            cold('c-------g-h---------|');

So you can run them and collect the results in the expect values?

Nope because

expectObservable(groupSubject).toBe(expectedInners[groupSubject.key], values);

treats the inner Observable groupSubject as any regular testable Observable, and expectedInners[groupSubject.key] as any regular marble string.

I guess what I'm saying is every time a value is emitted that happens to be an observable, you subscribe to it and create the message array, likewise for expectations, when you're converting the outer expectation to an array of messages, if you notice the value is an observable, you pull the marbles from it and create an inner array of messages and use that as the value for the outer message.

Yes, I totally understand this part, and this is working. However, it doesn't capture that tricky outer-unsubscribed but inners-still-subscribed test case for groupBy. I'm still debugging why.

It doesn't seem like unsubscription from the outer should affect that, since you're just building arrays to compare.

The array building process for the inner runs on the virtual time scheduler. I'm quite sure the unsubscription of the outer affects the inner hots, I moved around the unsubscription marker just to be sure. It could be a problem in TestScheduler or even with groupBy operator implementation in the worst case.

It might be hard for you to visualize what I'm saying, what do you think if I submit a work-in-progress PR? I can then rebase it heavily so it would look like a few nice commits.

[benlesh]

Oh oh... I'm saying you'd just do only expectObservable(source).toBe(expected, values) and that's all. Internally, it would build out all inner observables into message arrays and compare them all in the same check, since it would be a array of arrays (basically).

Do you see what I mean?

[staltz]

Yes I do, and I had tried that one already as well.

  it('should allow the outer to be unsubscribed early but inners continue', function () {
    var values = {
      a: '  foo',
      b: ' FoO ',
      c: 'baR  ',
      d: 'foO ',
      e: ' Baz   ',
      f: '  qux ',
      g: '   bar',
      h: ' BAR  ',
      i: 'FOO ',
      j: 'baz  ',
      k: ' bAZ ',
      l: '    fOo    '
    };
    var e1 = hot('-1--2--^-a-b-c-d-e-f-g-h-i-j-k-l-|', values);
    var unsub =         '         !';
    var expected =      '--w---x---';
    var w = cold(         'a-b---d---------i-----l-|', values);
    var x = cold(             'c-------g-h---------|', values);
    var expectedValues = { w: w, x: x };

    var source = e1
      .groupBy(function (x) { return x.toLowerCase().trim(); });
    expectObservable(source, unsub).toBe(expected, expectedValues);
  });

[benlesh]

Attn @trxcllnt ... any ideas?

[trxcllnt]

@staltz "assert that the outer Observable can be unsubscribed early, but you can still keep subscription to the inner Observables, which will keep emitting, because those are GroupSubjects (hot)."

Is this sentence correct? I believe disposing the groupBy subscription should dispose the inner GroupedObservables.

[staltz]

What does this test assert then? https://github.com/Reactive-Extensions/RxJS/blob/master/tests/observable/groupby.js#L829-L921

[staltz]

Check this

var outer = Rx.Observable.interval(1000).take(10)
  .groupBy(x => x % 3);

var inners = {};
var innerSubs = {};

var outerSub = outer.subscribe(groupSubject => {
  inners[groupSubject.key] = groupSubject;
  innerSubs[groupSubject.key] = groupSubject.subscribe(i => {
    console.log('inner ' + groupSubject.key + ': ' + i);
  });
})

setTimeout(() => {
  console.log('unsubscribe outer');
  outerSub.dispose();
}, 3500);

Produces the output on console:

"inner 0: 0"
"inner 1: 1"
"inner 2: 2"
"unsubscribe outer"
"inner 0: 3"
"inner 1: 4"
"inner 2: 5"
"inner 0: 6"
"inner 1: 7"
"inner 2: 8"
"inner 0: 9"

---

On the other hand, this code (where groupBy was swapped with map):

var outer = Rx.Observable.interval(1000).take(10)
  .map(x => {
    var inner = Rx.Observable.just(10 * x);
    inner.key = x;
    return inner;
  });

var inners = {};
var innerSubs = {};

var outerSub = outer.subscribe(groupSubject => {
  inners[groupSubject.key] = groupSubject;
  innerSubs[groupSubject.key] = groupSubject.subscribe(i => {
    console.log('inner ' + groupSubject.key + ': ' + i);
  });
})

setTimeout(() => {
  console.log('unsubscribe outer');
  outerSub.dispose();
}, 3500);

produces the output:

"inner 0: 0"
"inner 1: 10"
"inner 2: 20"
"unsubscribe outer"

Try it in the JSBin: https://jsbin.com/weyaju/1/edit?js,console

[staltz]

Ok, now I'm fairly convinced we have a bug in RxJS Next groupBy.

Check how RxJS legacy is collecting subscriptions to both outer and inner observer in a RefCountDisposable. https://github.com/Reactive-Extensions/RxJS/blob/master/src/core/linq/observable/groupbyuntil.js#L41

And how that RefCountDisposable is put in a GroupedObservable https://github.com/Reactive-Extensions/RxJS/blob/master/src/core/linq/groupedobservable.js#L10.

I think this means if there are still subscribers to either the outer observable or the inner group subjects, the outer subscriber will stay subscribed.

[staltz]

Please see PR #484.

[staltz]

All groupBy tests are implemented with PR #484, but I noticed groupBy in RxJS Next covers also the use case for groupByUntil, and we are still missing tests for that. So I would keep this issue open, and I'll work on groupByUntil tests next on a different PR, so we don't clutter the previous PR.

[staltz]

This issue should be resolved/closed once #508 is merged.

[staltz]

We can finally close this one.