fix(TestScheduler): flush expectations correctly

[cartant]

Description:

Adds a failing test for the TestScheduler and applies the fix from #3898.

Related issue (if exists): #3898

[coveralls]

Coverage increased (+0.2%) to 96.944% when pulling 66eb866 on cartant:issue-3898 into f9318d8 on ReactiveX:master.

[TPReal]

There is one thing I'm not sure about. The original change that introduced the bug (2d5b3b2#diff-a65b432aff1fe2954db903fd7fe6b322R141) changed the code so that flushTests was only modified in place (so it could be readonly, but still wasn't). I don't see why this would be important not to change the reference, but on the other hand what would be any other reason to modify flush() at all??

It would be possible to fix the issue without modifying the flushTests reference, but the fix that I proposed also seems to work, and the code is nicer. So I don't know, just dumped my thoughts here.

[cartant]

@TPReal After I created the PR, I looked to see if you had any commits in this or other repos from which I could obtain your email address - so that I could credit you with the PR's commits - but I couldn't find any. However, I just noticed that your email address is on your GitHub profile page, so I'll make you the author in a short while - as it's all your work.

[jayphelps]

There is one thing I'm not sure about. The original change that introduced the bug (2d5b3b2#diff-a65b432aff1fe2954db903fd7fe6b322R141) changed the code so that flushTests was only modified in place (so it could be readonly, but still wasn't). I don't see why this would be important not to change the reference, but on the other hand what would be any other reason to modify flush() at all??

I don't recall specifics unfortunately. Presumably there was an issue with the previous implementation, but it's not clear why. You solution appears fine, and if not, someone will report and we can fix appropriately.

[jayphelps]

LGTM

[TPReal]

Thanks for the quick fix, and for the credit!

[cartant]

Interestingly, this change broke a test that I had in rxjs-marbles. This particular test explicitly calls flush and, because the test uses TestScheduler.run, flush is called again at the end of the test.

Prior to this change, that wasn't a problem, as each ready test was removed from the array before the assertion was performed. So the additional call to flush did nothing, as the ready test had been removed. Now, the array is filtered, so if an assertion fails, the array is unchanged and the same assertion will fail whenever flush is again called.

However, I doubt this is going to be an issue for any situation in which the TestScheduler is not itself being tested - which is, basically, what I was doing in the above-referenced test. I don't think there's anything that needs to be fixed. I'm just commenting to make a note of this, as I've had to work around it, today.