Open Bug 1297305 Opened 8 years ago Updated 2 years ago

[Test] Move the test of document-timeline to web platform tests.

Categories

(Core :: DOM: Animation, defect, P5)

Product:
Core
Component:
DOM: Animation
Type:
defect

Tracking

()

People

(Reporter: mantaroh, Unassigned)

Details

(Keywords: good-first-bug)

Attachments

(1 file)

Bug 1297305 - Move the test of document-timeline to web platform tests,
58 bytes, text/x-review-board-request
birtles
: review+
Details 
We have document timeline test in dom/animation/test/document-timeline.
I think that this behavior of test is the behavior of specification[1], so we can move this test to the web platform test. (testing/web-platform/tests/web-animation/interface/DocumentTimeline/)

We should care about following point:
 - We prefer to use the promise_test instead of async_test.
 - We can't use the SimpleTest.js, use the testharness.js. [2]

[1] https://w3c.github.io/web-animations/#timelines
[2] https://github.com/w3c/testharness.js/blob/master/docs/api.md

Perhaps the changeset of bug 1258972 will help this bug.
Assignee: nobody → begeeben
Comment on attachment 8785550 [details]
Bug 1297305 - Move the test of document-timeline to web platform tests,

https://reviewboard.mozilla.org/r/74726/#review72570

Thanks for doing this. This looks good, but I'd like to check it again with the following changes:

::: testing/web-platform/tests/web-animations/interfaces/DocumentTimeline/document-timeline.html:3
(Diff revision 1)
> +<!doctype html>
> +<meta charset=utf-8>
> +<title>Web Animations API: DocumentTimeline tests</title>

Can we split this into two files:

* web-animations/interfaces/Document/timeline.html - the first test
* web-animations/interfaces/DocumentTimeline/currentTime.html - the remaining tests

?

::: testing/web-platform/tests/web-animations/interfaces/DocumentTimeline/document-timeline.html:24
(Diff revision 1)
> +{
> +  help:   'http://dev.w3.org/fxtf/web-animations/#the-document-timeline',
> +  assert: [ 'Each document has a timeline called the document timeline' ],
> +  author: 'Brian Birtles'
> +});

We can drop these lines since we no longer use them.

::: testing/web-platform/tests/web-animations/interfaces/DocumentTimeline/document-timeline.html:47
(Diff revision 1)
> +    t.step_func(function(rafTime) {
> +      assert_equals(document.timeline.currentTime, rafTime,
> +                    'document.timeline.currentTime matches' +
> +                    ' requestAnimationFrame time');
> +      t.done();
> +    });

I don't think we need step_func when we have a promise test?

See: https://github.com/w3c/testharness.js/blob/master/docs/api.md#promise-tests

::: testing/web-platform/tests/web-animations/interfaces/DocumentTimeline/document-timeline.html:56
(Diff revision 1)
> +{
> +  help: [
> +    'http://dev.w3.org/fxtf/web-animations/#the-global-clock',
> +    'http://dev.w3.org/fxtf/web-animations/#the-document-timeline'
> +  ],
> +  assert: [
> +    'The global clock is a source of monotonically increasing time values',
> +    'The time values of the document timeline are calculated as a fixed' +
> +    ' offset from the global clock',
> +    'the zero time corresponds to the navigationStart moment',
> +    'the time value of each document timeline must be equal to the time ' +
> +    'passed to animation frame request callbacks for that browsing context'
> +  ],
> +  author: 'Brian Birtles'
> +});

Again, I think we can drop need this metadata.

::: testing/web-platform/tests/web-animations/interfaces/DocumentTimeline/document-timeline.html:89
(Diff revision 1)
> +{
> +  help: 'http://dev.w3.org/fxtf/web-animations/#script-execution-and-live-updates-to-the-model',
> +  assert: [ 'The value returned by the currentTime attribute of a' +
> +            ' document timeline will not change within a script block' ],
> +  author: 'Brian Birtles'
> +});

We can drop this metadata too.

::: testing/web-platform/tests/web-animations/interfaces/DocumentTimeline/document-timeline.html:140
(Diff revision 1)
> +  if (hiddenIFrame.contentDocument.readyState === 'complete') {
> +    testToRunOnLoad();
> +  } else {
> +    hiddenIFrame.addEventListener("load", testToRunOnLoad);
> +  }

This promise_test doesn't return a promise so I think we need to rework it.
Attachment #8785550 - Flags: review?(bbirtles)
Comment on attachment 8785550 [details]
Bug 1297305 - Move the test of document-timeline to web platform tests,

https://reviewboard.mozilla.org/r/74726/#review72650

This Yifan for doing this! r=me with the following changes made.

Let me know if you don't have time to do it, however, and I can take care of it.

::: testing/web-platform/meta/MANIFEST.json:30465
(Diff revision 2)
> +        "path": "web-animations/interfaces/DocumentTimeline/document-timeline.html",
> +        "url": "/web-animations/interfaces/DocumentTimeline/document-timeline.html"

We should name this test:

web-animations/interfaces/DocumentTimeline/currentTime.html

::: testing/web-platform/meta/web-animations/interfaces/Animation/document-timeline.html.ini:1
(Diff revision 2)
> +[document-timeline.html]
> +  type: testharness
> +  [document.timeline.currentTime hidden subframe test]
> +    expected: FAIL
> +    bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1298573
> +

Do we still need this? This test passes for me.

In any case, this file seems to be in the wrong place (under 'Animation' instead of 'DocumentTimeline')

::: testing/web-platform/tests/web-animations/interfaces/Document/timeline.html:3
(Diff revision 2)
> +<!doctype html>
> +<meta charset=utf-8>
> +<title>Web Animations API: Timeline tests</title>

Let's just call this 'document.timeline tests'

::: testing/web-platform/tests/web-animations/interfaces/Document/timeline.html:4
(Diff revision 2)
> +<!doctype html>
> +<meta charset=utf-8>
> +<title>Web Animations API: Timeline tests</title>
> +<link rel="help" href="https://w3c.github.io/web-animations/#timelines">

Let's use the following URL here:

  https://w3c.github.io/web-animations/#the-documents-default-timeline

::: testing/web-platform/tests/web-animations/interfaces/DocumentTimeline/document-timeline.html:3
(Diff revision 2)
> +<!doctype html>
> +<meta charset=utf-8>
> +<title>Web Animations API: DocumentTimeline tests</title>

Likewise, let's just call this 'DocumentTimeline.currentTime tests'

::: testing/web-platform/tests/web-animations/interfaces/DocumentTimeline/document-timeline.html:4
(Diff revision 2)
> +<!doctype html>
> +<meta charset=utf-8>
> +<title>Web Animations API: DocumentTimeline tests</title>
> +<link rel="help" href="https://w3c.github.io/web-animations/#timelines">

Perhaps "https://w3c.github.io/web-animations/#document-timelines" is better here
Attachment #8785550 - Flags: review?(bbirtles) → review+
Oh, I've just noticed that we already have web-platform/tests/web-animations/interfaces/AnimationTimeline/document-timeline.html which contains half of these tests already. I guess we should remove that file after checking that we have the same tests here.
Pushed to try with the suggested changes to see if the failing test fails there too:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fe20aef56e5
Comment on attachment 8785550 [details]
Bug 1297305 - Move the test of document-timeline to web platform tests,

https://reviewboard.mozilla.org/r/74726/#review72650

Thank you! I can do this.
(In reply to Brian Birtles (:birtles) from comment #6)
> Pushed to try with the suggested changes to see if the failing test fails
> there too:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fe20aef56e5

Oops, left a stray comma there. Trying again:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b44e64c3aed
Comment on attachment 8785550 [details]
Bug 1297305 - Move the test of document-timeline to web platform tests,

https://reviewboard.mozilla.org/r/74726/#review72650

> Do we still need this? This test passes for me.
> 
> In any case, this file seems to be in the wrong place (under 'Animation' instead of 'DocumentTimeline')

It still fails on my laptop, maybe it's a platform thing?
Comment on attachment 8785550 [details]
Bug 1297305 - Move the test of document-timeline to web platform tests,

https://reviewboard.mozilla.org/r/74726/#review72650

> It still fails on my laptop, maybe it's a platform thing?

I ran the test on a Mac.
Just FYI, I did run the test on Linux.  It fails when I run only currentTime.html, it does not fail if I run both of test files, i.e. specifying DocumentTimeline directory.
Keywords: good-first-bug
Whiteboard: [good first bug]

This good-first-bug hasn't had any activity for 6 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Assignee: begeeben → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: