New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update playbackRate tests to check for behavior when set to an unsupported value. #6522

Merged
merged 2 commits into from Jul 19, 2017

Conversation

Projects
None yet
7 participants
@japacible
Contributor

japacible commented Jul 11, 2017

These tests coincide with updating the playbackRate behavior noted in whatwg/html#2754.

PTAL, thanks!

@sideshowbarker

This comment has been minimized.

Show comment
Hide comment
@sideshowbarker

sideshowbarker Jul 12, 2017

Member

w3c-test:mirror

Member

sideshowbarker commented Jul 12, 2017

w3c-test:mirror

@w3c-bots

This comment has been minimized.

Show comment
Hide comment
@w3c-bots

w3c-bots Jul 18, 2017

View the complete job log.

Firefox (nightly)

Testing web-platform-tests at revision 795d71c
Using browser at version BuildID 20170717100212; SourceStamp aff336ac161daa3ea350e59a288963edbd58ed39
Starting 10 test iterations
All results were stable

All results

1 test ran
/html/semantics/embedded-content/media-elements/playing-the-media-resource/playbackRate.html
Subtest Results Messages
ERROR
playbackRate initial value PASS
playbackRate set to small positive value PASS
playbackRate set to large positive value TIMEOUT Test timed out
playbackRate set to small negative value FAIL assert_equals: expected "NotSupportedError" but got "NS_ERROR_NOT_IMPLEMENTED"
playbackRate set to large negative value FAIL assert_equals: expected "NotSupportedError" but got "NS_ERROR_NOT_IMPLEMENTED"
playbackRate set to 0 PASS
playbackRate set to -1 FAIL assert_equals: expected "NotSupportedError" but got "NS_ERROR_NOT_IMPLEMENTED"

w3c-bots commented Jul 18, 2017

View the complete job log.

Firefox (nightly)

Testing web-platform-tests at revision 795d71c
Using browser at version BuildID 20170717100212; SourceStamp aff336ac161daa3ea350e59a288963edbd58ed39
Starting 10 test iterations
All results were stable

All results

1 test ran
/html/semantics/embedded-content/media-elements/playing-the-media-resource/playbackRate.html
Subtest Results Messages
ERROR
playbackRate initial value PASS
playbackRate set to small positive value PASS
playbackRate set to large positive value TIMEOUT Test timed out
playbackRate set to small negative value FAIL assert_equals: expected "NotSupportedError" but got "NS_ERROR_NOT_IMPLEMENTED"
playbackRate set to large negative value FAIL assert_equals: expected "NotSupportedError" but got "NS_ERROR_NOT_IMPLEMENTED"
playbackRate set to 0 PASS
playbackRate set to -1 FAIL assert_equals: expected "NotSupportedError" but got "NS_ERROR_NOT_IMPLEMENTED"
@w3c-bots

This comment has been minimized.

Show comment
Hide comment
@w3c-bots

w3c-bots Jul 18, 2017

View the complete job log.

Sauce (safari)

Testing web-platform-tests at revision 795d71c
Using browser at version 10.0
Starting 10 test iterations
All results were stable

All results

1 test ran
/html/semantics/embedded-content/media-elements/playing-the-media-resource/playbackRate.html
Subtest Results Messages
OK
playbackRate initial value PASS
playbackRate set to small positive value PASS
playbackRate set to large positive value PASS
playbackRate set to small negative value PASS
playbackRate set to large negative value PASS
playbackRate set to 0 PASS
playbackRate set to -1 PASS

w3c-bots commented Jul 18, 2017

View the complete job log.

Sauce (safari)

Testing web-platform-tests at revision 795d71c
Using browser at version 10.0
Starting 10 test iterations
All results were stable

All results

1 test ran
/html/semantics/embedded-content/media-elements/playing-the-media-resource/playbackRate.html
Subtest Results Messages
OK
playbackRate initial value PASS
playbackRate set to small positive value PASS
playbackRate set to large positive value PASS
playbackRate set to small negative value PASS
playbackRate set to large negative value PASS
playbackRate set to 0 PASS
playbackRate set to -1 PASS
@w3c-bots

This comment has been minimized.

Show comment
Hide comment
@w3c-bots

w3c-bots Jul 18, 2017

View the complete job log.

Chrome (unstable)

Testing web-platform-tests at revision 795d71c
Using browser at version 61.0.3153.4 dev
Starting 10 test iterations
All results were stable

All results

1 test ran
/html/semantics/embedded-content/media-elements/playing-the-media-resource/playbackRate.html
Subtest Results Messages
OK
playbackRate initial value PASS
playbackRate set to small positive value PASS
playbackRate set to large positive value PASS
playbackRate set to small negative value PASS
playbackRate set to large negative value PASS
playbackRate set to 0 PASS
playbackRate set to -1 PASS

w3c-bots commented Jul 18, 2017

View the complete job log.

Chrome (unstable)

Testing web-platform-tests at revision 795d71c
Using browser at version 61.0.3153.4 dev
Starting 10 test iterations
All results were stable

All results

1 test ran
/html/semantics/embedded-content/media-elements/playing-the-media-resource/playbackRate.html
Subtest Results Messages
OK
playbackRate initial value PASS
playbackRate set to small positive value PASS
playbackRate set to large positive value PASS
playbackRate set to small negative value PASS
playbackRate set to large negative value PASS
playbackRate set to 0 PASS
playbackRate set to -1 PASS
@w3c-bots

This comment has been minimized.

Show comment
Hide comment
@w3c-bots

w3c-bots Jul 18, 2017

View the complete job log.

Sauce (MicrosoftEdge)

Testing web-platform-tests at revision 795d71c
Using browser at version 14.14393
Starting 10 test iterations
All results were stable

All results

1 test ran
/html/semantics/embedded-content/media-elements/playing-the-media-resource/playbackRate.html
Subtest Results Messages
OK
playbackRate initial value PASS
playbackRate set to small positive value PASS
playbackRate set to large positive value PASS
playbackRate set to small negative value PASS
playbackRate set to large negative value PASS
playbackRate set to 0 PASS
playbackRate set to -1 PASS

w3c-bots commented Jul 18, 2017

View the complete job log.

Sauce (MicrosoftEdge)

Testing web-platform-tests at revision 795d71c
Using browser at version 14.14393
Starting 10 test iterations
All results were stable

All results

1 test ran
/html/semantics/embedded-content/media-elements/playing-the-media-resource/playbackRate.html
Subtest Results Messages
OK
playbackRate initial value PASS
playbackRate set to small positive value PASS
playbackRate set to large positive value PASS
playbackRate set to small negative value PASS
playbackRate set to large negative value PASS
playbackRate set to 0 PASS
playbackRate set to -1 PASS
@domenic

Tests LGTM. We can merge when the spec PR is merged.

Unfortunately it seems that this tests still pass in browsers like Chrome stable where setting the playbackRate to a not-supported value just changes playbackRate but does not actually update the playback speed.

Do you think there's any way to write a test of the actual playback speed without relying on browser internals? Stated another way, how do you plan on testing this change in Chrome?

v.addEventListener('ratechange', t.step_func(function() {
var initialRate = v.playbackRate;
v.addEventListener('ratechange', function() {

This comment has been minimized.

@domenic

domenic Jul 18, 2017

Member

Oh, wait, there's a slightly better way to do this. If you do

v.addEventListener('ratechange', t.step_func(function() {
  ...
  t.done();
}));

then exceptions thrown will properly be redirected to the test harness. That should cause Firefox to fail the test, instead of timing out.

You can then make it even shorter with an extra shortcut:

v.addEventListener('ratechange', t.step_func_done(function() {
  ...
}));
@domenic

domenic Jul 18, 2017

Member

Oh, wait, there's a slightly better way to do this. If you do

v.addEventListener('ratechange', t.step_func(function() {
  ...
  t.done();
}));

then exceptions thrown will properly be redirected to the test harness. That should cause Firefox to fail the test, instead of timing out.

You can then make it even shorter with an extra shortcut:

v.addEventListener('ratechange', t.step_func_done(function() {
  ...
}));

This comment has been minimized.

@japacible

japacible Jul 20, 2017

Contributor

Opened another pull request to address this feedback.

@japacible

japacible Jul 20, 2017

Contributor

Opened another pull request to address this feedback.

@mounirlamouri

This comment has been minimized.

Show comment
Hide comment
@mounirlamouri

mounirlamouri Jul 18, 2017

Contributor

I guess we could test the playback rate by listening to two progress events, check how much time was spent in between and check if the currentTime of the video is more or less equal to the delta multiplied by the playback rate. This will not be super precise because there might be imprecisions.

Contributor

mounirlamouri commented Jul 18, 2017

I guess we could test the playback rate by listening to two progress events, check how much time was spent in between and check if the currentTime of the video is more or less equal to the delta multiplied by the playback rate. This will not be super precise because there might be imprecisions.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Jul 18, 2017

Member

Yeah, up to you all whether you think there's a good way to write tests that add value (instead of e.g. being too flaky to be valuable). It's not a prerequisite on the spec side; just a nice to have I am noting while in the area. It'll especially make it easier to file bugs on other browsers by pointing them to failing tests.

Member

domenic commented Jul 18, 2017

Yeah, up to you all whether you think there's a good way to write tests that add value (instead of e.g. being too flaky to be valuable). It's not a prerequisite on the spec side; just a nice to have I am noting while in the area. It'll especially make it easier to file bugs on other browsers by pointing them to failing tests.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Jul 19, 2017

Member

I'm confused. Can the tests be squashed and merged or not?

Member

annevk commented Jul 19, 2017

I'm confused. Can the tests be squashed and merged or not?

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Jul 19, 2017

Member

Yes, once the spec change lands. We're just discussing further tests that could be added on top.

Member

domenic commented Jul 19, 2017

Yes, once the spec change lands. We're just discussing further tests that could be added on top.

@annevk annevk merged commit a5c429f into web-platform-tests:master Jul 19, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

annevk added a commit to whatwg/html that referenced this pull request Jul 19, 2017

domenic referenced this pull request Jul 20, 2017

Fix playbackRate tests to redirect exceptions into test harness
See It makes all cross-origin properties that would normally be enumerable
on same-origin objects, enumerable also on WindowProxy and Location
objects (including when accessed same-origin).

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Aug 25, 2017

Bug 1382780 - part1 : return NS_ERROR_DOM_NOT_SUPPORTED_ERR for the n…
…egative playback rate. r=cpearce

According to [1], we should return NotSupportedError for the negative playback rate.

[1] web-platform-tests/wpt#6522

MozReview-Commit-ID: KoqDkBmP3h9

--HG--
extra : rebase_source : ddf1cdd03a980686df6d6613e1bd507bf37d8337

luke-chang pushed a commit to luke-chang/gecko that referenced this pull request Aug 25, 2017

Bug 1382780 - part1 : return NS_ERROR_DOM_NOT_SUPPORTED_ERR for the n…
…egative playback rate. r=cpearce

According to [1], we should return NotSupportedError for the negative playback rate.

[1] web-platform-tests/wpt#6522

MozReview-Commit-ID: KoqDkBmP3h9

aethanyc pushed a commit to aethanyc/gecko-dev that referenced this pull request Aug 25, 2017

Bug 1382780 - part1 : return NS_ERROR_DOM_NOT_SUPPORTED_ERR for the n…
…egative playback rate. r=cpearce

According to [1], we should return NotSupportedError for the negative playback rate.

[1] web-platform-tests/wpt#6522

MozReview-Commit-ID: KoqDkBmP3h9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment