TestScheduler can miss tests when flushing

[TPReal]

Bug Report

Current Behavior
Calling expectObservable adds tests to flushTests field, and calling toBe on the expect, marks those tests as ready. Then calling flush should go over flushTests, and execute and remove those that are ready at this point. The logic of removing is broken, and different tests get removed than should be. As a result, some tests might never be checked, and some are checked multiple times.

The problem is that TestScheduler.flush is broken. When removing items from flushTests, it uses the index of the item from before removal, so for example if there are 3 tests, [ready1, ready2, notReady], then flush() will execute ready1, then remove at index 0, so ready1. Then execute ready2, then remove at index 1, but now it's notReady that is present at index 1. So after flush(), flushTests should be [notReady], but in fact it is [ready2].

Reproduction
https://stackblitz.com/edit/rxjs-testscheduler-flush-bug

  expectObservable(cold('-x')).toBe('-x');
  expectObservable(cold('-y')).toBe('-y');
  const expectation=expectObservable(cold('-z')).toBe.bind(null,'-q');
  flush(); // Causes the check z vs q not to be checked at all.
  expectation();

Expected behavior
The expectation in the code above should be executed, and the test should fail, because 'z' is not equal to 'q'. The test of 'y' should be executed once, while it is executed twice.

Environment

• RxJS version: 6.2.1

Possible Solution
Change the relevant part of TestScheduler.flush to:

  this.flushTests = this.flushTests.filter(test => {
    if (test.ready) {
      this.assertDeepEqual(test.actual, test.expected);
      return false;
    }
    return true;
  });

Additional context
I'm testing a service in an Angular application, calling also a material dialog. For some reason, I need to call flush() at some point, between subscribing to an observable (by calling expectObservable) and actually comparing its result with the expectation.

[ladyleet]

@cartant 👍 to close?

[cartant]

@ladyleet This is the issue for #3901. It'll be closed automatically when that's merged. However, I guess there's no reason it couldn't be closed right now.