Skip to content

fix(VirtualTimeScheduler): rework flush so it won't lose actions - #4433

Merged
benlesh merged 2 commits into
ReactiveX:masterfrom
baizulin:bugfix/virtual-time-scheduler-flush-loses-actions
Jan 30, 2019
Merged

benlesh merged 2 commits into
ReactiveX:masterfrom
baizulin:bugfix/virtual-time-scheduler-flush-loses-actions

Conversation

@baizulin

Copy link
Copy Markdown
Contributor

Description:
Previously VirtualTimeScheduler.flush would lose the action that was on a verge of maxFrames limit which rendered testing of observables that are ticking indefinitely impossible. After the fix the user can set maxFrames, flush, make assertions and repeat the process as many times as needed.

Related issue:
None

Previously VirtualTimeScheduler.flush would lose the action that was on a verge of maxFrames limit which rendered testing of observables that are ticking indefinitely impossible. After the fix the user can set maxFrames, flush, make assertions and repeat the process as many times as needed.
@cartant

cartant commented Dec 21, 2018

Copy link
Copy Markdown
Collaborator

Thanks for the PR. I will have a look at this tomorrow. (I also need to look at what's causing the dtslint run with TypeScript next to effect the errors in the Travis log - something that's unrelated to this PR.)

@baizulin

baizulin commented Jan 2, 2019

Copy link
Copy Markdown
Contributor Author

@cartant Hi! Did you have a chance to take a look?

@cartant

cartant commented Jan 3, 2019 •

Copy link
Copy Markdown
Collaborator

I looked at it briefly.

My concern is that maxFrames is no longer relevant given that its set to POSITIVE_INFINITY in TestScheduler#run. And that is the recommended mechanism for writing marble tests.

I'll wait to see what others have to say about it.

@benlesh benlesh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please review and make the suggested changes.

Thanks, @baizulin!

Comment thread src/internal/scheduler/VirtualTimeScheduler.ts
Comment thread spec/schedulers/VirtualTimeScheduler-spec.ts Outdated
Comment thread spec/schedulers/VirtualTimeScheduler-spec.ts Outdated
Comment thread spec/schedulers/VirtualTimeScheduler-spec.ts Outdated
@benlesh

benlesh commented Jan 9, 2019

Copy link
Copy Markdown
Member

@cartant ... VirtualTimeScheduler may be used on it's own, and this seems like a valid fix/bug to me.

Comment thread src/internal/scheduler/VirtualTimeScheduler.ts

@cartant cartant left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@baizulin

Copy link
Copy Markdown
Contributor Author

@cartant @benlesh guys let's fix this bug already, it's been over a month

@benlesh

benlesh commented Jan 30, 2019

Copy link
Copy Markdown
Member

@baizulin ... I appreciate that you put in the effort to submit this PR, I understand it's been a while and that can be frustrating. Please try to be respectful of our team members and our time, at this time all RxJS work is done by volunteers in our spare time.

Comment thread src/internal/scheduler/VirtualTimeScheduler.ts
@benlesh
benlesh merged commit d068bc9 into ReactiveX:master Jan 30, 2019
@lock lock Bot locked as resolved and limited conversation to collaborators Mar 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants