Speed up unnecessarily and unintentionally slow waitForAnimationsToFinish calls
#1301
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dnkoutso
approved these changes
Sep 5, 2024
amorde
approved these changes
Sep 5, 2024
justinseanmartin
force-pushed
the
jmartin/long-animations
branch
from
cca866a
to
9ad2cb4
Compare
dostrander
approved these changes
Sep 6, 2024
justinseanmartin
pushed a commit
to justinseanmartin/KIF
that referenced
this pull request
Sep 19, 2024
It doesn't really make sense that the default animation wait time is the same as the stabilization time. This effectively means the behavior of scanning the UI hierarchy for animations in progress is never utilized for most tests out of the box. For what its worth, we've used a 2s animation wait time and .1s stabilization time as our defaults for a very long time. This should generally not slow down any tests if there weren't any animations in progress. If there were animations in progress, this is probably a worthwhile tradeoff to make things more reliable. This can still be configured globally to be overidden. As part of this change, use `waitForAnimationsToFinish` in place of a static .5s sleep for the `longPress` action. There will be a followup PR kif-framework#1301 that should speed up some scenarios where we're waiting longer than desired for animations to complete.
justinseanmartin
pushed a commit
to justinseanmartin/KIF
that referenced
this pull request
Sep 19, 2024
It doesn't really make sense that the default animation wait time is the same as the stabilization time. This effectively means the behavior of scanning the UI hierarchy for animations in progress is never utilized for most tests out of the box. For what its worth, we've used a 2s animation wait time and .1s stabilization time as our defaults for a very long time. This should generally not slow down any tests if there weren't any animations in progress. If there were animations in progress, this is probably a worthwhile tradeoff to make things more reliable. This can still be configured globally to be overidden. As part of this change, use `waitForAnimationsToFinish` in place of a static .5s sleep for the `longPress` action. There will be a followup PR kif-framework#1301 that should speed up some scenarios where we're waiting longer than desired for animations to complete.
justinseanmartin
force-pushed
the
jmartin/long-animations
branch
2 times, most recently
from
bd90a8f
to
7e3217d
Compare
This was referenced Sep 23, 2024
Merged
added 2 commits
September 23, 2024 16:12
Two changes: * We won't recurse into layers that are hidden * We don't count animations that run for longer than 1 minute
Credit to @amorde for finding this
justinseanmartin
force-pushed
the
jmartin/long-animations
branch
from
7e3217d
to
27d9ff1
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We identified 3 issues where calls to
waitForAnimationsToFinishwould wait longer than we'd expect:KIFTouchVisualizerViewwas added to show where touch events are happening, but they animate in a separate window and have an alpha fade for .75s. Every tap action then will wait for a minimum of .75s after the tap for that animation to complete, which was unintentional. Credit to @amorde for finding and testing this issue.This does alter test timing slightly, and even if your code doesn't explicitly use
waitForAnimationsToFinish, it is also called automatically called after UI actions, such as tapping the screen. If you have tweaked the timeouts to be faster and more aggressive, you might encounter some race conditions. You might be able to work around some of this temporarily by increasing theKIFTestStepDefaultAnimationStabilizationTimeoutto .75s or 1s, but that will also slow down the execution of all of your tests (likely back to the speed before this change lands).This change has turned up quite a few race conditions in our tests, but nothing that seemed like it wasn't something that shouldn't have more explicit validation of the screen state.
I recommend reviewing this with whitespace diffs ignored.
CC: @amorde @dnkoutso