Skip to content

Conversation

@mithro
Copy link

@mithro mithro commented Jun 3, 2014

Not ready to merge, just sending for review.

@mithro
Copy link
Author

mithro commented Jun 3, 2014

@shans @ewilligers @rjwright - As requested, please take a look.

@mithro
Copy link
Author

mithro commented Jun 3, 2014

Copy link
Collaborator

Choose a reason for hiding this comment

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

This test doesn't measure much. Once the player is cancelled, calling cancel() is basically a no-op.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, this is the multi-player problem right? Did we decide on a solution?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ultimately we decided we couldn't do true black-box testing. With a test like this we can go three ways:
(1) if the test is trivial (the value isn't cached and isn't calculated) then we don't bother keeping it
(2) if the test is calculated but not cached, then we keep the test in the form this one is
(3) (this is the case for this test) if the test is cached, then we reset the cached status between calls, if possible, and test the combination of check-reset. Here, setting the source of the player back to animation voids the cancel, so doing that after player.cancel() would be appropriate.

@shans
Copy link
Collaborator

shans commented Jun 3, 2014

I think we mainly just need to set the source of the player after cancel() across the tests. These are looking good.

Copy link
Author

Choose a reason for hiding this comment

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

Comment from @shans

reverse isn't a state - calling reverse is somewhat similar to setting playbackRate to -1
(but it does more work too). Change these to a test that just calls reverse() multiple times and re-run.

@mithro
Copy link
Author

mithro commented Jun 13, 2014

Moved the selected test to the top level directory. What should I do with the generator and related directory?

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.

3 participants