AnimationKeyframeEffectV8Test.* are flaky |
|||||
Issue descriptionPossibly caused by https://chromium-review.googlesource.com/c/chromium/src/+/1033431, possibly unrelated, but dtapuska@ reported this flaking on a CQ he ran and indeed: https://logs.chromium.org/v/?s=chromium%2Fbuildbucket%2Fcr-buildbucket.appspot.com%2F8947703890062830096%2F%2B%2Fsteps%2Fwebkit_unit_tests__with_patch_%2F0%2Flogs%2FAnimationKeyframeEffectV8Test.KeyframeCompositeOverridesEffect%2F0 [ RUN ] AnimationKeyframeEffectV8Test.KeyframeCompositeOverridesEffect ../../third_party/blink/renderer/core/animation/keyframe_effect_test.cc:39: Failure Expected equality of these values: 0 GetDocument().Timeline().currentTime() Which is: -0.000999999 Looking at the flakiness dashboard it does seem flaky: https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=webkit_unit_tests%20(with%20patch)&tests=AnimationKeyframeEffectV8Test.KeyframeCompositeOverridesEffect
,
May 1 2018
This appears to come from the test setup:
void SetUp() override {
PageTestBase::SetUp(IntSize());
element = GetDocument().CreateElementForBinding("foo");
GetDocument().GetAnimationClock().ResetTimeForTesting(
base::TimeTicks() +
base::TimeDelta::FromSecondsD(GetDocument().Timeline().ZeroTime()));
GetDocument().documentElement()->AppendChild(element.Get());
EXPECT_EQ(0, GetDocument().Timeline().currentTime());
}
currentTime() is not always 0 here, I'm seeing it fail in runs with times ~= -0.001
This is likely due to the halfway nature of the base::TimeTicks integration, with this code going double --> time ticks --> double. (Or maybe even more round-trips?)
I've yet to successfully reproduce locally, but working on it.
,
May 2 2018
,
May 2 2018
Note that DocumentTimeline::currentTime is in milliseconds, so a ~0.001 difference is basically an off-by-one error in base::TimeTicks units (as they are done in integer microseconds). The best solution to this is probably to continue the migration so that we don't have the double/TimeTick round-trip issues, but sadly the next step of that migration is quite complex :/.
,
May 2 2018
,
May 3 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/66481283272a27fc0a3663f1d8720b0c3ea80263 commit 66481283272a27fc0a3663f1d8720b0c3ea80263 Author: Stephen McGruer <smcgruer@chromium.org> Date: Thu May 03 23:23:56 2018 Speculative fix to de-flake AnimationKeyframeEffectV8Test It appears like calling ResetTimeForTesting is not actually needed for this test to pass. Since it is causing flake due to the double/TimeTick round-trips (see bug), just remove it. Bug: 838676 Change-Id: Ice3e2c88e3afbdf07c52abfb8bffc9d8e0d0c8b1 Reviewed-on: https://chromium-review.googlesource.com/1042380 Reviewed-by: Robert Flack <flackr@chromium.org> Commit-Queue: Stephen McGruer <smcgruer@chromium.org> Cr-Commit-Position: refs/heads/master@{#555902} [modify] https://crrev.com/66481283272a27fc0a3663f1d8720b0c3ea80263/third_party/blink/renderer/core/animation/keyframe_effect_test.cc
,
May 4 2018
From monitoring the flakiness dashboard, this seems to have done the trick. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by smcgruer@chromium.org
, May 1 2018