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(schedulers): Fix Asap and AnimationFrame Schedulers to act like the Async Scheduler if delay > 0 #1953

Merged
merged 1 commit into from
Sep 24, 2016

Conversation

trxcllnt
Copy link
Member

Description:
Asap and AnimationFrame Schedulers should act like the Async Scheduler if delay > 0.

Related issue (if exists):
#1952

@trxcllnt trxcllnt force-pushed the fix-scheduler-flush-action-arg branch from 04effb6 to 19b9c13 Compare September 18, 2016 02:58
@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 97.187% when pulling 19b9c13 on trxcllnt:fix-scheduler-flush-action-arg into e91d113 on ReactiveX:master.

@trxcllnt trxcllnt force-pushed the fix-scheduler-flush-action-arg branch from 19b9c13 to 12a34ec Compare September 18, 2016 03:01
@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 97.187% when pulling 12a34ec on trxcllnt:fix-scheduler-flush-action-arg into e91d113 on ReactiveX:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 97.187% when pulling 12a34ec on trxcllnt:fix-scheduler-flush-action-arg into e91d113 on ReactiveX:master.

@@ -20,6 +20,19 @@ describe('Scheduler.animationFrame', () => {
}
});

it('should act like the async scheduler if delay > 0', (done: MochaDone) => {
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to suggest to use fakeTimer same as other test case (here) to avoid intermittent test failure by timing condition of test execution environment (usually occurs on mobile emulator browser test)

@trxcllnt trxcllnt force-pushed the fix-scheduler-flush-action-arg branch from 12a34ec to 2441ed0 Compare September 23, 2016 23:46
@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 97.062% when pulling 2441ed0 on trxcllnt:fix-scheduler-flush-action-arg into 48a8689 on ReactiveX:master.

@trxcllnt trxcllnt force-pushed the fix-scheduler-flush-action-arg branch from 2441ed0 to be544f1 Compare September 23, 2016 23:52
@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 97.062% when pulling be544f1 on trxcllnt:fix-scheduler-flush-action-arg into 48a8689 on ReactiveX:master.

@jayphelps
Copy link
Member

jayphelps commented Sep 23, 2016

@trxcllnt can we get a regression test for #1952, one where these changes are being flexed in a real observable? Right now, by just looking at the code in the PR, I can't say that this truly does fix it cause there's no "proof"

@trxcllnt trxcllnt force-pushed the fix-scheduler-flush-action-arg branch from be544f1 to 33e4bed Compare September 24, 2016 00:03
@trxcllnt
Copy link
Member Author

@jayphelps sure thing

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 97.062% when pulling 33e4bed on trxcllnt:fix-scheduler-flush-action-arg into 48a8689 on ReactiveX:master.

@trxcllnt trxcllnt force-pushed the fix-scheduler-flush-action-arg branch from 33e4bed to d5c682c Compare September 24, 2016 00:18
@trxcllnt
Copy link
Member Author

@jayphelps Scheduler with Observable tests added in spec/observables/interval-spec.ts

@coveralls
Copy link

coveralls commented Sep 24, 2016

Coverage Status

Coverage increased (+0.06%) to 97.187% when pulling 12a34ec on trxcllnt:fix-scheduler-flush-action-arg into e91d113 on ReactiveX:master.

@jayphelps
Copy link
Member

LGTM

@jayphelps jayphelps merged commit e20cdb7 into ReactiveX:master Sep 24, 2016
@lock
Copy link

lock bot commented Jun 6, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
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.

4 participants