Bug 210156 – [ Mac iOS ] animations/animation-direction-normal.html is a flaky failure

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Truitt Savell]

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

[Radar WebKit Bug Importer]

<rdar://problem/61411725>

[Antoine Quint]

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.

[Antoine Quint]

I'll still try something to improve the situation and use a requestAnimationFrame() loop rather than set timers.

[Antoine Quint]

That won't help either. This test needs to be rewritten completely.

[Antoine Quint]

*** Bug 209362 has been marked as a duplicate of this bug. ***

[Antoine Quint]

Created attachment 397350 [details]
Patch

[Simon Fraser (smfr)]

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?

[Antoine Quint]

(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.

[Antoine Quint]

Committed r260577: <https://trac.webkit.org/changeset/260577>