Bug 210156 - [ Mac iOS ] animations/animation-direction-normal.html is a flaky failure
Summary: [ Mac iOS ] animations/animation-direction-normal.html is a flaky failure
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
: 209362 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-04-07 14:53 PDT by Truitt Savell
Modified: 2020-04-23 10:23 PDT (History)
7 users (show)

See Also:


Attachments
Patch (18.44 KB, patch)
2020-04-23 09:56 PDT, Antoine Quint
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Truitt Savell 2020-04-07 14:53:41 PDT
animations/animation-direction-normal.html

This test has been a flaky failure on Mac and iOS for a while.

History:
https://results.webkit.org/?suite=layout-tests&test=animations%2Fanimation-direction-normal.html

Diff MacOS:
--- /Volumes/Data/slave/catalina-debug-tests-wk1/build/layout-test-results/animations/animation-direction-normal-expected.txt
+++ /Volumes/Data/slave/catalina-debug-tests-wk1/build/layout-test-results/animations/animation-direction-normal-actual.txt
@@ -1,4 +1,4 @@
-PASS - "margin-left" property for "box" element at 0.5s saw something close to: 50
-PASS - "margin-left" property for "box" element at 1s saw something close to: 100
+FAIL - "margin-left" property for "box" element at 0.5s expected: 50 but saw: 79.19999694824219
+FAIL - "margin-left" property for "box" element at 1s expected: 100 but saw: 129.3000030517578
 PASS - "margin-left" property for "box" element at 2.5s saw something close to: 50
 

Diff iOS:
--- /Volumes/Data/slave/ipados-simulator-13-debug-tests-wk2/build/layout-test-results/animations/animation-direction-normal-expected.txt
+++ /Volumes/Data/slave/ipados-simulator-13-debug-tests-wk2/build/layout-test-results/animations/animation-direction-normal-actual.txt
@@ -1,4 +1,4 @@
-PASS - "margin-left" property for "box" element at 0.5s saw something close to: 50
+FAIL - "margin-left" property for "box" element at 0.5s expected: 50 but saw: 18.200000762939453
 PASS - "margin-left" property for "box" element at 1s saw something close to: 100
 PASS - "margin-left" property for "box" element at 2.5s saw something close to: 50
Comment 1 Radar WebKit Bug Importer 2020-04-07 14:53:54 PDT
<rdar://problem/61411725>
Comment 2 Antoine Quint 2020-04-22 05:23:43 PDT
This test disables the use of a pause API and thus relies exclusively on timers. Presumably this is to check the runtime behavior of the test, but then this makes the test flaky by design since it relies on timers.

We should probably nuke such tests, I don't think they serve much value.
Comment 3 Antoine Quint 2020-04-22 05:57:35 PDT
I'll still try something to improve the situation and use a requestAnimationFrame() loop rather than set timers.
Comment 4 Antoine Quint 2020-04-22 09:22:34 PDT
That won't help either. This test needs to be rewritten completely.
Comment 5 Antoine Quint 2020-04-23 09:46:57 PDT
*** Bug 209362 has been marked as a duplicate of this bug. ***
Comment 6 Antoine Quint 2020-04-23 09:56:10 PDT
Created attachment 397350 [details]
Patch
Comment 7 Simon Fraser (smfr) 2020-04-23 10:01:22 PDT
Comment on attachment 397350 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397350&action=review

> LayoutTests/animations/resources/animation-test.js:34
> +        this._target = options.target;

"target" is a bit generic. Maybe targetProperty?

> LayoutTests/animations/resources/animation-test.js:91
> +        // Reset the animation.
> +        animation.cancel();
> +
> +        // Make sure it's in the paused state for seeking.
> +        animation.pause();

Is the cancel then pause really necessary?
Comment 8 Antoine Quint 2020-04-23 10:04:28 PDT
(In reply to Simon Fraser (smfr) from comment #7)
> Comment on attachment 397350 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=397350&action=review
> 
> > LayoutTests/animations/resources/animation-test.js:34
> > +        this._target = options.target;
> 
> "target" is a bit generic. Maybe targetProperty?

This is an element and it matches the terminology for KeyframeEffect::target.

> > LayoutTests/animations/resources/animation-test.js:91
> > +        // Reset the animation.
> > +        animation.cancel();
> > +
> > +        // Make sure it's in the paused state for seeking.
> > +        animation.pause();
> 
> Is the cancel then pause really necessary?

Yeah, I think we can get by with the cancel() call only.
Comment 9 Antoine Quint 2020-04-23 10:23:25 PDT
Committed r260577: <https://trac.webkit.org/changeset/260577>