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

feat(TestScheduler): add an animate "run mode" helper #5607

Merged
merged 13 commits into from
Jul 31, 2020

Conversation

cartant
Copy link
Collaborator

@cartant cartant commented Jul 26, 2020

Description:

This PR:

  • Adds a Date timestamp provider.

  • Adds a performance timestamp provider.

  • Adds a requestAnimationFrame provider.

  • All providers have delegate properties that are undefined by default but are used - by TestScheduler#run - to override the default behaviour. This mirrors the delegate implementation detail that's used with the schedulers.

  • Removes the arrow function from the static Scheduler.now - it's unnecessary, as the Date.now call now occurs within the dateTimestampProvider.

  • The animationFrames observable now uses the requestAnimationFramesProvider.

  • TestScheduler#run sets the delegates for the providers, with the requestAnimationFramesProvider's delegate working in conjunction with a animate() marble that's added to the run helpers:

    testScheduler.run(({ animate }) => {
      animate('---x---x---x---x---x');
      /* ... */
    });

    Each time an animation/repaint occurs, all queued rAF callbacks will be executed - in the order in which they were scheduled - and each will be passed the same timestamp - the TestScheduler's now value - which is also what will be returned from the timestamp providers - as the TestScheduler itself is the delegate.

  • Adds tests for the animate run helper.

  • Updates API guardian for the inclusion of the animate run helper.

  • Reimplements the animationFrames tests to use the run helper and the provider - which is spied upon - and removes the rAF mock.

  • Fixes the rAF provider so that cancellation of already fulfilled requests (upon unsubscription) is not attempted.

Related issue (if exists): #5438 and #5475

@cartant cartant force-pushed the issue-5475 branch 3 times, most recently from f6f6988 to 6cb1c47 Compare July 27, 2020 02:06
@cartant cartant changed the title WIP: Timestamp and rAF providers for TestScheduler "run mode" Timestamp and rAF providers for TestScheduler "run mode" Jul 28, 2020
@cartant cartant marked this pull request as ready for review July 28, 2020 04:28
@cartant cartant requested a review from benlesh July 28, 2020 04:28
@cartant cartant changed the title Timestamp and rAF providers for TestScheduler "run mode" feat(TestScheduler): add repaints "run mode" helper Jul 28, 2020
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.

FIRST: I've rarely been as excited about a change to this part of the code. This is pretty much the best addition since "run mode".

Just a few very minor things.

src/internal/testing/TestScheduler.ts Show resolved Hide resolved
src/internal/testing/TestScheduler.ts Outdated Show resolved Hide resolved
src/internal/testing/TestScheduler.ts Outdated Show resolved Hide resolved
@cartant
Copy link
Collaborator Author

cartant commented Jul 28, 2020

@benlesh I'll make those changes and will then add some docs - I was holding off on documenting it until it had been reviewed.

@cartant
Copy link
Collaborator Author

cartant commented Jul 29, 2020

@benlesh I've made some changes:

  • I've changed the name of the repaints run helper to animate.
  • I've moved the delegate and the run helper implementation into a createAnimator method. This is more consistent with the current implementation. FWIW, I'm not keen on making animate a method that needs to be bound to this and needs member properties, etc.
  • I've added a TSDoc comment to run - there was no existing comment/documentation - and I've linked it to an updated marble testing guide.

ATM, the documentation in the guide is a little light, but I think it's enough, for now. Once this PR is merged, I am going to:

  • rebase and fix up the PR that adds the timestamp and elapsed properties to the animationFrames observable's value.
  • look at removing/refactoring the delegate that's in AsyncScheduler, as I think it ought to be possible to add setInterval and setImmediate providers (and delegates). Doing that would allow for tests to actually use all of the schedulers within 'run mode'. ATM, they are bypassed and there is no way to write a test in which AsyncScheduler tasks are executed after subsequently-scheduled AsapScheduler tasks - i.e. there is no differentiation between virtual micro and macro tasks in the TestScheduler. The change would also mean that the AnimationFrameScheduler would work with the animate run helper.

If the docs need more work, I can look at them after I've done the above. Or someone else can do it.

@cartant cartant requested a review from benlesh July 29, 2020 04:43
@cartant cartant changed the title feat(TestScheduler): add repaints "run mode" helper feat(TestScheduler): add an animate "run mode" helper Jul 29, 2020
cartant added a commit to tmair/rxjs that referenced this pull request Jul 30, 2020
@benlesh benlesh merged commit edd6731 into ReactiveX:master Jul 31, 2020
@benlesh
Copy link
Member

benlesh commented Jul 31, 2020

Nice work, @cartant! I'm very pleased to see this land!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants