Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

baizulin
Copy link
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
Copy link
Collaborator

cartant commented Dec 21, 2018

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
Copy link
Contributor Author

baizulin commented Jan 2, 2019

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

@cartant
Copy link
Collaborator

cartant commented Jan 3, 2019

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.

Copy link
Member

@benlesh benlesh left a comment

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!

spec/schedulers/VirtualTimeScheduler-spec.ts Outdated Show resolved Hide resolved
spec/schedulers/VirtualTimeScheduler-spec.ts Outdated Show resolved Hide resolved
spec/schedulers/VirtualTimeScheduler-spec.ts Outdated Show resolved Hide resolved
@benlesh
Copy link
Member

benlesh commented Jan 9, 2019

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

Copy link
Collaborator

@cartant cartant left a comment

Choose a reason for hiding this comment

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

LGTM

@baizulin
Copy link
Contributor Author

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

@benlesh
Copy link
Member

benlesh commented Jan 30, 2019

@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.

@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